Re: A failure in prepared_xacts test
Michael Paquier writes: > I don't disagree with your point, still I'm not sure that this can be > made entirely bullet-proof. Anyway, I think that we should still > improve this test and make it more robust for parallel operations: > installcheck fails equally on HEAD if there is a prepared transaction > on the backend where the tests run, and that seems like a bad idea to > me to rely on cluster-wide scans for what should be a "local" test. True, it's antithetical to the point of an "installcheck" test if unrelated actions in another database can break it. So I'm fine with tightening up prepared_xacts's query. I just wonder how far we want to try to carry this. (BTW, on the same logic, should ecpg's twophase.pgc be using a prepared-transaction name that's less generic than "gxid"?) regards, tom lane
Re: A failure in prepared_xacts test
Hello Tom and Michael, 29.04.2024 08:11, Tom Lane wrote: Michael Paquier writes: If you grep the source tree, you'd notice that a prepared transaction named gxid only exists in the 2PC tests of ECPG, in src/interfaces/ecpg/test/sql/twophase.pgc. So the origin of the failure comes from a race condition due to test parallelization, because the scan of pg_prepared_xacts affects all databases with installcheck, and in your case it means that the scan of pg_prepared_xacts was running in parallel of the ECPG tests with an installcheck. Up to now, we've only worried about whether tests running in parallel within a single test suite can interact. It's quite scary to think that the meson setup has expanded the possibility of interactions to our entire source tree. Maybe that was a bad idea and we should fix the meson infrastructure to not do that. I fear that otherwise, we'll get bit regularly by very-low-probability bugs of this kind. Yes, I'm afraid of the same. For example, the test failure [1] is of that ilk, I guess. [1] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=rorqual=2024-04-17%2016%3A33%3A23 Best regards, Alexander
RE: Synchronizing slots from primary to standby
On Friday, March 15, 2024 10:45 PM Bertrand Drouvot wrote: > > Hi, > > On Thu, Mar 14, 2024 at 02:22:44AM +, Zhijie Hou (Fujitsu) wrote: > > Hi, > > > > Since the standby_slot_names patch has been committed, I am attaching > > the last doc patch for review. > > > > Thanks! > > 1 === > > + continue subscribing to publications now on the new primary server > without > + any data loss. > > I think "without any data loss" should be re-worded in this context. Data > loss in > the sense "data committed on the primary and not visible on the subscriber in > case of failover" can still occurs (in case synchronous replication is not > used). > > 2 === > > + If the result (failover_ready) of both above steps is > + true, existing subscriptions will be able to continue without data loss. > + > > I don't think that's true if synchronous replication is not used. Say, > > - synchronous replication is not used > - primary is not able to reach the standby anymore and standby_slot_names is > set > - new data is inserted into the primary > - then not replicated to subscriber (due to standby_slot_names) > > Then I think the both above steps will return true but data would be lost in > case > of failover. Thanks for the comments, attach the new version patch which reworded the above places. Best Regards, Hou zj v2-0001-Document-the-steps-to-check-if-the-standby-is-rea.patch Description: v2-0001-Document-the-steps-to-check-if-the-standby-is-rea.patch
Re: A failure in prepared_xacts test
On Mon, Apr 29, 2024 at 01:11:00AM -0400, Tom Lane wrote: > Up to now, we've only worried about whether tests running in parallel > within a single test suite can interact. It's quite scary to think > that the meson setup has expanded the possibility of interactions > to our entire source tree. Maybe that was a bad idea and we should > fix the meson infrastructure to not do that. I fear that otherwise, > we'll get bit regularly by very-low-probability bugs of this kind. I don't disagree with your point, still I'm not sure that this can be made entirely bullet-proof. Anyway, I think that we should still improve this test and make it more robust for parallel operations: installcheck fails equally on HEAD if there is a prepared transaction on the backend where the tests run, and that seems like a bad idea to me to rely on cluster-wide scans for what should be a "local" test. -- Michael signature.asc Description: PGP signature
Re: documentation structure
> > I've splitted it to7 patches. > each patch split one into separate new files. > Seems like a good start. Looking at the diffs of these, I wonder if we would be better off with a func/ directory, each function gets its own file in that dir, and either these files above include the individual files, or the original func.sgml just becomes the organizer of all the functions. That would allow us to do future reorganizations with minimal churn, make validation of this patch a bit more straightforward, and make it easier for future editors to find the function they need to edit.
Re: A failure in prepared_xacts test
Michael Paquier writes: > If you grep the source tree, you'd notice that a prepared transaction > named gxid only exists in the 2PC tests of ECPG, in > src/interfaces/ecpg/test/sql/twophase.pgc. So the origin of the > failure comes from a race condition due to test parallelization, > because the scan of pg_prepared_xacts affects all databases with > installcheck, and in your case it means that the scan of > pg_prepared_xacts was running in parallel of the ECPG tests with an > installcheck. Up to now, we've only worried about whether tests running in parallel within a single test suite can interact. It's quite scary to think that the meson setup has expanded the possibility of interactions to our entire source tree. Maybe that was a bad idea and we should fix the meson infrastructure to not do that. I fear that otherwise, we'll get bit regularly by very-low-probability bugs of this kind. regards, tom lane
Re: pg_input_error_info doc 2 exampled crammed together
On Sunday, April 28, 2024, Michael Paquier wrote: > On Sun, Apr 28, 2024 at 06:45:30PM -0700, David G. Johnston wrote: > > My preference would be to limit this section to a single example. The > > numeric one, as it provides values for more output columns. I would > change > > the output format to expanded from default, in order to show all columns > > and not overrun the length of a single line. > > Agreed that having two examples does not bring much, so this could be > brought to a single one. The first one is enough to show the point of > the function, IMO. It is shorter in width and it shows all the output > columns. > > Agreed. The column names are self-explanatory if you’ve seen errors before. The values are immaterial. Plus we don’t generally use psql-specific features in our examples. David J.
Re: A failure in prepared_xacts test
Hello Richard, 29.04.2024 04:12, Richard Guo wrote: Does anyone have any clue to this failure? FWIW, after another run of this test, the failure just disappears. Does it suggest that the test case is flaky? I think this could be caused by the ecpg test twophase executed simultaneously with the test prepared_xacts thanks to meson's jobs parallelization. Best regards, Alexander
Re: A failure in prepared_xacts test
On Mon, Apr 29, 2024 at 09:12:48AM +0800, Richard Guo wrote:
> Does anyone have any clue to this failure?
>
> FWIW, after another run of this test, the failure just disappears. Does
> it suggest that the test case is flaky?
If you grep the source tree, you'd notice that a prepared transaction
named gxid only exists in the 2PC tests of ECPG, in
src/interfaces/ecpg/test/sql/twophase.pgc. So the origin of the
failure comes from a race condition due to test parallelization,
because the scan of pg_prepared_xacts affects all databases with
installcheck, and in your case it means that the scan of
pg_prepared_xacts was running in parallel of the ECPG tests with an
installcheck.
The only location in the whole tree where we want to do predictible
scans of pg_prepared_xacts is prepared_xacts.sql, so rather than
playing with 2PC transactions across a bunch of tests, I think that we
should do two things, both touching prepared_xacts.sql:
- The 2PC transactions run in the main regression test suite should
use names that would be unlikely used elsewhere.
- Limit the scans of pg_prepared_xacts on these name patterns to avoid
interferences.
See for example the attached with both expected outputs updated
depending on the value set for max_prepared_transactions in the
backend. There may be an argument in back-patching that, but I don't
recall seeing this failure in the CI, so perhaps that's not worth
bothering with. What do you think?
--
Michael
diff --git a/src/test/regress/expected/prepared_xacts.out b/src/test/regress/expected/prepared_xacts.out
index ba8e3ccc6c..515a2ada9d 100644
--- a/src/test/regress/expected/prepared_xacts.out
+++ b/src/test/regress/expected/prepared_xacts.out
@@ -17,7 +17,7 @@ SELECT * FROM pxtest1;
bbb
(1 row)
-PREPARE TRANSACTION 'foo1';
+PREPARE TRANSACTION 'regress_foo1';
SELECT * FROM pxtest1;
foobar
@@ -25,21 +25,21 @@ SELECT * FROM pxtest1;
(1 row)
-- Test pg_prepared_xacts system view
-SELECT gid FROM pg_prepared_xacts;
- gid
---
- foo1
+SELECT gid FROM pg_prepared_xacts WHERE gid ~ '^regress_' ORDER BY gid;
+ gid
+--
+ regress_foo1
(1 row)
-- Test ROLLBACK PREPARED
-ROLLBACK PREPARED 'foo1';
+ROLLBACK PREPARED 'regress_foo1';
SELECT * FROM pxtest1;
foobar
aaa
(1 row)
-SELECT gid FROM pg_prepared_xacts;
+SELECT gid FROM pg_prepared_xacts WHERE gid ~ '^regress_' ORDER BY gid;
gid
-
(0 rows)
@@ -54,14 +54,14 @@ SELECT * FROM pxtest1;
ddd
(2 rows)
-PREPARE TRANSACTION 'foo2';
+PREPARE TRANSACTION 'regress_foo2';
SELECT * FROM pxtest1;
foobar
aaa
(1 row)
-COMMIT PREPARED 'foo2';
+COMMIT PREPARED 'regress_foo2';
SELECT * FROM pxtest1;
foobar
@@ -79,18 +79,18 @@ SELECT * FROM pxtest1;
eee
(2 rows)
-PREPARE TRANSACTION 'foo3';
-SELECT gid FROM pg_prepared_xacts;
- gid
---
- foo3
+PREPARE TRANSACTION 'regress_foo3';
+SELECT gid FROM pg_prepared_xacts WHERE gid ~ '^regress_' ORDER BY gid;
+ gid
+--
+ regress_foo3
(1 row)
BEGIN TRANSACTION ISOLATION LEVEL SERIALIZABLE;
INSERT INTO pxtest1 VALUES ('fff');
-- This should fail, because the gid foo3 is already in use
-PREPARE TRANSACTION 'foo3';
-ERROR: transaction identifier "foo3" is already in use
+PREPARE TRANSACTION 'regress_foo3';
+ERROR: transaction identifier "regress_foo3" is already in use
SELECT * FROM pxtest1;
foobar
@@ -98,7 +98,7 @@ SELECT * FROM pxtest1;
ddd
(2 rows)
-ROLLBACK PREPARED 'foo3';
+ROLLBACK PREPARED 'regress_foo3';
SELECT * FROM pxtest1;
foobar
@@ -116,11 +116,11 @@ SELECT * FROM pxtest1;
eee
(2 rows)
-PREPARE TRANSACTION 'foo4';
-SELECT gid FROM pg_prepared_xacts;
- gid
---
- foo4
+PREPARE TRANSACTION 'regress_foo4';
+SELECT gid FROM pg_prepared_xacts WHERE gid ~ '^regress_' ORDER BY gid;
+ gid
+--
+ regress_foo4
(1 row)
BEGIN TRANSACTION ISOLATION LEVEL SERIALIZABLE;
@@ -136,15 +136,15 @@ INSERT INTO pxtest1 VALUES ('fff');
ERROR: could not serialize access due to read/write dependencies among transactions
DETAIL: Reason code: Canceled on identification as a pivot, during write.
HINT: The transaction might succeed if retried.
-PREPARE TRANSACTION 'foo5';
-SELECT gid FROM pg_prepared_xacts;
- gid
---
- foo4
+PREPARE TRANSACTION 'regress_foo5';
+SELECT gid FROM pg_prepared_xacts WHERE gid ~ '^regress_' ORDER BY gid;
+ gid
+--
+ regress_foo4
(1 row)
-ROLLBACK PREPARED 'foo4';
-SELECT gid FROM pg_prepared_xacts;
+ROLLBACK PREPARED 'regress_foo4';
+SELECT gid FROM pg_prepared_xacts WHERE gid ~ '^regress_' ORDER BY gid;
gid
-
(0 rows)
@@ -165,7 +165,7 @@ SELECT pg_advisory_xact_lock_shared(1);
(1 row)
-PREPARE TRANSACTION 'foo6'; -- fails
+PREPARE TRANSACTION 'regress_foo6'; -- fails
ERROR: cannot PREPARE while holding both session-level and transaction-level locks on the same object
-- Test subtransactions
BEGIN TRANSACTION ISOLATION
Re: TerminateOtherDBBackends code comments inconsistency.
On Mon, Apr 22, 2024 at 9:56 PM Noah Misch wrote: > > On Mon, Apr 15, 2024 at 11:17:54AM +0530, Amit Kapila wrote: > > On Thu, Apr 11, 2024 at 6:58 PM Kirill Reshke > > wrote: > > > > > > While working on [0] i have noticed this comment in > > > TerminateOtherDBBackends function: > > > > > > /* > > > * Check whether we have the necessary rights to terminate other > > > * sessions. We don't terminate any session until we ensure that we > > > * have rights on all the sessions to be terminated. These checks are > > > * the same as we do in pg_terminate_backend. > > > * > > > * In this case we don't raise some warnings - like "PID %d is not a > > > * PostgreSQL server process", because for us already finished session > > > * is not a problem. > > > */ > > > > > > This statement is not true after 3a9b18b. > > > "These checks are the same as we do in pg_terminate_backend." > > The comment mismatch is a problem. Thanks for reporting it. The DROP > DATABASE doc mimics the comment, using, "permissions are the same as with > pg_terminate_backend". > How about updating the comments as in the attached? I see that commit 3a9b18b309 didn't change the docs of pg_terminate_backend and whatever is mentioned w.r.t permissions in the doc of that function sounds valid for drop database force to me. Do you have any specific proposal in your mind? > > > But the code is still correct, I assume... or not? In fact, we are > > > killing autovacuum workers which are working with a given database > > > (proc->roleId == 0), which is OK in that case. Are there any other > > > cases when proc->roleId == 0 but we should not be able to kill such a > > > process? > > > > > > > Good question. I am not aware of such cases but I wonder if we should > > add a check similar to 3a9b18b [1] for the reason given in the commit > > message. I have added Noah to see if he has any suggestions on this > > matter. > > > > [1] - > > commit 3a9b18b3095366cd0c4305441d426d04572d88c1 > > Author: Noah Misch > > Date: Mon Nov 6 06:14:13 2023 -0800 > > > > Ban role pg_signal_backend from more superuser backend types. > > That commit distinguished several scenarios. Here's how they apply in > TerminateOtherDBBackends(): > > - logical replication launcher, autovacuum launcher: the proc->databaseId test > already skips them, since they don't connect to a database. Vignesh said > this. > > - autovacuum worker: should terminate, since CountOtherDBBackends() would > terminate them in DROP DATABASE without FORCE. > > - background workers that connect to a database: the right thing is less clear > on these, but I lean toward allowing termination and changing the DROP > DATABASE doc. As a bgworker author, I would value being able to recommend > DROP DATABASE FORCE if a worker is sticking around unexpectedly. There's > relatively little chance of a bgworker actually wanting to block DROP > DATABASE FORCE or having an exploitable termination-time bug. > Agreed with this analysis and I don't see the need for the code change. -- With Regards, Amit Kapila. fix_comments_1.patch Description: Binary data
Re: pg_input_error_info doc 2 exampled crammed together
On Sun, Apr 28, 2024 at 06:45:30PM -0700, David G. Johnston wrote: > My preference would be to limit this section to a single example. The > numeric one, as it provides values for more output columns. I would change > the output format to expanded from default, in order to show all columns > and not overrun the length of a single line. Agreed that having two examples does not bring much, so this could be brought to a single one. The first one is enough to show the point of the function, IMO. It is shorter in width and it shows all the output columns. -- Michael signature.asc Description: PGP signature
Re: Introduce XID age and inactive timeout based replication slot invalidation
On Thu, Apr 25, 2024 at 11:11 AM Bharath Rupireddy wrote: > > On Mon, Apr 22, 2024 at 7:21 PM Masahiko Sawada wrote: > > > > > Please find the attached v35 patch. > > > > The documentation says about both 'active' and 'inactive_since' > > columns of pg_replication_slots say: > > > > --- > > active bool > > True if this slot is currently actively being used > > > > inactive_since timestamptz > > The time since the slot has become inactive. NULL if the slot is > > currently being used. Note that for slots on the standby that are > > being synced from a primary server (whose synced field is true), the > > inactive_since indicates the last synchronization (see Section 47.2.3) > > time. > > --- > > > > When reading the description I thought if 'active' is true, > > 'inactive_since' is NULL, but it doesn't seem to apply for temporary > > slots. > > Right. > > > Since we don't reset the active_pid field of temporary slots > > when the release, the 'active' is still true in the view but > > 'inactive_since' is not NULL. > > Right. inactive_since is reset whenever the temporary slot is acquired > again within the same backend that created the temporary slot. > > > Do you think we need to mention it in > > the documentation? > > I think that's the reason we dropped "active" from the statement. It > was earlier "NULL if the slot is currently actively being used.". But, > per Bertrand's comment > https://www.postgresql.org/message-id/ZehE2IJcsetSJMHC%40ip-10-97-1-34.eu-west-3.compute.internal > changed it to ""NULL if the slot is currently being used.". > > Temporary slots retain the active = true and active_pid = backend that created it> even when the slot is not being used until > the lifetime of the backend process. We haven't tied active or > active_pid flags to inactive_since, doing so now to represent the > temporary slot behaviour for active and active_pid will confuse users > more. > This is true and it's probably easy for us to understand as we developed this feature but the same may not be true for others. I wonder if we can be explicit about the difference of active/inactive_since by adding something like the following for inactive_since: Note that this field is not related to the active flag as temporary slots can remain active till the session ends even when they are not being used. Sawada-San, do you have any suggestions on the wording? > As far as the inactive_since of a slot is concerned, it is set > to 0 when the slot is being used (acquired) and set to current > timestamp when the slot is not being used (released). > > > As for the timeout-based slot invalidation feature, we could end up > > invalidating the temporary slots even if they are shown as active, > > which could confuse users. Do we want to somehow deal with it? > > Yes. As long as the temporary slot is lying unused holding up > resources for more than the specified > replication_slot_inactive_timeout, it is bound to get invalidated. > This keeps behaviour consistent and less-confusing to the users. > Agreed. We may want to add something in the docs for this to avoid confusion with the active flag. -- With Regards, Amit Kapila.
Re: Background Processes in Postgres Extension
On Sat, Apr 27, 2024 at 9:26 PM Sushrut Shivaswamy < sushrut.shivasw...@gmail.com> wrote: > Thanks for the suggestion on using postgres background worker. > > I tried creating one following the implementation in worker_spi and am > able to spawn a background worker successfully. > > However, the background worker seems to cause postmaster to crash when I > wait for it to finish using `WaitForBackgroundWorkerShutdown. > The function called by the background worker is empty except for logging a > message to disk. > When debugging a crash, backtrace from core dump is useful. if you can reproduce the crash, running reproduction with gdb attached to a process intended to crash is more useful. -- Best Wishes, Ashutosh Bapat
Re: New committers: Melanie Plageman, Richard Guo
On Fri, 26 Apr 2024 at 17:24, Jonathan S. Katz wrote: > > The Core Team would like to extend our congratulations to Melanie > Plageman and Richard Guo, who have accepted invitations to become our > newest PostgreSQL committers. > > Please join us in wishing them much success and few reverts! Congratulations to both of you. Regards, Vignesh
Re: Race condition in FetchTableStates() breaks synchronization of subscription tables
On Thu, 25 Apr 2024 at 16:20, Amit Kapila wrote: > > On Thu, Apr 25, 2024 at 7:01 AM Zhijie Hou (Fujitsu) > wrote: > > > > On Wednesday, April 24, 2024 6:29 PM vignesh C wrote: > > > > > > > > > The attached patch > > > v7-0001-Table-sync-missed-due-to-race-condition-in-subscr.patch > > > applies for master and PG16 branch, > > > v7-0001-Table-sync-missed-due-to-race-condition-in-subscr_PG15.patch > > > applies for PG15 branch. > > > > Thanks, I have verified that the patches can be applied cleanly and fix the > > issue on each branch. The regression test can also pass after applying the > > patch > > on my machine. > > > > Pushed. Thanks for pushing this, I have marked the commitfest entry at [1] as committed. [1] - https://commitfest.postgresql.org/48/4816/ Regards, Vignesh
Re: POC: GROUP BY optimization
On Wed, Apr 24, 2024 at 2:25 PM jian he wrote: > > hi. > I found an interesting case. > > CREATE TABLE t1 AS > SELECT (i % 10)::numeric AS x,(i % 10)::int8 AS y,'abc' || i % 10 AS > z, i::int4 AS w > FROM generate_series(1, 100) AS i; > CREATE INDEX t1_x_y_idx ON t1 (x, y); > ANALYZE t1; > SET enable_hashagg = off; > SET enable_seqscan = off; > > EXPLAIN (COSTS OFF) SELECT count(*) FROM t1 GROUP BY x,z,y,w; > EXPLAIN (COSTS OFF) SELECT count(*) FROM t1 GROUP BY x,w,y,z; > EXPLAIN (COSTS OFF) SELECT count(*) FROM t1 GROUP BY x,z,w,y; > EXPLAIN (COSTS OFF) SELECT count(*) FROM t1 GROUP BY x,w,z,y; > the above part will use: > -> Incremental Sort > Sort Key: x, $, $, $ > Presorted Key: x > -> Index Scan using t1_x_y_idx on t1 We can make these cases also `Presorted Key: x, y`. in `if (path->pathkeys && !pathkeys_contained_in(path->pathkeys, root->group_pathkeys))` branch we can simple do - infos = lappend(infos, info); + infos = lcons(info, infos); similar to what we did at plancat.c (search lcons). get_useful_group_keys_orderings returns a list of PathKeyInfo, then the caller function just iterates each element. so for the caller, order of the returned list element from get_useful_group_keys_orderings does not matter. for path Incremental Sort: function make_ordered_path will return the same cost for different numbers of presorted keys. for example: EXPLAIN (COSTS OFF) SELECT count(*) FROM t1 GROUP BY x,z,y,w; make_ordered_path cost is same for: `info->pathkeys: x,y,z,w` `info->pathkeys:x,z,y,w` if we arrange `info->pathkeys: x,y,z,w` before `info->pathkeys:x,z,y,w` in get_useful_group_keys_orderings. then with the same cost, we will choose the first one (`info->pathkeys: x,y,z,w`), if we use IncrementalSort, then we use `Presorted Key: x, y`.
Re: pg_input_error_info doc 2 exampled crammed together
On Sunday, April 28, 2024, jian he wrote: > > > after checking the definition of [1], [2], > maybe here we should use > Possibly, though I’d be curious to see how consistent we are on this point elsewhere before making a point of it. > > and also add `(1 row)` information. Doesn’t seem like added value. > > or we can simply add a empty new line between > ` value "420" is out of range for type integer || | > 22003` > and > `` My preference would be to limit this section to a single example. The numeric one, as it provides values for more output columns. I would change the output format to expanded from default, in order to show all columns and not overrun the length of a single line. David J.
Re: Support "Right Semi Join" plan shapes
Hi Richard Thank you so much for your tireless work on this,I see the new version of the patch improves some of the comments .I think it can commit in July Thanks On Thu, 25 Apr 2024 at 11:28, Richard Guo wrote: > Here is another rebase with a commit message to help review. I also > tweaked some comments. > > Thanks > Richard >
pg_input_error_info doc 2 exampled crammed together
hi.
select * from pg_input_error_info('420', 'integer')
select message, detail from pg_input_error_info('1234.567', 'numeric(7,4)')
I found above two examples at [0] crammed together.
select * from pg_input_error_info('420',
'integer')
message| detail | hint
| sql_error_code
--++--+
value "420" is out of range for type integer || | 22003
select message, detail from
pg_input_error_info('1234.567', 'numeric(7,4)')
message | detail
+---
numeric field overflow | A field with precision 7, scale 4 must round
to an absolute value less than 10^3.
after checking the definition of [1], [2],
maybe here we should use and also add `(1 row)` information.
or we can simply add a empty new line between
` value "420" is out of range for type integer || | 22003`
and
``
[0]
https://www.postgresql.org/docs/current/functions-info.html#FUNCTIONS-INFO-VALIDITY
[1] https://tdg.docbook.org/tdg/4.5/programlisting
[2] https://tdg.docbook.org/tdg/4.5/screen
--
I recommend David Deutsch's <>
Jian
A failure in prepared_xacts test
Yesterday I noticed a failure on cirrus-ci for the 'Right Semi Join' patch. The failure can be found at [1], and it looks like: --- /tmp/cirrus-ci-build/src/test/regress/expected/prepared_xacts.out 2024-04-27 00:41:25.831297000 + +++ /tmp/cirrus-ci-build/build/testrun/regress-running/regress/results/prepared_xacts.out 2024-04-27 00:45:50.261369000 + @@ -83,8 +83,9 @@ SELECT gid FROM pg_prepared_xacts; gid -- + gxid foo3 -(1 row) +(2 rows) Upon closer look, it seems that this issue is not caused by the patch about 'Right Semi Join', because this query, which initially included two left joins, can actually be reduced to a function scan after removing these two useless left joins. It seems that no semi-joins would be involved. EXPLAIN SELECT gid FROM pg_prepared_xacts; QUERY PLAN Function Scan on pg_prepared_xact p (cost=0.00..10.00 rows=1000 width=32) (1 row) Does anyone have any clue to this failure? FWIW, after another run of this test, the failure just disappears. Does it suggest that the test case is flaky? [1] https://api.cirrus-ci.com/v1/artifact/task/6220592364388352/testrun/build/testrun/regress-running/regress/regression.diffs Thanks Richard
Re: Tarball builds in the new world order
Alvaro Herrera writes: > Why is it that the .gitrevision file is only created here, instead of > being added to the tarball that "git archive" produces? Adding an > argument like > --add-virtual-file $(distdir)/.gitrevision:$(GIT_REFSPEC) > to the git archive call should suffice. I think we don't want to do that. In the first place, it's redundant because "git archive" includes the commit hash in the tar header, and in the second place it gets away from the concept that the tarball contains exactly what is in our git tree. Now admittedly, if anyone's built tooling that relies on the presence of the .gitrevision file, they might prefer that we keep on including it. But I'm not sure anyone has, and in any case I think switching to the git-approved way of incorporating the hash is the best thing in the long run. What I'm thinking of doing, as soon as we've sorted the tarball creation process, is to make a test tarball available to the packagers group so that anyone interested can start working on updating their packaging process for the new approach. Hopefully, if anyone's especially unhappy about omitting .gitrevision, they'll speak up. regards, tom lane
Re: Refactoring backend fork+exec code
On 27/04/2024 11:27, Anton A. Melnikov wrote: Hello! Maybe add PGDLLIMPORT to extern bool LoadedSSL; and extern struct ClientSocket *MyClientSocket; definitions in the src/include/postmaster/postmaster.h ? Peter E noticed and Michael fixed them in commit 768ceeeaa1 already. -- Heikki Linnakangas Neon (https://neon.tech)
Re: Tarball builds in the new world order
On 2024-Apr-26, Tom Lane wrote:
> --- mk-one-release.orig 2024-04-23 17:30:08.983226671 -0400
> +++ mk-one-release2024-04-26 15:17:29.713669677 -0400
> @@ -39,13 +39,17 @@ mkdir pgsql
> git archive ${gitref} | tar xf - -C pgsql
>
> # Include the git ref in the output tarballs
> +# (This has no effect with v17 and up; instead we rely on "git archive"
> +# to include the commit hash in the tar header)
> echo ${gitref} >pgsql/.gitrevision
Why is it that the .gitrevision file is only created here, instead of
being added to the tarball that "git archive" produces? Adding an
argument like
--add-virtual-file $(distdir)/.gitrevision:$(GIT_REFSPEC)
to the git archive call should suffice.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"I can't go to a restaurant and order food because I keep looking at the
fonts on the menu. Five minutes later I realize that it's also talking
about food" (Donald Knuth)
Re: DROP OWNED BY fails to clean out pg_init_privs grants
I wrote:
> Here's a draft patch that attacks that. It seems to fix the
> problem with test_pg_dump: no dangling pg_init_privs grants
> are left behind.
Here's a v2 that attempts to add some queries to test_pg_dump.sql
to provide visual verification that pg_shdepend and pg_init_privs
are updated correctly during DROP OWNED BY. It's a little bit
nasty to look at the ACL column of pg_init_privs, because that text
involves the bootstrap superuser's name which is site-dependent.
What I did to try to make the test stable is
replace(initprivs::text, current_user, 'postgres') AS initprivs
This is of course not bulletproof: with a sufficiently weird
bootstrap superuser name, we could get false matches to parts
of "regress_dump_test_role" or to privilege strings. That
seems unlikely enough to live with, but I wonder if anybody has
a better idea.
regards, tom lane
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 2907079e2a..c8cb46c5b9 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -7184,6 +7184,20 @@ SCRAM-SHA-256$iteration count:
+
+ SHARED_DEPENDENCY_INITACL (i)
+
+
+ The referenced object (which must be a role) is mentioned in a
+ pg_init_privs
+ entry for the dependent object.
+ (A SHARED_DEPENDENCY_INITACL entry is not made for
+ the owner of the object, since the owner will have
+ a SHARED_DEPENDENCY_OWNER entry anyway.)
+
+
+
+
SHARED_DEPENDENCY_POLICY (r)
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index 7abf3c2a74..04c41c0c14 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -165,9 +165,9 @@ static AclMode pg_type_aclmask_ext(Oid type_oid, Oid roleid,
AclMode mask, AclMaskHow how,
bool *is_missing);
static void recordExtensionInitPriv(Oid objoid, Oid classoid, int objsubid,
- Acl *new_acl);
+ Oid ownerId, Acl *new_acl);
static void recordExtensionInitPrivWorker(Oid objoid, Oid classoid, int objsubid,
- Acl *new_acl);
+ Oid ownerId, Acl *new_acl);
/*
@@ -1790,7 +1790,7 @@ ExecGrant_Attribute(InternalGrant *istmt, Oid relOid, const char *relname,
CatalogTupleUpdate(attRelation, >t_self, newtuple);
/* Update initial privileges for extensions */
- recordExtensionInitPriv(relOid, RelationRelationId, attnum,
+ recordExtensionInitPriv(relOid, RelationRelationId, attnum, ownerId,
ACL_NUM(new_acl) > 0 ? new_acl : NULL);
/* Update the shared dependency ACL info */
@@ -2050,7 +2050,8 @@ ExecGrant_Relation(InternalGrant *istmt)
CatalogTupleUpdate(relation, >t_self, newtuple);
/* Update initial privileges for extensions */
- recordExtensionInitPriv(relOid, RelationRelationId, 0, new_acl);
+ recordExtensionInitPriv(relOid, RelationRelationId, 0,
+ ownerId, new_acl);
/* Update the shared dependency ACL info */
updateAclDependencies(RelationRelationId, relOid, 0,
@@ -2251,7 +2252,7 @@ ExecGrant_common(InternalGrant *istmt, Oid classid, AclMode default_privs,
CatalogTupleUpdate(relation, >t_self, newtuple);
/* Update initial privileges for extensions */
- recordExtensionInitPriv(objectid, classid, 0, new_acl);
+ recordExtensionInitPriv(objectid, classid, 0, ownerId, new_acl);
/* Update the shared dependency ACL info */
updateAclDependencies(classid,
@@ -2403,7 +2404,8 @@ ExecGrant_Largeobject(InternalGrant *istmt)
CatalogTupleUpdate(relation, >t_self, newtuple);
/* Update initial privileges for extensions */
- recordExtensionInitPriv(loid, LargeObjectRelationId, 0, new_acl);
+ recordExtensionInitPriv(loid, LargeObjectRelationId, 0,
+ownerId, new_acl);
/* Update the shared dependency ACL info */
updateAclDependencies(LargeObjectRelationId,
@@ -2575,7 +2577,7 @@ ExecGrant_Parameter(InternalGrant *istmt)
/* Update initial privileges for extensions */
recordExtensionInitPriv(parameterId, ParameterAclRelationId, 0,
-new_acl);
+ownerId, new_acl);
/* Update the shared dependency ACL info */
updateAclDependencies(ParameterAclRelationId, parameterId, 0,
@@ -4463,6 +4465,7 @@ recordExtObjInitPriv(Oid objoid, Oid classoid)
}
recordExtensionInitPrivWorker(objoid, classoid, curr_att,
+ pg_class_tuple->relowner,
DatumGetAclP(attaclDatum));
ReleaseSysCache(attTuple);
@@ -4475,6 +4478,7 @@ recordExtObjInitPriv(Oid objoid, Oid classoid)
/* Add the record, if any, for the top-level object */
if (!isNull)
recordExtensionInitPrivWorker(objoid, classoid, 0,
+ pg_class_tuple->relowner,
DatumGetAclP(aclDatum));
ReleaseSysCache(tuple);
@@ -4485,6 +4489,7 @@ recordExtObjInitPriv(Oid objoid, Oid classoid)
Datum aclDatum;
bool isNull;
HeapTuple tuple;
+ Form_pg_largeobject_metadata form_lo_meta;
ScanKeyData
Re: Fix overflow hazard in timestamp_pl_interval
Joseph Koshakow writes: >> Attached is a patch that fixes some overflow/underflow hazards in >> `timestamp_pl_interval`. The microseconds overflow could produce >> incorrect result. The month overflow would generally still result in an >> error from the timestamp month field being too low, but it's still >> better to catch it early. Yeah. I had earlier concluded that we couldn't overflow here without triggering the range checks in tm2timestamp, but clearly that was too optimistic. >> I couldn't find any existing timestamp plus interval tests so I stuck >> a new tests in `timestamp.sql`. If there's a better place, then >> please let me know. They're in horology.sql, so I moved the tests there and pushed it. Thanks for the report! regards, tom lane
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
On Sun, Apr 28, 2024 at 08:18:42AM -0500, Justin Pryzby wrote: > > I will explore this. Do we copy extended stats when we do CREATE > > TABLE ... PARTITION OF? I think we need to do the same here. > > Right, they're not copied because an extended stats objs on the parent > does something different than putting stats objects on each child. > I've convinced myself that it's wrong to copy the parent's stats obj. > If someone wants stats objects on each child, they'll have to handle > them specially after MERGE/SPLIT, just as they would for per-child > defaults/constraints/etc. I dug up this thread, in which the idea of copying extended stats from parent to child was considered some 6 years ago, but never implemented; for consistency, MERGE/SPLIT shouldn't copy extended stats, either. https://www.postgresql.org/message-id/20180305195750.aecbpihhcvuskzba%40alvherre.pgsql -- Justin
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
On Sunday, April 28, 2024, Alexander Lakhin wrote: > > When we deal with mixed ownership, say, bob is an owner of a > partitioned table, but not an owner of a partition, should we > allow him to perform merge with that partition? > > Attaching via alter table requires the user to own both the partitioned table and the table being acted upon. Merge needs to behave similarly. The fact that we let the superuser break the requirement of common ownership is unfortunate but I guess understandable. But given the existing behavior of attach merge should likewise fail if it find the user doesn’t own the partitions being merged. The fact that the user can select from those tables can be acted upon manually if desired; these administrative commands should all ensure common ownership and fail if that precondition is not met. David J.
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
On Sunday, April 28, 2024, Alexander Lakhin wrote: > > When we deal with mixed ownership, say, bob is an owner of a > partitioned table, but not an owner of a partition, should we > allow him to perform merge with that partition? > > IIUC Merge causes the source tables to be dropped, their data having been effectively moved into the new partition. bob must not be allowed to drop Alice’s tables. Only an owner may do that. So if we do allow bob to build a new partition using his select access, the tables he selected from would have to remain behind if he is not an owner of them. David J.
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
On Sun, Apr 28, 2024 at 04:04:54AM +0300, Alexander Korotkov wrote: > Hi Justin, > > Thank you for your review. Please check v9 of the patchset [1]. > > On Wed, Apr 24, 2024 at 11:26 PM Justin Pryzby wrote: > > This patch also/already fixes the schema issue I reported. Thanks. > > > > If you wanted to include a test case for that: > > > > begin; > > CREATE SCHEMA s; > > CREATE SCHEMA t; > > CREATE TABLE p(i int) PARTITION BY RANGE(i); > > CREATE TABLE s.c1 PARTITION OF p FOR VALUES FROM (1)TO(2); > > CREATE TABLE s.c2 PARTITION OF p FOR VALUES FROM (2)TO(3); > > ALTER TABLE p MERGE PARTITIONS (s.c1, s.c2) INTO s.c1; -- misbehaves if > > merging into the same name as an existing partition > > \d+ p > > ... > > Partitions: c1 FOR VALUES FROM (1) TO (3) > > There is already a test which checks merging into the same name as an > existing partition. And there are tests with schema-qualified names. > I'm not yet convinced we need a test with both these properties > together. I mentioned that the combination of schemas and merge-into-same-name is what currently doesn't work right. > > Also, extended stats objects are currently cloned to new child tables. > > But I suggested in [0] that they probably shouldn't be. > > I will explore this. Do we copy extended stats when we do CREATE > TABLE ... PARTITION OF? I think we need to do the same here. Right, they're not copied because an extended stats objs on the parent does something different than putting stats objects on each child. I've convinced myself that it's wrong to copy the parent's stats obj. If someone wants stats objects on each child, they'll have to handle them specially after MERGE/SPLIT, just as they would for per-child defaults/constraints/etc. On Sun, Apr 28, 2024 at 04:04:54AM +0300, Alexander Korotkov wrote: > On Wed, Apr 24, 2024 at 11:26 PM Justin Pryzby wrote: > > This patch adds documentation saying: > > + Any indexes, constraints and user-defined row-level triggers that > > exist > > + in the parent table are cloned on new partitions [...] > > > > Which is good to say, and addresses part of my message [0] > > [0] ZiJW1g2nbQs9ekwK@pryzbyj2023 > > Makes sense. Extracted this into a separate patch in v10. I adjusted the language some and fixed a typo in the commit message. s/parition/partition/ -- Justin >From e00033fc4b8254c70bf8a3d41d513edd9540e2d7 Mon Sep 17 00:00:00 2001 From: Alexander Korotkov Date: Sun, 28 Apr 2024 03:39:30 +0300 Subject: [PATCH] Document the way partition MERGE/SPLIT operations create new partitions Reported-by: Justin Pryzby Discussion: https://postgr.es/m/ZilrByTp-pbz6Mvf%40pryzbyj2023 --- doc/src/sgml/ref/alter_table.sgml | 12 1 file changed, 12 insertions(+) diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml index fe36ff82e52..fc2dfffe49f 100644 --- a/doc/src/sgml/ref/alter_table.sgml +++ b/doc/src/sgml/ref/alter_table.sgml @@ -1153,6 +1153,12 @@ WITH ( MODULUS numeric_literal, REM splitting we have a partition with the same name). Only simple, non-partitioned partition can be split. + + The new partitions will be created the same as tables created with the + SQL command CREATE TABLE partition_nameN (LIKE name INCLUDING ALL EXCLUDING INDEXES EXCLUDING IDENTITY). + The indexes and identity are created later, after moving the data + into the new partitions. + This command acquires an ACCESS EXCLUSIVE lock. @@ -1213,6 +1219,12 @@ WITH ( MODULUS numeric_literal, REM can have the same name as one of the merged partitions. Only simple, non-partitioned partitions can be merged. + + The new partition will be created the same as a table created with the + SQL command CREATE TABLE partition_name (LIKE name INCLUDING ALL EXCLUDING INDEXES EXCLUDING IDENTITY). + The indexes and identity are created later, after moving the data + into the new partition. + This command acquires an ACCESS EXCLUSIVE lock. -- 2.42.0
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
On Sun, Apr 28, 2024 at 2:00 PM Alexander Lakhin wrote: > 28.04.2024 03:59, Alexander Korotkov wrote: > > The revised patchset is attached. I'm going to push it if there are > > no objections. > > I have one additional question regarding security, if you don't mind: > What permissions should a user have to perform split/merge? > > When we deal with mixed ownership, say, bob is an owner of a > partitioned table, but not an owner of a partition, should we > allow him to perform merge with that partition? > Consider the following script: > CREATE ROLE alice; > GRANT CREATE ON SCHEMA public TO alice; > > SET SESSION AUTHORIZATION alice; > CREATE TABLE t (i int PRIMARY KEY, t text, u text) PARTITION BY RANGE (i); > CREATE TABLE tp_00 PARTITION OF t FOR VALUES FROM (0) TO (10); > CREATE TABLE tp_10 PARTITION OF t FOR VALUES FROM (10) TO (20); > > CREATE POLICY p1 ON tp_00 USING (u = current_user); > ALTER TABLE tp_00 ENABLE ROW LEVEL SECURITY; > > INSERT INTO t(i, t, u) VALUES (0, 'info for bob', 'bob'); > INSERT INTO t(i, t, u) VALUES (1, 'info for alice', 'alice'); > RESET SESSION AUTHORIZATION; > > CREATE ROLE bob; > GRANT CREATE ON SCHEMA public TO bob; > ALTER TABLE t OWNER TO bob; > GRANT SELECT ON TABLE tp_00 TO bob; > > SET SESSION AUTHORIZATION bob; > SELECT * FROM tp_00; > --- here bob can see his info only > \d > Schema | Name | Type| Owner > +---+---+--- > public | t | partitioned table | bob > public | tp_00 | table | alice > public | tp_10 | table | alice > > -- but then bob can do: > ALTER TABLE t MERGE PARTITIONS (tp_00, tp_10) INTO tp_00; > -- (yes, he also can detach the partition tp_00, but then he couldn't > -- re-attach nor read it) > > \d > Schema | Name | Type| Owner > +---+---+--- > public | t | partitioned table | bob > public | tp_00 | table | bob > > Thus bob effectively have captured the partition with the data. > > What do you think, does this create a new security risk? Alexander, thank you for discovering this. I believe that the one who merges partitions should have permissions for all the partitions merged. I'll recheck this and provide the patch. -- Regards, Alexander Korotkov
Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
Hello, 28.04.2024 03:59, Alexander Korotkov wrote: The revised patchset is attached. I'm going to push it if there are no objections. I have one additional question regarding security, if you don't mind: What permissions should a user have to perform split/merge? When we deal with mixed ownership, say, bob is an owner of a partitioned table, but not an owner of a partition, should we allow him to perform merge with that partition? Consider the following script: CREATE ROLE alice; GRANT CREATE ON SCHEMA public TO alice; SET SESSION AUTHORIZATION alice; CREATE TABLE t (i int PRIMARY KEY, t text, u text) PARTITION BY RANGE (i); CREATE TABLE tp_00 PARTITION OF t FOR VALUES FROM (0) TO (10); CREATE TABLE tp_10 PARTITION OF t FOR VALUES FROM (10) TO (20); CREATE POLICY p1 ON tp_00 USING (u = current_user); ALTER TABLE tp_00 ENABLE ROW LEVEL SECURITY; INSERT INTO t(i, t, u) VALUES (0, 'info for bob', 'bob'); INSERT INTO t(i, t, u) VALUES (1, 'info for alice', 'alice'); RESET SESSION AUTHORIZATION; CREATE ROLE bob; GRANT CREATE ON SCHEMA public TO bob; ALTER TABLE t OWNER TO bob; GRANT SELECT ON TABLE tp_00 TO bob; SET SESSION AUTHORIZATION bob; SELECT * FROM tp_00; --- here bob can see his info only \d Schema | Name | Type | Owner +---+---+--- public | t | partitioned table | bob public | tp_00 | table | alice public | tp_10 | table | alice -- but then bob can do: ALTER TABLE t MERGE PARTITIONS (tp_00, tp_10) INTO tp_00; -- (yes, he also can detach the partition tp_00, but then he couldn't -- re-attach nor read it) \d Schema | Name | Type | Owner +---+---+--- public | t | partitioned table | bob public | tp_00 | table | bob Thus bob effectively have captured the partition with the data. What do you think, does this create a new security risk? Best regards, Alexander
Re: New committers: Melanie Plageman, Richard Guo
On 2024-Apr-26, Jonathan S. Katz wrote: > The Core Team would like to extend our congratulations to Melanie Plageman > and Richard Guo, who have accepted invitations to become our newest > PostgreSQL committers. > > Please join us in wishing them much success and few reverts! May your commits be plenty and your reverts rare :-) -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "Ni aún el genio muy grande llegaría muy lejos si tuviera que sacarlo todo de su propio interior" (Goethe)
Re: Typos in the code and README
On Sat, 20 Apr 2024 at 16:09, David Rowley wrote: > Here are a few more to see if it motivates anyone else to do a more > thorough search for another batch. I've pushed these now. David
Re: partitioning and identity column
27.04.2024 18:00, Alexander Lakhin wrote: Please look also at another script, which produces the same error: I've discovered yet another problematic case: CREATE TABLE tbl1 (a int GENERATED ALWAYS AS IDENTITY, b text) PARTITION BY LIST (a); CREATE TABLE tbl2 (b text, a int NOT NULL); ALTER TABLE tbl1 ATTACH PARTITION tbl2 DEFAULT; INSERT INTO tbl2 DEFAULT VALUES; ERROR: no owned sequence found Though it works with tbl2(a int NOT NULL, b text)... Take a look at this too, please. Best regards, Alexander
