aboutsummaryrefslogtreecommitdiff
path: root/src/backend/commands
Commit message (Collapse)AuthorAge
* Invalidate relcache for publications defined for all tables.Amit Kapila2021-09-08
| | | | | | | | | | | | | | | Updates/Deletes on a relation were allowed even without replica identity after we define the publication for all tables. This would later lead to an error on subscribers. The reason was that for such publications we were not invalidating the relcache and the publication information for relations was not getting rebuilt. Similarly, we were not invalidating the relcache after dropping of such publications which will prohibit Updates/Deletes without replica identity even without any publication. Author: Vignesh C and Hou Zhijie Reviewed-by: Hou Zhijie, Kyotaro Horiguchi, Amit Kapila Backpatch-through: 10, where it was introduced Discussion: https://postgr.es/m/CALDaNm0pF6zeWqCA8TCe2sDuwFAy8fCqba=nHampCKag-qLixg@mail.gmail.com
* Disallow creating an ICU collation if the DB encoding won't support it.Tom Lane2021-09-03
| | | | | | | | | | | | | | | | Previously this was allowed, but the collation effectively vanished into the ether because of the way lookup_collation() works: you could not use the collation, nor even drop it. Seems better to give an error up front than to leave the user wondering why it doesn't work. (Because this test is in DefineCollation not CreateCollation, it does not prevent pg_import_system_collations from creating ICU collations, regardless of the initially-chosen encoding.) Per bug #17170 from Andrew Bille. Back-patch to v10 where ICU support was added. Discussion: https://postgr.es/m/17170-95845cf3f0a9c36d@postgresql.org
* Fix data loss in wal_level=minimal crash recovery of CREATE TABLESPACE.Noah Misch2021-08-27
| | | | | | | | | | | | | | | | | | | | | | | | | If the system crashed between CREATE TABLESPACE and the next checkpoint, the result could be some files in the tablespace unexpectedly containing no rows. Affected files would be those for which the system did not write WAL; see the wal_skip_threshold documentation. Before v13, a different set of conditions governed the writing of WAL; see v12's <sect2 id="populate-pitr">. (The v12 conditions were broader in some ways and narrower in others.) Users may want to audit non-default tablespaces for unexpected short files. The bug could have truncated an index without affecting the associated table, and reindexing the index would fix that particular problem. This fixes the bug by making create_tablespace_directories() more like TablespaceCreateDbspace(). create_tablespace_directories() was recursively removing tablespace contents, reasoning that WAL redo would recreate everything removed that way. That assumption holds for other wal_level values. Under wal_level=minimal, the old approach could delete files for which no other copy existed. Back-patch to 9.6 (all supported versions). Reviewed by Robert Haas and Prabhat Sahu. Reported by Robert Haas. Discussion: https://postgr.es/m/CA+TgmoaLO9ncuwvr2nN-J4VEP5XyAcy=zKiHxQzBbFRxxGxm0w@mail.gmail.com
* Fix toast rewrites in logical decoding.Amit Kapila2021-08-25
| | | | | | | | | | | | | | | | | | | Commit 325f2ec555 introduced pg_class.relwrite to skip operations on tables created as part of a heap rewrite during DDL. It links such transient heaps to the original relation OID via this new field in pg_class but forgot to do anything about toast tables. So, logical decoding was not able to skip operations on internally created toast tables. This leads to an error when we tried to decode the WAL for the next operation for which it appeared that there is a toast data where actually it didn't have any toast data. To fix this, we set pg_class.relwrite for internally created toast tables as well which allowed skipping operations on them during logical decoding. Author: Bertrand Drouvot Reviewed-by: David Zhang, Amit Kapila Backpatch-through: 11, where it was introduced Discussion: https://postgr.es/m/b5146fb1-ad9e-7d6e-f980-98ed68744a7c@amazon.com
* Prevent ALTER TYPE/DOMAIN/OPERATOR from changing extension membership.Tom Lane2021-08-17
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | If recordDependencyOnCurrentExtension is invoked on a pre-existing, free-standing object during an extension update script, that object will become owned by the extension. In our current code this is possible in three cases: * Replacing a "shell" type or operator. * CREATE OR REPLACE overwriting an existing object. * ALTER TYPE SET, ALTER DOMAIN SET, and ALTER OPERATOR SET. The first of these cases is intentional behavior, as noted by the existing comments for GenerateTypeDependencies. It seems like appropriate behavior for CREATE OR REPLACE too; at least, the obvious alternatives are not better. However, the fact that it happens during ALTER is an artifact of trying to share code (GenerateTypeDependencies and makeOperatorDependencies) between the CREATE and ALTER cases. Since an extension script would be unlikely to ALTER an object that didn't already belong to the extension, this behavior is not very troubling for the direct target object ... but ALTER TYPE SET will recurse to dependent domains, and it is very uncool for those to become owned by the extension if they were not already. Let's fix this by redefining the ALTER cases to never change extension membership, full stop. We could minimize the behavioral change by only changing the behavior when ALTER TYPE SET is recursing to a domain, but that would complicate the code and it does not seem like a better definition. Per bug #17144 from Alex Kozhemyakin. Back-patch to v13 where ALTER TYPE SET was added. (The other cases are older, but since they only affect the directly-named object, there's not enough of a problem to justify changing the behavior further back.) Discussion: https://postgr.es/m/17144-e67d7a8f049de9af@postgresql.org
* Really fix the ambiguity in REFRESH MATERIALIZED VIEW CONCURRENTLY.Tom Lane2021-08-07
| | | | | | | | | | | | | | | | | | Rather than trying to pick table aliases that won't conflict with any possible user-defined matview column name, adjust the queries' syntax so that the aliases are only used in places where they can't be mistaken for column names. Mostly this consists of writing "alias.*" not just "alias", which adds clarity for humans as well as machines. We do have the issue that "SELECT alias.*" acts differently from "SELECT alias", but we can use the same hack ruleutils.c uses for whole-row variables in SELECT lists: write "alias.*::compositetype". We might as well revert to the original aliases after doing this; they're a bit easier to read. Like 75d66d10e, back-patch to all supported branches. Discussion: https://postgr.es/m/2488325.1628261320@sss.pgh.pa.us
* Don't allow to set replication slot_name as ''.Amit Kapila2021-07-19
| | | | | | | | | | | | | We don't allow to create replication slot_name as an empty string ('') via SQL API pg_create_logical_replication_slot() but it is allowed to be set via Alter Subscription command. This will lead to apply worker repeatedly keep trying to stream data via slot_name '' and the user is not allowed to create the slot with that name. Author: Japin Li Reviewed-By: Ranier Vilela, Amit Kapila Backpatch-through: 10, where it was introduced Discussion: https://postgr.es/m/MEYP282MB1669CBD98E721C77CA696499B61A9@MEYP282MB1669.AUSP282.PROD.OUTLOOK.COM
* Preserve firing-on state when cloning row triggers to partitionsAlvaro Herrera2021-07-16
| | | | | | | | | | | | | | | When triggers are cloned from partitioned tables to their partitions, the 'tgenabled' flag (origin/replica/always/disable) was not propagated. Make it so that the flag on the trigger on partition is initially set to the same value as on the partitioned table. Add a test case to verify the behavior. Backpatch to 11, where this appeared in commit 86f575948c77. Author: Álvaro Herrera <alvherre@alvh.no-ip.org> Reported-by: Justin Pryzby <pryzby@telsasoft.com> Discussion: https://postgr.es/m/20200930223450.GA14848@telsasoft.com
* Fix unexpected error messages for various flavors of ALTER TABLEMichael Paquier2021-07-14
| | | | | | | | | | | | | | | | | | | | | Some commands of ALTER TABLE could fail with the following error: ERROR: "tab" is of the wrong type This error is unexpected, as all the code paths leading to ATWrongRelkindError() should use a supported set of relkinds to generate correct error messages. This commit closes the gap with such mistakes, by adding all the missing relkind combinations. Tests are added to check all the problems found. Note that some combinations are not used, but these are left around as it could have an impact on applications relying on this code. 2ed532e has done a much larger refactoring on HEAD to make such error messages easier to manage in the long-term, so nothing is needed there. Author: Kyotaro Horiguchi Reviewed-by: Peter Eisentraut, Ahsan Hadi, Michael Paquier Discussion: https://postgr.es/m/20210216.181415.368926598204753659.horikyota.ntt@gmail.com Backpatch-through: 11
* Lock the extension during ALTER EXTENSION ADD/DROP.Tom Lane2021-07-11
| | | | | | | | | | | | | | | | | Although we were careful to lock the object being added or dropped, we failed to get any sort of lock on the extension itself. This allowed the ALTER to proceed in parallel with a DROP EXTENSION, which is problematic for a couple of reasons. If both commands succeeded we'd be left with a dangling link in pg_depend, which would cause problems later. Also, if the ALTER failed for some reason, it might try to print the extension's name, and that could result in a crash or (in older branches) a silly error message complaining about extension "(null)". Per bug #17098 from Alexander Lakhin. Back-patch to all supported branches. Discussion: https://postgr.es/m/17098-b960f3616c861f83@postgresql.org
* Remove unnecessary failure cases in RemoveRoleFromObjectPolicy().Tom Lane2021-06-25
| | | | | | | | | | | | | | | | | | | | | | | | It's not really necessary for this function to open or lock the relation associated with the pg_policy entry it's modifying. The error checks it's making on the rel are if anything counterproductive (e.g., if we don't want to allow installation of policies on system catalogs, here is not the place to prevent that). In particular, it seems just wrong to insist on an ownership check. That has the net effect of forcing people to use superuser for DROP OWNED BY, which surely is not an effect we want. Also there is no point in rebuilding the dependencies of the policy expressions, which aren't being changed. Lastly, locking the table also seems counterproductive; it's not helping to prevent race conditions, since we failed to re-read the pg_policy row after acquiring the lock. That means that concurrent DDL would likely result in "tuple concurrently updated/deleted" errors; which is the same behavior this code will produce, with less overhead. Per discussion of bug #17062. Back-patch to all supported versions, as the failure cases this eliminates seem just as undesirable in 9.6 as in HEAD. Discussion: https://postgr.es/m/1573181.1624220108@sss.pgh.pa.us
* Fix misbehavior of DROP OWNED BY with duplicate polroles entries.Tom Lane2021-06-18
| | | | | | | | | | | | | | | | | | Ordinarily, a pg_policy.polroles array wouldn't list the same role more than once; but CREATE POLICY does not prevent that. If we perform DROP OWNED BY on a role that is listed more than once, RemoveRoleFromObjectPolicy either suffered an assertion failure or encountered a tuple-updated-by-self error. Rewrite it to cope correctly with duplicate entries, and add a CommandCounterIncrement call to prevent the other problem. Per discussion, there's other cleanup that ought to happen here, but this seems like the minimum essential fix. Per bug #17062 from Alexander Lakhin. It's been broken all along, so back-patch to all supported branches. Discussion: https://postgr.es/m/17062-11f471ae3199ca23@postgresql.org
* Avoid scribbling on input node tree in CREATE/ALTER DOMAIN.Tom Lane2021-06-18
| | | | | | | | | | | | | | | | | This works fine in the "simple Query" code path; but if the statement is in the plan cache then it's corrupted for future re-execution. Apply copyObject() to protect the original tree from modification, as we've done elsewhere. This narrow fix is applied only to the back branches. In HEAD, the problem was fixed more generally by commit 7c337b6b5; but that changed ProcessUtility's API, so it's infeasible to back-patch. Per bug #17053 from Charles Samborski. Discussion: https://postgr.es/m/931771.1623893989@sss.pgh.pa.us Discussion: https://postgr.es/m/17053-3ca3f501bbc212b4@postgresql.org
* Don't set a fast default for anything but a plain tableAndrew Dunstan2021-06-18
| | | | | | | | | | | | | | | | | The fast default code added in Release 11 omitted to check that the table a fast default was being added to was a plain table. Thus one could be added to a foreign table, which predicably blows up. Here we perform that check. In addition, on the back branches, since some of these might have escaped into the wild, if we encounter a missing value for an attribute of something other than a plain table we ignore it. Fixes bug #17056 Backpatch to release 11, Reviewed by: Andres Freund, Álvaro Herrera and Tom Lane
* Fix plancache refcount leak after error in ExecuteQuery.Tom Lane2021-06-16
| | | | | | | | | | | | | | | | | When stuffing a plan from the plancache into a Portal, one is not supposed to risk throwing an error between GetCachedPlan and PortalDefineQuery; if that happens, the plan refcount incremented by GetCachedPlan will be leaked. I managed to break this rule while refactoring code in 9dbf2b7d7. There is no visible consequence other than some memory leakage, and since nobody is very likely to trigger the relevant error conditions many times in a row, it's not surprising we haven't noticed. Nonetheless, it's a bug, so rearrange the order of operations to remove the hazard. Noted on the way to looking for a better fix for bug #17053. This mistake is pretty old, so back-patch to all supported branches.
* Avoid misbehavior when persisting a non-stable cursor.Tom Lane2021-06-08
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | PersistHoldablePortal has long assumed that it should store the entire output of the query-to-be-persisted, which requires rewinding and re-reading the output. This is problematic if the query is not stable: we might get different row contents, or even a different number of rows, which'd confuse the cursor state mightily. In the case where the cursor is NO SCROLL, this is very easy to solve: just store the remaining query output, without any rewinding, and tweak the portal's cursor state to match. Aside from removing the semantic problem, this could be significantly more efficient than storing the whole output. If the cursor is scrollable, there's not much we can do, but it was already the case that scrolling a volatile query's result was pretty unsafe. We can just document more clearly that getting correct results from that is not guaranteed. There are already prohibitions in place on using SCROLL with FOR UPDATE/SHARE, which is one way for a SELECT query to have non-stable results. We could imagine prohibiting SCROLL when the query contains volatile functions, but that would be expensive to enforce. Moreover, it could break applications that work just fine, if they have functions that are in fact stable but the user neglected to mark them so. So settle for documenting the hazard. While this problem has existed in some guise for a long time, it got a lot worse in v11, which introduced the possibility of persisting plpgsql cursors (perhaps implicit ones) even when they violate the rules for what can be marked WITH HOLD. Hence, I've chosen to back-patch to v11 but not further. Per bug #17050 from Алексей Булгаков. Discussion: https://postgr.es/m/17050-f77aa827dc85247c@postgresql.org
* Reduce risks of conflicts in internal queries of REFRESH MATVIEW CONCURRENTLYMichael Paquier2021-06-03
| | | | | | | | | | | | | | | | | | | | | The internal SQL queries used by REFRESH MATERIALIZED VIEW CONCURRENTLY include some aliases for its diff and temporary relations with rather-generic names: diff, newdata, newdata2 and mv. Depending on the queries used for the materialized view, using CONCURRENTLY could lead to some internal failures if the query and those internal aliases conflict. Those names have been chosen in 841c29c8. This commit switches instead to a naming pattern which is less likely going to cause conflicts, based on an idea from Thomas Munro, by appending _$ to those aliases. This is not perfect as those new names could still conflict, but at least it has the advantage to keep the code readable and simple while reducing the likelihood of conflicts to be close to zero. Reported-by: Mathis Rudolf Author: Bharath Rupireddy Reviewed-by: Bernd Helmle, Thomas Munro, Michael Paquier Discussion: https://postgr.es/m/109c267a-10d2-3c53-b60e-720fcf44d9e8@credativ.de Backpatch-through: 9.6
* Improve docs and error messages for parallel vacuum.Amit Kapila2021-05-25
| | | | | | | | | | | | The error messages, docs, and one of the options were using 'parallel degree' to indicate parallelism used by vacuum command. We normally use 'parallel workers' at other places so change it for parallel vacuum accordingly. Author: Bharath Rupireddy Reviewed-by: Dilip Kumar, Amit Kapila Backpatch-through: 13 Discussion: https://postgr.es/m/CALj2ACWz=PYrrFXVsEKb9J1aiX4raA+UBe02hdRp_zqDkrWUiw@mail.gmail.com
* Fix access to no-longer-open relcache entry in logical-rep worker.Tom Lane2021-05-22
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | If we redirected a replicated tuple operation into a partition child table, and then tried to fire AFTER triggers for that event, the relation cache entry for the child table was already closed. This has no visible ill effects as long as the entry is still there and still valid, but an unluckily-timed cache flush could result in a crash or other misbehavior. To fix, postpone the ExecCleanupTupleRouting call (which is what closes the child table) until after we've fired triggers. This requires a bit of refactoring so that the cleanup function can have access to the necessary state. In HEAD, I took the opportunity to simplify some of worker.c's function APIs based on use of the new ApplyExecutionData struct. However, it doesn't seem safe/practical to back-patch that aspect, at least not without a lot of analysis of possible interactions with a04daa97a. In passing, add an Assert to afterTriggerInvokeEvents to catch such cases. This seems worthwhile because we've grown a number of fairly unstructured ways of calling AfterTriggerEndQuery. Back-patch to v13, where worker.c grew the ability to deal with partitioned target tables. Discussion: https://postgr.es/m/3382681.1621381328@sss.pgh.pa.us
* Fix usage of "tableoid" in GENERATED expressions.Tom Lane2021-05-21
| | | | | | | | | | | | | | | We consider this supported (though I've got my doubts that it's a good idea, because tableoid is not immutable). However, several code paths failed to fill the field in soon enough, causing such a GENERATED expression to see zero or the wrong value. This occurred when ALTER TABLE adds a new GENERATED column to a table with existing rows, and during regular INSERT or UPDATE on a foreign table with GENERATED columns. Noted during investigation of a report from Vitaly Ustinov. Back-patch to v12 where GENERATED came in. Discussion: https://postgr.es/m/CAM_DEiWR2DPT6U4xb-Ehigozzd3n3G37ZB1+867zbsEVtYoJww@mail.gmail.com
* Restore the portal-level snapshot after procedure COMMIT/ROLLBACK.Tom Lane2021-05-21
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | COMMIT/ROLLBACK necessarily destroys all snapshots within the session. The original implementation of intra-procedure transactions just cavalierly did that, ignoring the fact that this left us executing in a rather different environment than normal. In particular, it turns out that handling of toasted datums depends rather critically on there being an outer ActiveSnapshot: otherwise, when SPI or the core executor pop whatever snapshot they used and return, it's unsafe to dereference any toasted datums that may appear in the query result. It's possible to demonstrate "no known snapshots" and "missing chunk number N for toast value" errors as a result of this oversight. Historically this outer snapshot has been held by the Portal code, and that seems like a good plan to preserve. So add infrastructure to pquery.c to allow re-establishing the Portal-owned snapshot if it's not there anymore, and add enough bookkeeping support that we can tell whether it is or not. We can't, however, just re-establish the Portal snapshot as part of COMMIT/ROLLBACK. As in normal transaction start, acquiring the first snapshot should wait until after SET and LOCK commands. Hence, teach spi.c about doing this at the right time. (Note that this patch doesn't fix the problem for any PLs that try to run intra-procedure transactions without using SPI to execute SQL commands.) This makes SPI's no_snapshots parameter rather a misnomer, so in HEAD, rename that to allow_nonatomic. replication/logical/worker.c also needs some fixes, because it wasn't careful to hold a snapshot open around AFTER trigger execution. That code doesn't use a Portal, which I suspect someday we're gonna have to fix. But for now, just rearrange the order of operations. This includes back-patching the recent addition of finish_estate() to centralize the cleanup logic there. This also back-patches commit 2ecfeda3e into v13, to improve the test coverage for worker.c (it was that test that exposed that worker.c's snapshot management is wrong). Per bug #15990 from Andreas Wicht. Back-patch to v11 where intra-procedure COMMIT was added. Discussion: https://postgr.es/m/15990-eee2ac466b11293d@postgresql.org
* AlterSubscription_refresh: avoid stomping on global variableAlvaro Herrera2021-05-07
| | | | | | | | | | | | | | | | | | | | | | | This patch replaces use of the global "wrconn" variable in AlterSubscription_refresh with a local variable of the same name, making it consistent with other functions in subscriptioncmds.c (e.g. DropSubscription). The global wrconn is only meant to be used for logical apply/tablesync worker. Abusing it this way is known to cause trouble if an apply worker manages to do a subscription refresh, such as reported by Jeremy Finzel and diagnosed by Andres Freund back in November 2020, at https://www.postgresql.org/message-id/20201111215820.qihhrz7fayu6myfi@alap3.anarazel.de Backpatch to 10. In branch master, also move the connection establishment to occur outside the PG_TRY block; this way we can remove a test for NULL in PG_FINALLY, and it also makes the code more consistent with similar code in the same file. Author: Peter Smith <peter.b.smith@fujitsu.com> Reviewed-by: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> Reviewed-by: Japin Li <japinli@hotmail.com> Discussion: https://postgr.es/m/CAHut+Pu7Jv9L2BOEx_Z0UtJxfDevQSAUW2mJqWU+CtmDrEZVAg@mail.gmail.com
* Have ALTER CONSTRAINT recurse on partitioned tablesAlvaro Herrera2021-05-05
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | When ALTER TABLE .. ALTER CONSTRAINT changes deferrability properties changed in a partitioned table, we failed to propagate those changes correctly to partitions and to triggers. Repair by adding a recursion mechanism to affect all derived constraints and all derived triggers. (In particular, recurse to partitions even if their respective parents are already in the desired state: it is possible for the partitions to have been altered individually.) Because foreign keys involve tables in two sides, we cannot use the standard ALTER TABLE recursion mechanism, so we invent our own by following pg_constraint.conparentid down. When ALTER TABLE .. ALTER CONSTRAINT is invoked on the derived pg_constraint object that's automaticaly created in a partition as a result of a constraint added to its parent, raise an error instead of pretending to work and then failing to modify all the affected triggers. Before this commit such a command would be allowed but failed to affect all triggers, so it would silently misbehave. (Restoring dumps of existing databases is not affected, because pg_dump does not produce anything for such a derived constraint anyway.) Add some tests for the case. Backpatch to 11, where foreign key support was added to partitioned tables by commit 3de241dba86f. (A related change is commit f56f8f8da6af in pg12 which added support for FKs *referencing* partitioned tables; this is what forces us to use an ad-hoc recursion mechanism for this.) Diagnosed by Tom Lane from bug report from Ron L Johnson. As of this writing, no reviews were offered. Discussion: https://postgr.es/m/75fe0761-a291-86a9-c8d8-4906da077469@gmail.com Discussion: https://postgr.es/m/3144850.1607369633@sss.pgh.pa.us
* Fix OID passed to object-alter hook during ALTER CONSTRAINTAlvaro Herrera2021-05-04
| | | | | | | | | | The OID of the constraint is used instead of the OID of the trigger -- an easy mistake to make. Apparently the object-alter hooks are not very well tested :-( Backpatch to 12, where this typo was introduced by 578b229718e8 Discussion: https://postgr.es/m/20210503231633.GA6994@alvherre.pgsql
* Fix ALTER TABLE / INHERIT with generated columnsPeter Eisentraut2021-05-04
| | | | | | | | | When running ALTER TABLE t2 INHERIT t1, we must check that columns in t2 that correspond to a generated column in t1 are also generated and have the same generation expression. Otherwise, this would allow creating setups that a normal CREATE TABLE sequence would not allow. Discussion: https://www.postgresql.org/message-id/22de27f6-7096-8d96-4619-7b882932ca25@2ndquadrant.com
* Fix some inappropriately-disallowed uses of ALTER ROLE/DATABASE SET.Tom Lane2021-04-13
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Most GUC check hooks that inspect database state have special checks that prevent them from throwing hard errors for state-dependent issues when source == PGC_S_TEST. This allows, for example, "ALTER DATABASE d SET default_text_search_config = foo" when the "foo" configuration hasn't been created yet. Without this, we have problems during dump/reload or pg_upgrade, because pg_dump has no idea about possible dependencies of GUC values and can't ensure a safe restore ordering. However, check_role() and check_session_authorization() hadn't gotten the memo about that, and would throw hard errors anyway. It's not entirely clear what is the use-case for "ALTER ROLE x SET role = y", but we've now heard two independent complaints about that bollixing an upgrade, so apparently some people are doing it. Hence, fix these two functions to act more like other check hooks with similar needs. (But I did not change their insistence on being inside a transaction, as it's still not apparent that setting either GUC from the configuration file would be wise.) Also fix check_temp_buffers, which had a different form of the disease of making state-dependent checks without any exception for PGC_S_TEST. A cursory survey of other GUC check hooks did not find any more issues of this ilk. (There are a lot of interdependencies among PGC_POSTMASTER and PGC_SIGHUP GUCs, which may be a bad idea, but they're not relevant to the immediate concern because they can't be set via ALTER ROLE/DATABASE.) Per reports from Charlie Hornsby and Nathan Bossart. Back-patch to all supported branches. Discussion: https://postgr.es/m/HE1P189MB0523B31598B0C772C908088DB7709@HE1P189MB0523.EURP189.PROD.OUTLOOK.COM Discussion: https://postgr.es/m/20160711223641.1426.86096@wrigleys.postgresql.org
* Remove StoreSingleInheritance reimplementationAlvaro Herrera2021-03-25
| | | | | | | I introduced this duplicate code in commit 8b08f7d4820f for no good reason. Remove it, and backpatch to 11 where it was introduced. Author: Álvaro Herrera <alvherre@alvh.no-ip.org>
* VACUUM ANALYZE: Always update pg_class.reltuples.Peter Geoghegan2021-03-10
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | vacuumlazy.c sometimes fails to update pg_class entries for each index (to ensure that pg_class.reltuples is current), even though analyze.c assumed that that must have happened during VACUUM ANALYZE. There are at least a couple of reasons for this. For example, vacuumlazy.c could fail to update pg_class when the index AM indicated that its statistics are merely an estimate, per the contract for amvacuumcleanup() routines established by commit e57345975cf back in 2006. Stop assuming that pg_class must have been updated with accurate statistics within VACUUM ANALYZE -- update pg_class for indexes at the same time as the table relation in all cases. That way VACUUM ANALYZE will never fail to keep pg_class.reltuples reasonably accurate. The only downside of this approach (compared to the old approach) is that it might inaccurately set pg_class.reltuples for indexes whose heap relation ends up with the same inaccurate value anyway. This doesn't seem too bad. We already consistently called vac_update_relstats() (to update pg_class) for the heap/table relation twice during any VACUUM ANALYZE -- once in vacuumlazy.c, and once in analyze.c. We now make sure that we call vac_update_relstats() at least once (though often twice) for each index. This is follow up work to commit 9f3665fb, which dealt with issues in btvacuumcleanup(). Technically this fixes an unrelated issue, though. btvacuumcleanup() no longer provides an accurate num_index_tuples value following commit 9f3665fb (when there was no btbulkdelete() call during the VACUUM operation in question), but hashvacuumcleanup() has worked in the same way for many years now. Author: Peter Geoghegan <pg@bowt.ie> Reviewed-By: Masahiko Sawada <sawada.mshk@gmail.com> Discussion: https://postgr.es/m/CAH2-WzknxdComjhqo4SUxVFk_Q1171GJO2ZgHZ1Y6pion6u8rA@mail.gmail.com Backpatch: 13-, just like commit 9f3665fb.
* Validate the OID argument of pg_import_system_collations().Tom Lane2021-03-08
| | | | | | | | | | | | | "SELECT pg_import_system_collations(0)" caused an assertion failure. With a random nonzero argument --- or indeed with zero, in non-assert builds --- it would happily make pg_collation entries with garbage values of collnamespace. These are harmless as far as I can tell (unless maybe the OID happens to become used for a schema, later on?). In any case this isn't a security issue, since the function is superuser-only. But it seems like a gotcha for unwary DBAs, so let's add a check that the given OID belongs to some schema. Back-patch to v10 where this function was introduced.
* Fix use-after-free bug with AfterTriggersTableData.storeslotAlvaro Herrera2021-02-27
| | | | | | | | | | | | | | | AfterTriggerSaveEvent() wrongly allocates the slot in execution-span memory context, whereas the correct thing is to allocate it in a transaction-span context, because that's where the enclosing AfterTriggersTableData instance belongs into. Backpatch to 12 (the test back to 11, where it works well with no code changes, and it's good to have to confirm that the case was previously well supported); this bug seems introduced by commit ff11e7f4b9ae. Reported-by: Bertrand Drouvot <bdrouvot@amazon.com> Author: Amit Langote <amitlangote09@gmail.com> Discussion: https://postgr.es/m/39a71864-b120-5a5c-8cc5-c632b6f16761@amazon.com
* Fix permission checks on constraint violation errors on partitions.Heikki Linnakangas2021-02-08
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | If a cross-partition UPDATE violates a constraint on the target partition, and the columns in the new partition are in different physical order than in the parent, the error message can reveal columns that the user does not have SELECT permission on. A similar bug was fixed earlier in commit 804b6b6db4. The cause of the bug is that the callers of the ExecBuildSlotValueDescription() function got confused when constructing the list of modified columns. If the tuple was routed from a parent, we converted the tuple to the parent's format, but the list of modified columns was grabbed directly from the child's RTE entry. ExecUpdateLockMode() had a similar issue. That lead to confusion on which columns are key columns, leading to wrong tuple lock being taken on tables referenced by foreign keys, when a row is updated with INSERT ON CONFLICT UPDATE. A new isolation test is added for that corner case. With this patch, the ri_RangeTableIndex field is no longer set for partitions that don't have an entry in the range table. Previously, it was set to the RTE entry of the parent relation, but that was confusing. NOTE: This modifies the ResultRelInfo struct, replacing the ri_PartitionRoot field with ri_RootResultRelInfo. That's a bit risky to backpatch, because it breaks any extensions accessing the field. The change that ri_RangeTableIndex is not set for partitions could potentially break extensions, too. The ResultRelInfos are visible to FDWs at least, and this patch required small changes to postgres_fdw. Nevertheless, this seem like the least bad option. I don't think these fields widely used in extensions; I don't think there are FDWs out there that uses the FDW "direct update" API, other than postgres_fdw. If there is, you will get a compilation error, so hopefully it is caught quickly. Backpatch to 11, where support for both cross-partition UPDATEs, and unique indexes on partitioned tables, were added. Reviewed-by: Amit Langote Security: CVE-2021-3393
* Fix backslash-escaping multibyte chars in COPY FROM.Heikki Linnakangas2021-02-05
| | | | | | | | | | | | | | | | | | | | | | | If a multi-byte character is escaped with a backslash in TEXT mode input, and the encoding is one of the client-only encodings where the bytes after the first one can have an ASCII byte "embedded" in the char, we didn't skip the character correctly. After a backslash, we only skipped the first byte of the next character, so if it was a multi-byte character, we would try to process its second byte as if it was a separate character. If it was one of the characters with special meaning, like '\n', '\r', or another '\\', that would cause trouble. One such exmple is the byte sequence '\x5ca45c2e666f6f' in Big5 encoding. That's supposed to be [backslash][two-byte character][.][f][o][o], but because the second byte of the two-byte character is 0x5c, we incorrectly treat it as another backslash. And because the next character is a dot, we parse it as end-of-copy marker, and throw an "end-of-copy marker corrupt" error. Backpatch to all supported versions. Reviewed-by: John Naylor, Kyotaro Horiguchi Discussion: https://www.postgresql.org/message-id/a897f84f-8dca-8798-3139-07da5bb38728%40iki.fi
* Remove bogus restriction from BEFORE UPDATE triggersAlvaro Herrera2021-01-28
| | | | | | | | | | | | | | | | | | | | | | In trying to protect the user from inconsistent behavior, commit 487e9861d0cf "Enable BEFORE row-level triggers for partitioned tables" tried to prevent BEFORE UPDATE FOR EACH ROW triggers from moving the row from one partition to another. However, it turns out that the restriction is wrong in two ways: first, it fails spuriously, preventing valid situations from working, as in bug #16794; and second, they don't protect from any misbehavior, because tuple routing would cope anyway. Fix by removing that restriction. We keep the same restriction on BEFORE INSERT FOR EACH ROW triggers, though. It is valid and useful there. In the future we could remove it by having tuple reroute work for inserts as it does for updates. Backpatch to 13. Author: Álvaro Herrera <alvherre@alvh.no-ip.org> Reported-by: Phillip Menke <pg@pmenke.de> Discussion: https://postgr.es/m/16794-350a655580fbb9ae@postgresql.org
* Prevent excess SimpleLruTruncate() deletion.Noah Misch2021-01-16
| | | | | | | | | | | | | | | | | Every core SLRU wraps around. With the exception of pg_notify, the wrap point can fall in the middle of a page. Account for this in the PagePrecedes callback specification and in SimpleLruTruncate()'s use of said callback. Update each callback implementation to fit the new specification. This changes SerialPagePrecedesLogically() from the style of asyncQueuePagePrecedes() to the style of CLOGPagePrecedes(). (Whereas pg_clog and pg_serial share a key space, pg_serial is nothing like pg_notify.) The bug fixed here has the same symptoms and user followup steps as 592a589a04bd456410b853d86bd05faa9432cbbb. Back-patch to 9.5 (all supported versions). Reviewed by Andrey Borodin and (in earlier versions) by Tom Lane. Discussion: https://postgr.es/m/20190202083822.GC32531@gust.leadboat.com
* Disallow CREATE STATISTICS on system catalogsTomas Vondra2021-01-15
| | | | | | | | | | | | | | Add a check that CREATE STATISTICS does not add extended statistics on system catalogs, similarly to indexes etc. It can be overriden using the allow_system_table_mods GUC. This bug exists since 7b504eb282c, adding the extended statistics, so backpatch all the way back to PostgreSQL 10. Author: Tomas Vondra Reported-by: Dean Rasheed Backpatch-through: 10 Discussion: https://postgr.es/m/CAEZATCXAPrrOKwEsyZKQ4uzzJQWBCt6QAvOcgqRGdWwT1zb%2BrQ%40mail.gmail.com
* Prevent drop of tablespaces used by partitioned relationsAlvaro Herrera2021-01-14
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | When a tablespace is used in a partitioned relation (per commits ca4103025dfe in pg12 for tables and 33e6c34c3267 in pg11 for indexes), it is possible to drop the tablespace, potentially causing various problems. One such was reported in bug #16577, where a rewriting ALTER TABLE causes a server crash. Protect against this by using pg_shdepend to keep track of tablespaces when used for relations that don't keep physical files; we now abort a tablespace if we see that the tablespace is referenced from any partitioned relations. Backpatch this to 11, where this problem has been latent all along. We don't try to create pg_shdepend entries for existing partitioned indexes/tables, but any ones that are modified going forward will be protected. Note slight behavior change: when trying to drop a tablespace that contains both regular tables as well as partitioned ones, you'd previously get ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE and now you'll get ERRCODE_DEPENDENT_OBJECTS_STILL_EXIST. Arguably, the latter is more correct. It is possible to add protecting pg_shdepend entries for existing tables/indexes, by doing ALTER TABLE ONLY some_partitioned_table SET TABLESPACE pg_default; ALTER TABLE ONLY some_partitioned_table SET TABLESPACE original_tablespace; for each partitioned table/index that is not in the database default tablespace. Because these partitioned objects do not have storage, no file needs to be actually moved, so it shouldn't take more time than what's required to acquire locks. This query can be used to search for such relations: SELECT ... FROM pg_class WHERE relkind IN ('p', 'I') AND reltablespace <> 0 Reported-by: Alexander Lakhin <exclusion@gmail.com> Discussion: https://postgr.es/m/16577-881633a9f9894fd5@postgresql.org Author: Álvaro Herrera <alvherre@alvh.no-ip.org> Reviewed-by: Michael Paquier <michael@paquier.xyz>
* Avoid memcpy() with a NULL source pointer and count == 0Alvaro Herrera2020-12-01
| | | | | | | | | | | | | | | | When memcpy() is called on a pointer, the compiler is entitled to assume that the pointer is not null, which can lead to optimizing nearby code in potentially undesirable ways. We still want such optimizations (gcc's -fdelete-null-pointer-checks) in cases where they're valid. Related: commit 13bba02271dc. Backpatch to pg11, where this particular instance appeared. Reported-by: Ranier Vilela <ranier.vf@gmail.com> Reported-by: Zhihong Yu <zyu@yugabyte.com> Discussion: https://postgr.es/m/CAEudQApUndmQkr5fLrCKXQ7+ib44i7S+Kk93pyVThS85PnG3bQ@mail.gmail.com Discussion: https://postgr.es/m/CALNJ-vSdhwSM5f4tnNn1cdLHvXMVe_S+V3nR5GwNrmCPNB2VtQ@mail.gmail.com
* Remove leftover comments, left behind by removal of WITH OIDS.Heikki Linnakangas2020-11-30
| | | | | Author: Amit Langote Discussion: https://www.postgresql.org/message-id/CA%2BHiwqGaRoF3XrhPW-Y7P%2BG7bKo84Z_h%3DkQHvMh-80%3Dav3wmOw%40mail.gmail.com
* Fix a recently-introduced race condition in LISTEN/NOTIFY handling.Tom Lane2020-11-28
| | | | | | | | | | | | | | | | | | | | | | | | | | Commit 566372b3d fixed some race conditions involving concurrent SimpleLruTruncate calls, but it introduced new ones in async.c. A newly-listening backend could attempt to read Notify SLRU pages that were in process of being truncated, possibly causing an error. Also, the QUEUE_TAIL pointer could become set to a value that's not equal to the queue position of any backend. While that's fairly harmless in v13 and up (thanks to commit 51004c717), in older branches it resulted in near-permanent disabling of the queue truncation logic, so that continued use of NOTIFY led to queue-fill warnings and eventual inability to send any more notifies. (A server restart is enough to make that go away, but it's still pretty unpleasant.) The core of the problem is confusion about whether QUEUE_TAIL represents the "logical" tail of the queue (i.e., the oldest still-interesting data) or the "physical" tail (the oldest data we've not yet truncated away). To fix, split that into two variables. QUEUE_TAIL regains its definition as the logical tail, and we introduce a new variable to track the oldest un-truncated page. Per report from Mikael Gustavsson. Like the previous patch, back-patch to all supported branches. Discussion: https://postgr.es/m/1b8561412e8a4f038d7a491c8b922788@smhi.se
* Use "true" not "TRUE" in one ICU function call.Tom Lane2020-11-16
| | | | | | | | | | | | | This was evidently missed in commit 6337865f3, which generally did s/TRUE/true/ everywhere. It escaped notice up to now because ICU versions before ICU 68 provided definitions of "TRUE" and "FALSE" regardless. With ICU 68, it fails to compile. Per report from Condor. Back-patch to v11 where 6337865f3 came in. (I've not tested v10, where this call originated, but I imagine it's fine since we defined TRUE in c.h back then.) Discussion: https://postgr.es/m/7a6f3336165bfe3ca66abcda7966f9d0@stz-bg.com
* Fix fuzzy thinking about amcanmulticol versus amcaninclude.Tom Lane2020-11-15
| | | | | | | | | | | | | | | | | | These flags should be independent: in particular an index AM should be able to say that it supports include columns without necessarily supporting multiple key columns. The included-columns patch got this wrong, possibly aided by the fact that it didn't bother to update the documentation. While here, clarify some text about amcanreturn, which was a little vague about what should happen when amcanreturn reports that only some of the index columns are returnable. Noted while reviewing the SP-GiST included-columns patch, which quite incorrectly (and unsafely) changed SP-GiST to claim amcanmulticol = true as a workaround for this bug. Backpatch to v11 where included columns were introduced.
* In security-restricted operations, block enqueue of at-commit user code.Noah Misch2020-11-09
| | | | | | | | | | | | | | | | | | Specifically, this blocks DECLARE ... WITH HOLD and firing of deferred triggers within index expressions and materialized view queries. An attacker having permission to create non-temp objects in at least one schema could execute arbitrary SQL functions under the identity of the bootstrap superuser. One can work around the vulnerability by disabling autovacuum and not manually running ANALYZE, CLUSTER, REINDEX, CREATE INDEX, VACUUM FULL, or REFRESH MATERIALIZED VIEW. (Don't restore from pg_dump, since it runs some of those commands.) Plain VACUUM (without FULL) is safe, and all commands are fine when a trusted user owns the target object. Performance may degrade quickly under this workaround, however. Back-patch to 9.5 (all supported versions). Reviewed by Robert Haas. Reported by Etienne Stalmans. Security: CVE-2020-25695
* In INSERT/UPDATE, use the table's real tuple descriptor as target.Tom Lane2020-11-08
| | | | | | | | | | | | | | | | | | | | | | | This back-patches commit 20d3fe900 into the v12 and v13 branches. At the time I thought that commit was not fixing any observable bug, but Bertrand Drouvot showed otherwise: adding a dropped column to the previously-considered scenario crashes v12 and v13, unless the dropped column happens to be an integer. That is, of course, because the tupdesc we derive from the plan output tlist fails to describe the dropped column accurately, so that we'll do the wrong thing with a tuple in which that column isn't NULL. There is no bug in pre-v12 branches because they already did use the table's real tuple descriptor for any trigger-returned tuple. It seems that this set of bugs can be blamed on the changes that removed es_trig_tuple_slot, though I've not attempted to pin that down precisely. Although there's no code change needed in HEAD, update the test case to include a dropped column there too. Discussion: https://postgr.es/m/db5d97c8-f48a-51e2-7b08-b73d5434d425@amazon.com Discussion: https://postgr.es/m/16644-5da7ef98a7ac4545@postgresql.org
* Message style improvementsAlvaro Herrera2020-11-07
| | | | | | | | | | | | | * Avoid pointlessly highlighting that an index vacuum was executed by a parallel worker; user doesn't care. * Don't give the impression that a non-concurrent reindex of an invalid index on a TOAST table would work, because it wouldn't. * Add a "translator:" comment for a mysterious message. Discussion: https://postgr.es/m/20201107034943.GA16596@alvherre.pgsql Reviewed-by: Michael Paquier <michael@paquier.xyz>
* Revert "Accept relations of any kind in LOCK TABLE".Tom Lane2020-11-06
| | | | | | | | | | Revert 59ab4ac32, as well as the followup fix 33862cb9c, in all branches. We need to think a bit harder about what the behavior of LOCK TABLE on views should be, and there's no time for that before next week's releases. We'll take another crack at this later. Discussion: https://postgr.es/m/16703-e348f58aab3cf6cc@postgresql.org
* Don't throw an error for LOCK TABLE on a self-referential view.Tom Lane2020-11-05
| | | | | | | | | | | | | LOCK TABLE has complained about "infinite recursion" when applied to a self-referential view, ever since we made it recurse into views in v11. However, that breaks pg_dump's new assumption that it's okay to lock every relation. There doesn't seem to be any good reason to throw an error: if we just abandon the recursion, we've still satisfied the requirement of locking every referenced relation. Per bug #16703 from Andrew Bille (via Alexander Lakhin). Discussion: https://postgr.es/m/16703-e348f58aab3cf6cc@postgresql.org
* Allow users with BYPASSRLS to alter their own passwords.Tom Lane2020-11-03
| | | | | | | | | | | | | | | | The intention in commit 491c029db was to require superuserness to change the BYPASSRLS property, but the actual effect of the coding in AlterRole() was to require superuserness to change anything at all about a BYPASSRLS role. Other properties of a BYPASSRLS role should be changeable under the same rules as for a normal role, though. Fix that, and also take care of some documentation omissions related to BYPASSRLS and REPLICATION role properties. Tom Lane and Stephen Frost, per bug report from Wolfgang Walther. Back-patch to all supported branches. Discussion: https://postgr.es/m/a5548a9f-89ee-3167-129d-162b5985fcf8@technowledgy.de
* Disallow ALTER TABLE ONLY / DROP EXPRESSIONPeter Eisentraut2020-11-03
| | | | | | | | | | | | | | | The current implementation cannot handle this correctly, so just forbid it for now. GENERATED clauses must be attached to the column definition and cannot be added later like DEFAULT, so if a child table has a generation expression that the parent does not have, the child column will necessarily be an attlocal column. So to implement ALTER TABLE ONLY / DROP EXPRESSION, we'd need extra code to update attislocal of the direct child tables, somewhat similar to how DROP COLUMN does it, so that the resulting state can be properly dumped and restored. Discussion: https://www.postgresql.org/message-id/flat/15830.1575468847%40sss.pgh.pa.us
* Accept relations of any kind in LOCK TABLEAlvaro Herrera2020-10-27
| | | | | | | | | | | | | | | The restriction that only tables and views can be locked by LOCK TABLE is quite arbitrary, since the underlying mechanism can lock any relation type. Drop the restriction so that programs such as pg_dump can lock all relations they're interested in, preventing schema changes that could cause a dump to fail after expending much effort. Backpatch to 9.5. Author: Álvaro Herrera <alvherre@alvh.no-ip.org> Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us> Reported-by: Wells Oliver <wells.oliver@gmail.com> Discussion: https://postgr.es/m/20201021200659.GA32358@alvherre.pgsql
* Fix corner case for a BEFORE ROW UPDATE trigger returning OLD.Tom Lane2020-10-25
| | | | | | | | | | | | | | | | | | | | | | | | | If the old row has any "missing" attributes that are supposed to be retrieved from an associated tuple descriptor, the wrong things happened because the trigger result is shoved directly into an executor slot that lacks the missing-attribute data. Notably, CHECK-constraint verification would incorrectly see those columns as NULL, and so would RETURNING-list evaluation. Band-aid around this by forcibly expanding the tuple before passing it to the trigger function. (IMO it was a fundamental misdesign to put the missing-attribute data into tuple constraints, which so much of the system considers to be optional. But we're probably stuck with that now, and will have to continue to apply band-aids as we find other places with similar issues.) Back-patch to v12. v11 would also have the issue, except that commit 920311ab1 already applied a similar band-aid. That forced expansion in more cases than seem really necessary, though, so this isn't a directly equivalent fix. Amit Langote, with some cosmetic changes by me Discussion: https://postgr.es/m/16644-5da7ef98a7ac4545@postgresql.org