public inbox for [email protected]help / color / mirror / Atom feed
Re: Possible regression setting GUCs on \connect 24+ messages / 8 participants [nested] [flat]
* Re: Possible regression setting GUCs on \connect @ 2023-04-27 16:13 Tom Lane <[email protected]> 0 siblings, 1 reply; 24+ messages in thread From: Tom Lane @ 2023-04-27 16:13 UTC (permalink / raw) To: David Steele <[email protected]>; +Cc: pgsql-hackers David Steele <[email protected]> writes: > I have been updating pgAudit for PG16 and ran into the following issue > in the regression tests: > \connect - user1 > WARNING: permission denied to set parameter "pgaudit.log_level" > This happens after switching back and forth a few times between the > current user when the regression script was executed and user1 which is > created in the script. Specifically, it happens at [1]. If this is new in v16, I'm inclined to blame 096dd80f3, but that's just a guess without a test case. regards, tom lane ^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Possible regression setting GUCs on \connect @ 2023-04-27 16:35 David Steele <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 1 reply; 24+ messages in thread From: David Steele @ 2023-04-27 16:35 UTC (permalink / raw) To: Alexander Korotkov <[email protected]>; +Cc: pgsql-hackers; Tom Lane <[email protected]> On 4/27/23 19:13, Tom Lane wrote: > David Steele <[email protected]> writes: >> I have been updating pgAudit for PG16 and ran into the following issue >> in the regression tests: >> \connect - user1 >> WARNING: permission denied to set parameter "pgaudit.log_level" >> This happens after switching back and forth a few times between the >> current user when the regression script was executed and user1 which is >> created in the script. Specifically, it happens at [1]. > > If this is new in v16, I'm inclined to blame 096dd80f3, but that's > just a guess without a test case. Seems plausible. This can be reproduced by cloning [1] into contrib and running `make check`. I can work out another test case but it may not end up being simpler. Thoughts on this Alexander? [1] https://github.com/pgaudit/pgaudit/tree/dev-pg16-ci ^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Possible regression setting GUCs on \connect @ 2023-04-27 18:16 Tom Lane <[email protected]> parent: David Steele <[email protected]> 0 siblings, 1 reply; 24+ messages in thread From: Tom Lane @ 2023-04-27 18:16 UTC (permalink / raw) To: David Steele <[email protected]>; +Cc: Alexander Korotkov <[email protected]>; pgsql-hackers David Steele <[email protected]> writes: > Seems plausible. This can be reproduced by cloning [1] into contrib and > running `make check`. I can work out another test case but it may not > end up being simpler. > [1] https://github.com/pgaudit/pgaudit/tree/dev-pg16-ci I tried to replicate this per that recipe, but it works for me: $ git clone https://github.com/pgaudit/pgaudit.git pgaudit $ cd pgaudit $ git checkout dev-pg16-ci $ make -s check # +++ regress check in contrib/pgaudit +++ # using temp instance on port 61696 with PID 1191703 ok 1 - pgaudit 310 ms 1..1 # All 1 tests passed. This is at commit 6f879bddbdcfbf9995ecee1db9a587e06027bd13 on your dev-pg16-ci branch and df38157d94662a64e2f83aa8a0110fd1ee7c4776 on PG master. Note that I had to add $ diff -pud Makefile~ Makefile --- Makefile~ 2023-04-27 14:02:19.041714415 -0400 +++ Makefile 2023-04-27 14:07:10.056909016 -0400 @@ -20,3 +20,5 @@ top_builddir = ../.. include $(top_builddir)/src/Makefile.global include $(top_srcdir)/contrib/contrib-global.mk endif + +EXTRA_INSTALL += contrib/pg_stat_statements else I got failures about pg_stat_statements not being installed. But I don't see the failure you're complaining of. regards, tom lane ^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Possible regression setting GUCs on \connect @ 2023-04-27 18:28 David Steele <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 2 replies; 24+ messages in thread From: David Steele @ 2023-04-27 18:28 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Alexander Korotkov <[email protected]>; pgsql-hackers On 4/27/23 21:16, Tom Lane wrote: > David Steele <[email protected]> writes: >> Seems plausible. This can be reproduced by cloning [1] into contrib and >> running `make check`. I can work out another test case but it may not >> end up being simpler. >> [1] https://github.com/pgaudit/pgaudit/tree/dev-pg16-ci > > I tried to replicate this per that recipe, but it works for me: > > $ git clone https://github.com/pgaudit/pgaudit.git pgaudit > $ cd pgaudit > $ git checkout dev-pg16-ci > $ make -s check > # +++ regress check in contrib/pgaudit +++ > # using temp instance on port 61696 with PID 1191703 > ok 1 - pgaudit 310 ms > 1..1 > # All 1 tests passed. I included the errors in the expect log so I could link to them. So test success means the error is happening. > Note that I had to add > > $ diff -pud Makefile~ Makefile > --- Makefile~ 2023-04-27 14:02:19.041714415 -0400 > +++ Makefile 2023-04-27 14:07:10.056909016 -0400 > @@ -20,3 +20,5 @@ top_builddir = ../.. > include $(top_builddir)/src/Makefile.global > include $(top_srcdir)/contrib/contrib-global.mk > endif > + > +EXTRA_INSTALL += contrib/pg_stat_statements Yeah, I rarely run tests in-tree, but I'll add this if it does not break our regular CI. -David ^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Possible regression setting GUCs on \connect @ 2023-04-27 18:43 Nathan Bossart <[email protected]> parent: David Steele <[email protected]> 1 sibling, 1 reply; 24+ messages in thread From: Nathan Bossart @ 2023-04-27 18:43 UTC (permalink / raw) To: David Steele <[email protected]>; +Cc: Tom Lane <[email protected]>; Alexander Korotkov <[email protected]>; pgsql-hackers I suspect the problem is that GUCArrayDelete() is using the wrong Datum: diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 9dd624b3ae..ee9f87e7f2 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -6496,7 +6496,7 @@ GUCArrayDelete(ArrayType *array, ArrayType **usersetArray, const char *name) { newarray = construct_array_builtin(&d, 1, TEXTOID); if (usersetArray) - newUsersetArray = construct_array_builtin(&d, 1, BOOLOID); + newUsersetArray = construct_array_builtin(&userSetDatum, 1, BOOLOID); } index++; -- Nathan Bossart Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Possible regression setting GUCs on \connect @ 2023-04-27 19:22 Tom Lane <[email protected]> parent: David Steele <[email protected]> 1 sibling, 1 reply; 24+ messages in thread From: Tom Lane @ 2023-04-27 19:22 UTC (permalink / raw) To: David Steele <[email protected]>; +Cc: Alexander Korotkov <[email protected]>; pgsql-hackers David Steele <[email protected]> writes: > On 4/27/23 21:16, Tom Lane wrote: >> I tried to replicate this per that recipe, but it works for me: > I included the errors in the expect log so I could link to them. So test > success means the error is happening. Ah, got it. I added some debug output to the test, and what I see is that at the "\connect - user1" commands that work ok, what we have in pg_db_role_setting is along the lines of +select setdatabase, setrole::regrole, setconfig, setuser from pg_db_role_setting; + setdatabase | setrole | setconfig | setuser +-------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------------+--------------- + 0 | user1 | {"pgaudit.log=read, WRITE",pgaudit.log_level=notice,pgaudit.log_client=on,pgaudit.role=auditor,pgaudit.log_relation=on} | {f,f,f,f,f} ... while where it's not working: +select setdatabase, setrole::regrole, setconfig, setuser from pg_db_role_setting; + setdatabase | setrole | setconfig | setuser +-------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------------+--------------- + 0 | user1 | {pgaudit.log_level=notice,pgaudit.log_client=on,pgaudit.role=auditor,pgaudit.log_statement=off} | {t,f,f,f} ... So it is failing because setuser = 't' for that setting; which makes the failure itself unsurprising, but it seems like the flag should not have been set that way. Digging further, it looks like the flag array is not correctly updated during ALTER USER RESET: select setdatabase, setrole::regrole, setconfig, setuser from pg_db_role_setting; NOTICE: AUDIT: SESSION,1,1,READ,SELECT,TABLE,pg_catalog.pg_db_role_setting,"select setdatabase, setrole::regrole, setconfig, setuser from pg_db_role_setting;",<not logged> setdatabase | setrole | setconfig | setuser -------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------------+--------------- 0 | user1 | {"pgaudit.log=read, WRITE",pgaudit.log_level=notice,pgaudit.log_client=on,pgaudit.role=auditor,pgaudit.log_relation=on} | {f,f,f,f,f} ... that's fine ... ALTER ROLE user1 RESET pgaudit.log_relation; select setdatabase, setrole::regrole, setconfig, setuser from pg_db_role_setting; setdatabase | setrole | setconfig | setuser -------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------------+--------------- 0 | user1 | {"pgaudit.log=read, WRITE",pgaudit.log_level=notice,pgaudit.log_client=on,pgaudit.role=auditor} | {t,f,f,f} ... wrong ... ALTER ROLE user1 RESET pgaudit.log; select setdatabase, setrole::regrole, setconfig, setuser from pg_db_role_setting; setdatabase | setrole | setconfig | setuser -------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------------+--------------- 0 | user1 | {pgaudit.log_level=notice,pgaudit.log_client=on,pgaudit.role=auditor} | {t,f,f} ... and wronger. I had not paid any attention to 096dd80f3 when it went in, but now that I have looked at it I'm quite distressed, independently of this probably-minor bug. It seems to me that this feature is not well designed and completely ignores the precedents set by commits a0ffa885e and 13d838815. The right way to do this was not to add some poorly-explained option to ALTER ROLE, but to record the role OID that issued the ALTER ROLE, and then to check when loading the ALTER ROLE setting whether that role (still) has the right to change the specified setting. As implemented, this can't possibly track changes in GRANT/REVOKE SET privileges correctly, and I wonder if it's not introducing outright security holes like the one fixed by 13d838815. I think we ought to revert 096dd80f3 and try again in v17. regards, tom lane ^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Possible regression setting GUCs on \connect @ 2023-04-27 19:34 Tom Lane <[email protected]> parent: Nathan Bossart <[email protected]> 0 siblings, 0 replies; 24+ messages in thread From: Tom Lane @ 2023-04-27 19:34 UTC (permalink / raw) To: Nathan Bossart <[email protected]>; +Cc: David Steele <[email protected]>; Alexander Korotkov <[email protected]>; pgsql-hackers Nathan Bossart <[email protected]> writes: > I suspect the problem is that GUCArrayDelete() is using the wrong Datum: > - newUsersetArray = construct_array_builtin(&d, 1, BOOLOID); > + newUsersetArray = construct_array_builtin(&userSetDatum, 1, BOOLOID); Ah, should have checked my mail earlier. However, my concern about whether we even want this feature still stands. regards, tom lane ^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Possible regression setting GUCs on \connect @ 2023-04-27 22:14 Nathan Bossart <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 1 reply; 24+ messages in thread From: Nathan Bossart @ 2023-04-27 22:14 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: David Steele <[email protected]>; Alexander Korotkov <[email protected]>; pgsql-hackers On Thu, Apr 27, 2023 at 03:22:04PM -0400, Tom Lane wrote: > The right way to do this was not to add some > poorly-explained option to ALTER ROLE, but to record the role OID that > issued the ALTER ROLE, and then to check when loading the ALTER ROLE > setting whether that role (still) has the right to change the > specified setting. As implemented, this can't possibly track changes > in GRANT/REVOKE SET privileges correctly, and I wonder if it's not > introducing outright security holes like the one fixed by 13d838815. I generally agree. At least, I think it would be nice to avoid adding a new option if possible. It's not clear to me why we'd need to also check privileges at login time as opposed to only checking them at ALTER ROLE SET time. ISTM that the former approach would introduce some interesting problems around dropping roles or changing roles' privileges. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Possible regression setting GUCs on \connect @ 2023-04-27 22:38 Tom Lane <[email protected]> parent: Nathan Bossart <[email protected]> 0 siblings, 1 reply; 24+ messages in thread From: Tom Lane @ 2023-04-27 22:38 UTC (permalink / raw) To: Nathan Bossart <[email protected]>; +Cc: David Steele <[email protected]>; Alexander Korotkov <[email protected]>; pgsql-hackers Nathan Bossart <[email protected]> writes: > On Thu, Apr 27, 2023 at 03:22:04PM -0400, Tom Lane wrote: >> The right way to do this was not to add some >> poorly-explained option to ALTER ROLE, but to record the role OID that >> issued the ALTER ROLE, and then to check when loading the ALTER ROLE >> setting whether that role (still) has the right to change the >> specified setting. As implemented, this can't possibly track changes >> in GRANT/REVOKE SET privileges correctly, and I wonder if it's not >> introducing outright security holes like the one fixed by 13d838815. > I generally agree. At least, I think it would be nice to avoid adding a > new option if possible. It's not clear to me why we'd need to also check > privileges at login time as opposed to only checking them at ALTER ROLE SET > time. Perhaps there's room to argue about that. But ISTM that if someone does ALTER ROLE SET on the strength of some privilege you granted them, and then you regret that and revoke the privilege, then the ALTER ROLE setting should not continue to work. So I would regard the session-start-time check as the primary one. Checking when ALTER ROLE is done is just a user-friendliness detail. Also, in the case of an extension-defined GUC, we don't necessarily know its privilege level at either ALTER ROLE time or session start, since the extension might not yet be loaded at either point. I've forgotten exactly what restrictive hack we use to get around that, but the *only* way to do that fully correctly is to record the role that did the ALTER and then check its privileges at extension load time. I think that 096dd80f3 has made it harder not easier to get to the point of doing that correctly, because it's added a feature that we'll have to figure out how to make interoperate with a correct implementation. regards, tom lane ^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Possible regression setting GUCs on \connect @ 2023-04-27 23:30 Alexander Korotkov <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 2 replies; 24+ messages in thread From: Alexander Korotkov @ 2023-04-27 23:30 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Nathan Bossart <[email protected]>; David Steele <[email protected]>; pgsql-hackers On Fri, Apr 28, 2023 at 1:38 AM Tom Lane <[email protected]> wrote: > Nathan Bossart <[email protected]> writes: > > On Thu, Apr 27, 2023 at 03:22:04PM -0400, Tom Lane wrote: > >> The right way to do this was not to add some > >> poorly-explained option to ALTER ROLE, but to record the role OID that > >> issued the ALTER ROLE, and then to check when loading the ALTER ROLE > >> setting whether that role (still) has the right to change the > >> specified setting. As implemented, this can't possibly track changes > >> in GRANT/REVOKE SET privileges correctly, and I wonder if it's not > >> introducing outright security holes like the one fixed by 13d838815. > > > I generally agree. At least, I think it would be nice to avoid adding a > > new option if possible. It's not clear to me why we'd need to also check > > privileges at login time as opposed to only checking them at ALTER ROLE SET > > time. > > Perhaps there's room to argue about that. But ISTM that if someone > does ALTER ROLE SET on the strength of some privilege you granted > them, and then you regret that and revoke the privilege, then the > ALTER ROLE setting should not continue to work. So I would regard > the session-start-time check as the primary one. Checking when > ALTER ROLE is done is just a user-friendliness detail. From my point of view that is much different from what we're doing with other database objects. If some role gets revoked from privilege, that doesn't affect the actions done with that privilege before. The law is not retroactive. If one has created some tables, those tables still work if the creator gets revoked privilege or even gets deleted. Why should the setting behave differently? Additionally, I think if we start recording role OID, then we need a full set of management clauses for each individual option ownership. Otherwise, we would leave this new role OID without necessarily management facilities. But with them, the whole stuff will look like awful overengineering. > Also, in the case of an extension-defined GUC, we don't necessarily > know its privilege level at either ALTER ROLE time or session start, > since the extension might not yet be loaded at either point. Yes, that's it. > I've forgotten exactly what restrictive hack we use to get around > that, As I understand the restrictive hack is to assume that the role is at least SUSET. > but the *only* way to do that fully correctly is to record > the role that did the ALTER and then check its privileges at extension > load time. This depends on the understanding of correctness. Recording role OID would mean altering that role privileges or deleting the role would affect the settings. Even if that is correct, this is very very far from the behavior we had for decades. > I think that 096dd80f3 has made it harder not easier > to get to the point of doing that correctly, because it's added > a feature that we'll have to figure out how to make interoperate > with a correct implementation. With 096dd80f3, we still may revoke the setting of USERSET options from the public. Even if the option is not currently loaded at ALTER time, we still may find an explicit revoke recorder in the system catalog. That behavior will be current if we understand the default options as separate material things (as it is today), but not part of the setter role. ------ Regards, Alexander Korotkov ^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Possible regression setting GUCs on \connect @ 2023-04-28 00:04 Alexander Korotkov <[email protected]> parent: Alexander Korotkov <[email protected]> 1 sibling, 1 reply; 24+ messages in thread From: Alexander Korotkov @ 2023-04-28 00:04 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Nathan Bossart <[email protected]>; David Steele <[email protected]>; pgsql-hackers On Fri, Apr 28, 2023 at 2:30 AM Alexander Korotkov <[email protected]> wrote: > Additionally, I think if we start recording role OID, then we need a > full set of management clauses for each individual option ownership. > Otherwise, we would leave this new role OID without necessarily > management facilities. But with them, the whole stuff will look like > awful overengineering. I can also predict a lot of ambiguous cases. For instance, we existing setting can be overridden with a different role OID. If it has been overridden can the overwriter turn it back? ------ Regards, Alexander Korotkov ^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Possible regression setting GUCs on \connect @ 2023-04-28 13:42 Jonathan S. Katz <[email protected]> parent: Alexander Korotkov <[email protected]> 0 siblings, 0 replies; 24+ messages in thread From: Jonathan S. Katz @ 2023-04-28 13:42 UTC (permalink / raw) To: Alexander Korotkov <[email protected]>; Tom Lane <[email protected]>; +Cc: Nathan Bossart <[email protected]>; David Steele <[email protected]>; pgsql-hackers; [email protected] On 4/27/23 8:04 PM, Alexander Korotkov wrote: > On Fri, Apr 28, 2023 at 2:30 AM Alexander Korotkov <[email protected]> wrote: >> Additionally, I think if we start recording role OID, then we need a >> full set of management clauses for each individual option ownership. >> Otherwise, we would leave this new role OID without necessarily >> management facilities. But with them, the whole stuff will look like >> awful overengineering. > > I can also predict a lot of ambiguous cases. For instance, we > existing setting can be overridden with a different role OID. If it > has been overridden can the overwriter turn it back? [RMT hat] While the initial bug has been fixed, given there is discussion on reverting 096dd80f3, I've added this as an open item. I want to study this a bit more before providing my own opinion on revert. Thanks, Jonathan Attachments: [application/pgp-signature] OpenPGP_signature (840B, ../../[email protected]/2-OpenPGP_signature) download ^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Possible regression setting GUCs on \connect @ 2023-05-17 06:47 Amit Kapila <[email protected]> parent: Alexander Korotkov <[email protected]> 1 sibling, 2 replies; 24+ messages in thread From: Amit Kapila @ 2023-05-17 06:47 UTC (permalink / raw) To: Alexander Korotkov <[email protected]>; +Cc: Tom Lane <[email protected]>; Nathan Bossart <[email protected]>; David Steele <[email protected]>; pgsql-hackers On Fri, Apr 28, 2023 at 5:01 AM Alexander Korotkov <[email protected]> wrote: > > On Fri, Apr 28, 2023 at 1:38 AM Tom Lane <[email protected]> wrote: > > Nathan Bossart <[email protected]> writes: > > > On Thu, Apr 27, 2023 at 03:22:04PM -0400, Tom Lane wrote: > > >> The right way to do this was not to add some > > >> poorly-explained option to ALTER ROLE, but to record the role OID that > > >> issued the ALTER ROLE, and then to check when loading the ALTER ROLE > > >> setting whether that role (still) has the right to change the > > >> specified setting. As implemented, this can't possibly track changes > > >> in GRANT/REVOKE SET privileges correctly, and I wonder if it's not > > >> introducing outright security holes like the one fixed by 13d838815. > > > > > I generally agree. At least, I think it would be nice to avoid adding a > > > new option if possible. It's not clear to me why we'd need to also check > > > privileges at login time as opposed to only checking them at ALTER ROLE SET > > > time. > > > > Perhaps there's room to argue about that. But ISTM that if someone > > does ALTER ROLE SET on the strength of some privilege you granted > > them, and then you regret that and revoke the privilege, then the > > ALTER ROLE setting should not continue to work. So I would regard > > the session-start-time check as the primary one. Checking when > > ALTER ROLE is done is just a user-friendliness detail. > > From my point of view that is much different from what we're doing > with other database objects. If some role gets revoked from > privilege, that doesn't affect the actions done with that privilege > before. The law is not retroactive. If one has created some tables, > those tables still work if the creator gets revoked privilege or even > gets deleted. Why should the setting behave differently? > I see there are mainly three concerns (a) Avoid adding the new option USER SET, (b) The behavior of this feature varies from the precedents set by a0ffa885e and 13d838815, (c) As per discussion, not following 13d838815 could lead to a similar security hole in this feature. Now, I don't know whether Tom and or Nathan share your viewpoint and feel that nothing should be done. It would have been better if such a discussion happens during development but I can understand that mostly the other senior people are sometimes busy enough to pay attention to all the work going on. I see that when Alexander proposed this new option and behavior in the original thread [1], there were no objections, so the commit followed the normal community rules but we have seen various times that post-commit reviews also lead to changing or reverting the feature. I see that you seem to think it would be over-engineering to follow the suggestions shared here but without the patch or some further discussion, it won't be easy to conclude that. Tom/Nathan, do you have any further suggestions here? [1] - https://www.postgresql.org/message-id/[email protected]... -- With Regards, Amit Kapila. ^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Possible regression setting GUCs on \connect @ 2023-05-17 11:56 Alexander Korotkov <[email protected]> parent: Amit Kapila <[email protected]> 1 sibling, 0 replies; 24+ messages in thread From: Alexander Korotkov @ 2023-05-17 11:56 UTC (permalink / raw) To: Amit Kapila <[email protected]>; +Cc: Tom Lane <[email protected]>; Nathan Bossart <[email protected]>; David Steele <[email protected]>; pgsql-hackers Hi, Amit. On Wed, May 17, 2023 at 9:47 AM Amit Kapila <[email protected]> wrote: > I see there are mainly three concerns (a) Avoid adding the new option > USER SET, (b) The behavior of this feature varies from the precedents > set by a0ffa885e and 13d838815, (c) As per discussion, not following > 13d838815 could lead to a similar security hole in this feature. > > Now, I don't know whether Tom and or Nathan share your viewpoint and > feel that nothing should be done. It would have been better if such a > discussion happens during development but I can understand that mostly > the other senior people are sometimes busy enough to pay attention to > all the work going on. I see that when Alexander proposed this new > option and behavior in the original thread [1], there were no > objections, so the commit followed the normal community rules but we > have seen various times that post-commit reviews also lead to changing > or reverting the feature. > > I see that you seem to think it would be over-engineering to follow > the suggestions shared here but without the patch or some further > discussion, it won't be easy to conclude that. > > Tom/Nathan, do you have any further suggestions here? I think the main question regarding the USER SET option is its contradiction with Tom's plans to track the setter role OID per setting. If we do track role OID then it makes USER SET both unnecessary for users and undesired complications for development. However, I've expressed my doubts about the tracking setter role OID [1], [2]. I think these plans look good in the big picture, but implementation will have so many caveats that implementation will stall for a long time (probably forever). If we accept this view, the USER SET option might seem a good practical solution for real-world issues. I think if we would elaborate more on tracking setter role OID, come to at least sketchy design then it could be easily to come to an agreement on future directions. Links. 1. https://www.postgresql.org/message-id/CAPpHfdsy-jxhgR0bWk1Fv63c6txwMAkzxFMGMf29jqa9uU_CdQ%40mail.gma... 2. https://www.postgresql.org/message-id/CAPpHfdu6roOVEUsV9TWNdQ%3DTZCrNEEwJM62EQiKULUyjpERhtg%40mail.g... ------ Regards, Alexander Korotkov ^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Possible regression setting GUCs on \connect @ 2023-05-17 12:08 Tom Lane <[email protected]> parent: Amit Kapila <[email protected]> 1 sibling, 2 replies; 24+ messages in thread From: Tom Lane @ 2023-05-17 12:08 UTC (permalink / raw) To: Amit Kapila <[email protected]>; +Cc: Alexander Korotkov <[email protected]>; Nathan Bossart <[email protected]>; David Steele <[email protected]>; pgsql-hackers Amit Kapila <[email protected]> writes: > Tom/Nathan, do you have any further suggestions here? My recommendation is to revert this feature. I do not see any way that we won't regret it as a poor design. regards, tom lane ^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Possible regression setting GUCs on \connect @ 2023-05-17 16:47 Nathan Bossart <[email protected]> parent: Tom Lane <[email protected]> 1 sibling, 1 reply; 24+ messages in thread From: Nathan Bossart @ 2023-05-17 16:47 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Amit Kapila <[email protected]>; Alexander Korotkov <[email protected]>; David Steele <[email protected]>; pgsql-hackers On Wed, May 17, 2023 at 08:08:36AM -0400, Tom Lane wrote: > Amit Kapila <[email protected]> writes: >> Tom/Nathan, do you have any further suggestions here? > > My recommendation is to revert this feature. I do not see any > way that we won't regret it as a poor design. I agree. The problem seems worth solving, but I think we ought to consider a different approach. Apologies for not chiming in earlier on the original thread. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Possible regression setting GUCs on \connect @ 2023-05-17 16:57 Jonathan S. Katz <[email protected]> parent: Nathan Bossart <[email protected]> 0 siblings, 0 replies; 24+ messages in thread From: Jonathan S. Katz @ 2023-05-17 16:57 UTC (permalink / raw) To: Nathan Bossart <[email protected]>; Tom Lane <[email protected]>; +Cc: Amit Kapila <[email protected]>; Alexander Korotkov <[email protected]>; David Steele <[email protected]>; pgsql-hackers On 5/17/23 12:47 PM, Nathan Bossart wrote: > On Wed, May 17, 2023 at 08:08:36AM -0400, Tom Lane wrote: >> Amit Kapila <[email protected]> writes: >>> Tom/Nathan, do you have any further suggestions here? >> >> My recommendation is to revert this feature. I do not see any >> way that we won't regret it as a poor design. > > I agree. The problem seems worth solving, but I think we ought to consider > a different approach. Apologies for not chiming in earlier on the original > thread. [RMT hat, personal opinion] I do agree that the feature itself is useful, but given there is disagreement over the feature design, particularly from people who have spent time working on features and analyzing the security ramifications in this area, the safest option is to revert and try again for v17. I suggest we revert before Beta 1. Thanks, Jonathan Attachments: [application/pgp-signature] OpenPGP_signature (840B, ../../[email protected]/2-OpenPGP_signature) download ^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Possible regression setting GUCs on \connect @ 2023-05-17 17:30 Alexander Korotkov <[email protected]> parent: Tom Lane <[email protected]> 1 sibling, 2 replies; 24+ messages in thread From: Alexander Korotkov @ 2023-05-17 17:30 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Amit Kapila <[email protected]>; Nathan Bossart <[email protected]>; David Steele <[email protected]>; pgsql-hackers Tom, On Wed, May 17, 2023 at 3:08 PM Tom Lane <[email protected]> wrote: > Amit Kapila <[email protected]> writes: > > Tom/Nathan, do you have any further suggestions here? > > My recommendation is to revert this feature. I do not see any > way that we won't regret it as a poor design. I have carefully noted your concerns regarding the USER SET patch that I've committed. It's clear that you have strong convictions about this, particularly in relation to your plan of storing the setter role OID in pg_db_role_setting. I want to take a moment to acknowledge the significance of your perspective and I respect that you have a different view on this matter. Although I have not yet had the opportunity to see the feasibility of your approach, I am open to understanding it further. Anyway, I don't want to do anything counter-productive. So, I've taken the decision to revert the USER SET patch for the time being. I'm looking forward to continuing working with you on this subject for v17. ------ Regards, Alexander Korotkov ^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Possible regression setting GUCs on \connect @ 2023-05-17 19:23 Jonathan S. Katz <[email protected]> parent: Alexander Korotkov <[email protected]> 1 sibling, 0 replies; 24+ messages in thread From: Jonathan S. Katz @ 2023-05-17 19:23 UTC (permalink / raw) To: Alexander Korotkov <[email protected]>; Tom Lane <[email protected]>; +Cc: Amit Kapila <[email protected]>; Nathan Bossart <[email protected]>; David Steele <[email protected]>; pgsql-hackers On 5/17/23 1:30 PM, Alexander Korotkov wrote: > Tom, > > On Wed, May 17, 2023 at 3:08 PM Tom Lane <[email protected]> wrote: >> Amit Kapila <[email protected]> writes: >>> Tom/Nathan, do you have any further suggestions here? >> >> My recommendation is to revert this feature. I do not see any >> way that we won't regret it as a poor design. > > I have carefully noted your concerns regarding the USER SET patch that > I've committed. It's clear that you have strong convictions about > this, particularly in relation to your plan of storing the setter role > OID in pg_db_role_setting. > > I want to take a moment to acknowledge the significance of your > perspective and I respect that you have a different view on this > matter. Although I have not yet had the opportunity to see the > feasibility of your approach, I am open to understanding it further. > > Anyway, I don't want to do anything counter-productive. So, I've > taken the decision to revert the USER SET patch for the time being. Thanks Alexander. I know reverting a feature is not easy and appreciate you taking the time to work through this discussion. > I'm looking forward to continuing working with you on this subject for v17. +1; I think everyone agrees there is a feature here that will be helpful to our users. Thanks, Jonathan Attachments: [application/pgp-signature] OpenPGP_signature (840B, ../../[email protected]/2-OpenPGP_signature) download ^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Possible regression setting GUCs on \connect @ 2023-05-18 15:47 Robert Haas <[email protected]> parent: Alexander Korotkov <[email protected]> 1 sibling, 1 reply; 24+ messages in thread From: Robert Haas @ 2023-05-18 15:47 UTC (permalink / raw) To: Alexander Korotkov <[email protected]>; +Cc: Tom Lane <[email protected]>; Amit Kapila <[email protected]>; Nathan Bossart <[email protected]>; David Steele <[email protected]>; pgsql-hackers On Wed, May 17, 2023 at 1:31 PM Alexander Korotkov <[email protected]> wrote: > I have carefully noted your concerns regarding the USER SET patch that > I've committed. It's clear that you have strong convictions about > this, particularly in relation to your plan of storing the setter role > OID in pg_db_role_setting. > > I want to take a moment to acknowledge the significance of your > perspective and I respect that you have a different view on this > matter. Although I have not yet had the opportunity to see the > feasibility of your approach, I am open to understanding it further. > > Anyway, I don't want to do anything counter-productive. So, I've > taken the decision to revert the USER SET patch for the time being. This discussion made me go back and look at the commit in question. My opinion is that the feature as it was committed is quite hard to understand. The documentation for it said this: "Specifies that variable should be set on behalf of ordinary role." But what does that even mean? What's an "ordinary role"? What does "on behalf of" mean? I think these are not terms we use elsewhere in the documentation, and I think it wouldn't be easy for users to understand how to use the feature properly. I'm not sure whether Tom's idea about what the design should be is good or bad, but I think that whatever we end up with, we should try to explain more clearly and thoroughly what problem the feature solves and how it does so. Imagine a paragraph someplace that says something like "You might want to do X. But if you try to do it, you'll find that it doesn't work because Y: [SQL example] We can work around this problem using the Z feature. That lets us tell the system that it should Q, which fixes Y: [SQL example].". It sounds like Tom might be proposing that we solve this problem in some way that doesn't actually require any new SQL syntax, and if we do that, then this might not be needed. But if we do add syntax, then I think something like this would be really good to have. -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Possible regression setting GUCs on \connect @ 2023-05-18 18:33 Tom Lane <[email protected]> parent: Robert Haas <[email protected]> 0 siblings, 0 replies; 24+ messages in thread From: Tom Lane @ 2023-05-18 18:33 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: Alexander Korotkov <[email protected]>; Amit Kapila <[email protected]>; Nathan Bossart <[email protected]>; David Steele <[email protected]>; pgsql-hackers Robert Haas <[email protected]> writes: > This discussion made me go back and look at the commit in question. My > opinion is that the feature as it was committed is quite hard to > understand. The documentation for it said this: "Specifies that > variable should be set on behalf of ordinary role." But what does that > even mean? What's an "ordinary role"? What does "on behalf of" mean? Yeah. And even more to the point: how would the feature interact with per-user grants of SET privilege? It seems like it would have to ignore or override that, which is not a conclusion I like at all. I think that commit a0ffa885e pretty much nailed down the user interface we want, and what remains is to work out how granting SET privilege interacts with the time-delayed nature of ALTER USER/DATABASE SET. But the answer to that does not seem difficult to me: remember who issued the ALTER and see if they still have SET privilege at the time we activate a particular entry. regards, tom lane ^ permalink raw reply [nested|flat] 24+ messages in thread
* [PATCH v39] postgres_fdw: Start using new libpq cancel APIs @ 2024-03-18 18:37 Jelte Fennema-Nio <[email protected]> 0 siblings, 0 replies; 24+ messages in thread From: Jelte Fennema-Nio @ 2024-03-18 18:37 UTC (permalink / raw) Commit 61461a300c1c introduced new functions to libpq for cancelling queries. This replaces the usage of the old ones in postgres_fdw. Author: Jelte Fennema-Nio <[email protected]> Discussion: https://postgr.es/m/CAGECzQT_VgOWWENUqvUV9xQmbaCyXjtRRAYO8W07oqashk_N+g@mail.gmail.com --- contrib/postgres_fdw/connection.c | 108 +++++++++++++++--- .../postgres_fdw/expected/postgres_fdw.out | 15 +++ contrib/postgres_fdw/sql/postgres_fdw.sql | 7 ++ 3 files changed, 113 insertions(+), 17 deletions(-) diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c index 4931ebf591..0c66eaa001 100644 --- a/contrib/postgres_fdw/connection.c +++ b/contrib/postgres_fdw/connection.c @@ -133,7 +133,7 @@ static void pgfdw_inval_callback(Datum arg, int cacheid, uint32 hashvalue); static void pgfdw_reject_incomplete_xact_state_change(ConnCacheEntry *entry); static void pgfdw_reset_xact_state(ConnCacheEntry *entry, bool toplevel); static bool pgfdw_cancel_query(PGconn *conn); -static bool pgfdw_cancel_query_begin(PGconn *conn); +static bool pgfdw_cancel_query_begin(PGconn *conn, TimestampTz endtime); static bool pgfdw_cancel_query_end(PGconn *conn, TimestampTz endtime, bool consume_input); static bool pgfdw_exec_cleanup_query(PGconn *conn, const char *query, @@ -1315,36 +1315,106 @@ pgfdw_cancel_query(PGconn *conn) endtime = TimestampTzPlusMilliseconds(GetCurrentTimestamp(), CONNECTION_CLEANUP_TIMEOUT); - if (!pgfdw_cancel_query_begin(conn)) + if (!pgfdw_cancel_query_begin(conn, endtime)) return false; return pgfdw_cancel_query_end(conn, endtime, false); } +/* + * Submit a cancel request to the given connection, waiting only until + * the given time. + * + * We sleep interruptibly until we receive confirmation that the cancel + * request has been accepted, and if it is, return true; if the timeout + * lapses without that, or the request fails for whatever reason, return + * false. + */ static bool -pgfdw_cancel_query_begin(PGconn *conn) +pgfdw_cancel_query_begin(PGconn *conn, TimestampTz endtime) { - PGcancel *cancel; - char errbuf[256]; + bool timed_out = false; + bool failed = false; + PGcancelConn *cancel_conn = PQcancelCreate(conn); - /* - * Issue cancel request. Unfortunately, there's no good way to limit the - * amount of time that we might block inside PQgetCancel(). - */ - if ((cancel = PQgetCancel(conn))) + if (!PQcancelStart(cancel_conn)) { - if (!PQcancel(cancel, errbuf, sizeof(errbuf))) + PG_TRY(); { ereport(WARNING, (errcode(ERRCODE_CONNECTION_FAILURE), errmsg("could not send cancel request: %s", - errbuf))); - PQfreeCancel(cancel); - return false; + pchomp(PQcancelErrorMessage(cancel_conn))))); } - PQfreeCancel(cancel); + PG_FINALLY(); + { + PQcancelFinish(cancel_conn); + } + PG_END_TRY(); + return false; } - return true; + /* In what follows, do not leak any PGcancelConn on an error. */ + PG_TRY(); + { + while (true) + { + PostgresPollingStatusType pollres = PQcancelPoll(cancel_conn); + TimestampTz now = GetCurrentTimestamp(); + long cur_timeout; + int waitEvents = WL_LATCH_SET | WL_TIMEOUT | WL_EXIT_ON_PM_DEATH; + + if (pollres == PGRES_POLLING_OK) + break; + + /* If timeout has expired, give up, else get sleep time. */ + cur_timeout = TimestampDifferenceMilliseconds(now, endtime); + if (cur_timeout <= 0) + { + timed_out = true; + failed = true; + goto exit; + } + + switch (pollres) + { + case PGRES_POLLING_READING: + waitEvents |= WL_SOCKET_READABLE; + break; + case PGRES_POLLING_WRITING: + waitEvents |= WL_SOCKET_WRITEABLE; + break; + default: + failed = true; + goto exit; + } + + /* Sleep until there's something to do */ + WaitLatchOrSocket(MyLatch, waitEvents, PQcancelSocket(cancel_conn), + cur_timeout, PG_WAIT_EXTENSION); + ResetLatch(MyLatch); + + CHECK_FOR_INTERRUPTS(); + } +exit: + if (failed) + { + if (timed_out) + ereport(WARNING, + (errmsg("could not cancel request due to timeout"))); + else + ereport(WARNING, + (errcode(ERRCODE_CONNECTION_FAILURE), + errmsg("could not send cancel request: %s", + pchomp(PQcancelErrorMessage(cancel_conn))))); + } + } + PG_FINALLY(); + { + PQcancelFinish(cancel_conn); + } + PG_END_TRY(); + + return !failed; } static bool @@ -1685,7 +1755,11 @@ pgfdw_abort_cleanup_begin(ConnCacheEntry *entry, bool toplevel, */ if (PQtransactionStatus(entry->conn) == PQTRANS_ACTIVE) { - if (!pgfdw_cancel_query_begin(entry->conn)) + TimestampTz endtime; + + endtime = TimestampTzPlusMilliseconds(GetCurrentTimestamp(), + CONNECTION_CLEANUP_TIMEOUT); + if (!pgfdw_cancel_query_begin(entry->conn, endtime)) return false; /* Unable to cancel running query */ *cancel_requested = lappend(*cancel_requested, entry); } diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out index 58a603ac56..e03160bd97 100644 --- a/contrib/postgres_fdw/expected/postgres_fdw.out +++ b/contrib/postgres_fdw/expected/postgres_fdw.out @@ -2739,6 +2739,21 @@ SELECT t1.c1, t2.c2 FROM v4 t1 LEFT JOIN ft5 t2 ON (t1.c1 = t2.c1) ORDER BY t1.c (10 rows) ALTER VIEW v4 OWNER TO regress_view_owner; +-- Make sure this big CROSS JOIN query is pushed down +EXPLAIN (VERBOSE, COSTS OFF) SELECT count(*) FROM ft1 CROSS JOIN ft2 CROSS JOIN ft4 CROSS JOIN ft5; + QUERY PLAN +--------------------------------------------------------------------------------------------------------------------------------------------------------------------- + Foreign Scan + Output: (count(*)) + Relations: Aggregate on ((((public.ft1) INNER JOIN (public.ft2)) INNER JOIN (public.ft4)) INNER JOIN (public.ft5)) + Remote SQL: SELECT count(*) FROM ((("S 1"."T 1" r1 INNER JOIN "S 1"."T 1" r2 ON (TRUE)) INNER JOIN "S 1"."T 3" r4 ON (TRUE)) INNER JOIN "S 1"."T 4" r6 ON (TRUE)) +(4 rows) + +-- Make sure query cancellation works +SET statement_timeout = '10ms'; +select count(*) from ft1 CROSS JOIN ft2 CROSS JOIN ft4 CROSS JOIN ft5; -- this takes very long +ERROR: canceling statement due to statement timeout +RESET statement_timeout; -- ==================================================================== -- Check that userid to use when querying the remote table is correctly -- propagated into foreign rels present in subqueries under an UNION ALL diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql index e3d147de6d..2626e68cc6 100644 --- a/contrib/postgres_fdw/sql/postgres_fdw.sql +++ b/contrib/postgres_fdw/sql/postgres_fdw.sql @@ -737,6 +737,13 @@ SELECT t1.c1, t2.c2 FROM v4 t1 LEFT JOIN ft5 t2 ON (t1.c1 = t2.c1) ORDER BY t1.c SELECT t1.c1, t2.c2 FROM v4 t1 LEFT JOIN ft5 t2 ON (t1.c1 = t2.c1) ORDER BY t1.c1, t2.c1 OFFSET 10 LIMIT 10; ALTER VIEW v4 OWNER TO regress_view_owner; +-- Make sure this big CROSS JOIN query is pushed down +EXPLAIN (VERBOSE, COSTS OFF) SELECT count(*) FROM ft1 CROSS JOIN ft2 CROSS JOIN ft4 CROSS JOIN ft5; +-- Make sure query cancellation works +SET statement_timeout = '10ms'; +select count(*) from ft1 CROSS JOIN ft2 CROSS JOIN ft4 CROSS JOIN ft5; -- this takes very long +RESET statement_timeout; + -- ==================================================================== -- Check that userid to use when querying the remote table is correctly -- propagated into foreign rels present in subqueries under an UNION ALL -- 2.39.2 --hlk45oagubnrswfe-- ^ permalink raw reply [nested|flat] 24+ messages in thread
* [PATCH v39] postgres_fdw: Start using new libpq cancel APIs @ 2024-03-18 18:37 Jelte Fennema-Nio <[email protected]> 0 siblings, 0 replies; 24+ messages in thread From: Jelte Fennema-Nio @ 2024-03-18 18:37 UTC (permalink / raw) Commit 61461a300c1c introduced new functions to libpq for cancelling queries. This replaces the usage of the old ones in postgres_fdw. Author: Jelte Fennema-Nio <[email protected]> Discussion: https://postgr.es/m/CAGECzQT_VgOWWENUqvUV9xQmbaCyXjtRRAYO8W07oqashk_N+g@mail.gmail.com --- contrib/postgres_fdw/connection.c | 108 +++++++++++++++--- .../postgres_fdw/expected/postgres_fdw.out | 15 +++ contrib/postgres_fdw/sql/postgres_fdw.sql | 7 ++ 3 files changed, 113 insertions(+), 17 deletions(-) diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c index 4931ebf591..0c66eaa001 100644 --- a/contrib/postgres_fdw/connection.c +++ b/contrib/postgres_fdw/connection.c @@ -133,7 +133,7 @@ static void pgfdw_inval_callback(Datum arg, int cacheid, uint32 hashvalue); static void pgfdw_reject_incomplete_xact_state_change(ConnCacheEntry *entry); static void pgfdw_reset_xact_state(ConnCacheEntry *entry, bool toplevel); static bool pgfdw_cancel_query(PGconn *conn); -static bool pgfdw_cancel_query_begin(PGconn *conn); +static bool pgfdw_cancel_query_begin(PGconn *conn, TimestampTz endtime); static bool pgfdw_cancel_query_end(PGconn *conn, TimestampTz endtime, bool consume_input); static bool pgfdw_exec_cleanup_query(PGconn *conn, const char *query, @@ -1315,36 +1315,106 @@ pgfdw_cancel_query(PGconn *conn) endtime = TimestampTzPlusMilliseconds(GetCurrentTimestamp(), CONNECTION_CLEANUP_TIMEOUT); - if (!pgfdw_cancel_query_begin(conn)) + if (!pgfdw_cancel_query_begin(conn, endtime)) return false; return pgfdw_cancel_query_end(conn, endtime, false); } +/* + * Submit a cancel request to the given connection, waiting only until + * the given time. + * + * We sleep interruptibly until we receive confirmation that the cancel + * request has been accepted, and if it is, return true; if the timeout + * lapses without that, or the request fails for whatever reason, return + * false. + */ static bool -pgfdw_cancel_query_begin(PGconn *conn) +pgfdw_cancel_query_begin(PGconn *conn, TimestampTz endtime) { - PGcancel *cancel; - char errbuf[256]; + bool timed_out = false; + bool failed = false; + PGcancelConn *cancel_conn = PQcancelCreate(conn); - /* - * Issue cancel request. Unfortunately, there's no good way to limit the - * amount of time that we might block inside PQgetCancel(). - */ - if ((cancel = PQgetCancel(conn))) + if (!PQcancelStart(cancel_conn)) { - if (!PQcancel(cancel, errbuf, sizeof(errbuf))) + PG_TRY(); { ereport(WARNING, (errcode(ERRCODE_CONNECTION_FAILURE), errmsg("could not send cancel request: %s", - errbuf))); - PQfreeCancel(cancel); - return false; + pchomp(PQcancelErrorMessage(cancel_conn))))); } - PQfreeCancel(cancel); + PG_FINALLY(); + { + PQcancelFinish(cancel_conn); + } + PG_END_TRY(); + return false; } - return true; + /* In what follows, do not leak any PGcancelConn on an error. */ + PG_TRY(); + { + while (true) + { + PostgresPollingStatusType pollres = PQcancelPoll(cancel_conn); + TimestampTz now = GetCurrentTimestamp(); + long cur_timeout; + int waitEvents = WL_LATCH_SET | WL_TIMEOUT | WL_EXIT_ON_PM_DEATH; + + if (pollres == PGRES_POLLING_OK) + break; + + /* If timeout has expired, give up, else get sleep time. */ + cur_timeout = TimestampDifferenceMilliseconds(now, endtime); + if (cur_timeout <= 0) + { + timed_out = true; + failed = true; + goto exit; + } + + switch (pollres) + { + case PGRES_POLLING_READING: + waitEvents |= WL_SOCKET_READABLE; + break; + case PGRES_POLLING_WRITING: + waitEvents |= WL_SOCKET_WRITEABLE; + break; + default: + failed = true; + goto exit; + } + + /* Sleep until there's something to do */ + WaitLatchOrSocket(MyLatch, waitEvents, PQcancelSocket(cancel_conn), + cur_timeout, PG_WAIT_EXTENSION); + ResetLatch(MyLatch); + + CHECK_FOR_INTERRUPTS(); + } +exit: + if (failed) + { + if (timed_out) + ereport(WARNING, + (errmsg("could not cancel request due to timeout"))); + else + ereport(WARNING, + (errcode(ERRCODE_CONNECTION_FAILURE), + errmsg("could not send cancel request: %s", + pchomp(PQcancelErrorMessage(cancel_conn))))); + } + } + PG_FINALLY(); + { + PQcancelFinish(cancel_conn); + } + PG_END_TRY(); + + return !failed; } static bool @@ -1685,7 +1755,11 @@ pgfdw_abort_cleanup_begin(ConnCacheEntry *entry, bool toplevel, */ if (PQtransactionStatus(entry->conn) == PQTRANS_ACTIVE) { - if (!pgfdw_cancel_query_begin(entry->conn)) + TimestampTz endtime; + + endtime = TimestampTzPlusMilliseconds(GetCurrentTimestamp(), + CONNECTION_CLEANUP_TIMEOUT); + if (!pgfdw_cancel_query_begin(entry->conn, endtime)) return false; /* Unable to cancel running query */ *cancel_requested = lappend(*cancel_requested, entry); } diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out index 58a603ac56..e03160bd97 100644 --- a/contrib/postgres_fdw/expected/postgres_fdw.out +++ b/contrib/postgres_fdw/expected/postgres_fdw.out @@ -2739,6 +2739,21 @@ SELECT t1.c1, t2.c2 FROM v4 t1 LEFT JOIN ft5 t2 ON (t1.c1 = t2.c1) ORDER BY t1.c (10 rows) ALTER VIEW v4 OWNER TO regress_view_owner; +-- Make sure this big CROSS JOIN query is pushed down +EXPLAIN (VERBOSE, COSTS OFF) SELECT count(*) FROM ft1 CROSS JOIN ft2 CROSS JOIN ft4 CROSS JOIN ft5; + QUERY PLAN +--------------------------------------------------------------------------------------------------------------------------------------------------------------------- + Foreign Scan + Output: (count(*)) + Relations: Aggregate on ((((public.ft1) INNER JOIN (public.ft2)) INNER JOIN (public.ft4)) INNER JOIN (public.ft5)) + Remote SQL: SELECT count(*) FROM ((("S 1"."T 1" r1 INNER JOIN "S 1"."T 1" r2 ON (TRUE)) INNER JOIN "S 1"."T 3" r4 ON (TRUE)) INNER JOIN "S 1"."T 4" r6 ON (TRUE)) +(4 rows) + +-- Make sure query cancellation works +SET statement_timeout = '10ms'; +select count(*) from ft1 CROSS JOIN ft2 CROSS JOIN ft4 CROSS JOIN ft5; -- this takes very long +ERROR: canceling statement due to statement timeout +RESET statement_timeout; -- ==================================================================== -- Check that userid to use when querying the remote table is correctly -- propagated into foreign rels present in subqueries under an UNION ALL diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql index e3d147de6d..2626e68cc6 100644 --- a/contrib/postgres_fdw/sql/postgres_fdw.sql +++ b/contrib/postgres_fdw/sql/postgres_fdw.sql @@ -737,6 +737,13 @@ SELECT t1.c1, t2.c2 FROM v4 t1 LEFT JOIN ft5 t2 ON (t1.c1 = t2.c1) ORDER BY t1.c SELECT t1.c1, t2.c2 FROM v4 t1 LEFT JOIN ft5 t2 ON (t1.c1 = t2.c1) ORDER BY t1.c1, t2.c1 OFFSET 10 LIMIT 10; ALTER VIEW v4 OWNER TO regress_view_owner; +-- Make sure this big CROSS JOIN query is pushed down +EXPLAIN (VERBOSE, COSTS OFF) SELECT count(*) FROM ft1 CROSS JOIN ft2 CROSS JOIN ft4 CROSS JOIN ft5; +-- Make sure query cancellation works +SET statement_timeout = '10ms'; +select count(*) from ft1 CROSS JOIN ft2 CROSS JOIN ft4 CROSS JOIN ft5; -- this takes very long +RESET statement_timeout; + -- ==================================================================== -- Check that userid to use when querying the remote table is correctly -- propagated into foreign rels present in subqueries under an UNION ALL -- 2.39.2 --hlk45oagubnrswfe-- ^ permalink raw reply [nested|flat] 24+ messages in thread
* [PATCH v39] postgres_fdw: Start using new libpq cancel APIs @ 2024-03-18 18:37 Jelte Fennema-Nio <[email protected]> 0 siblings, 0 replies; 24+ messages in thread From: Jelte Fennema-Nio @ 2024-03-18 18:37 UTC (permalink / raw) Commit 61461a300c1c introduced new functions to libpq for cancelling queries. This replaces the usage of the old ones in postgres_fdw. Author: Jelte Fennema-Nio <[email protected]> Discussion: https://postgr.es/m/CAGECzQT_VgOWWENUqvUV9xQmbaCyXjtRRAYO8W07oqashk_N+g@mail.gmail.com --- contrib/postgres_fdw/connection.c | 108 +++++++++++++++--- .../postgres_fdw/expected/postgres_fdw.out | 15 +++ contrib/postgres_fdw/sql/postgres_fdw.sql | 7 ++ 3 files changed, 113 insertions(+), 17 deletions(-) diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c index 4931ebf591..0c66eaa001 100644 --- a/contrib/postgres_fdw/connection.c +++ b/contrib/postgres_fdw/connection.c @@ -133,7 +133,7 @@ static void pgfdw_inval_callback(Datum arg, int cacheid, uint32 hashvalue); static void pgfdw_reject_incomplete_xact_state_change(ConnCacheEntry *entry); static void pgfdw_reset_xact_state(ConnCacheEntry *entry, bool toplevel); static bool pgfdw_cancel_query(PGconn *conn); -static bool pgfdw_cancel_query_begin(PGconn *conn); +static bool pgfdw_cancel_query_begin(PGconn *conn, TimestampTz endtime); static bool pgfdw_cancel_query_end(PGconn *conn, TimestampTz endtime, bool consume_input); static bool pgfdw_exec_cleanup_query(PGconn *conn, const char *query, @@ -1315,36 +1315,106 @@ pgfdw_cancel_query(PGconn *conn) endtime = TimestampTzPlusMilliseconds(GetCurrentTimestamp(), CONNECTION_CLEANUP_TIMEOUT); - if (!pgfdw_cancel_query_begin(conn)) + if (!pgfdw_cancel_query_begin(conn, endtime)) return false; return pgfdw_cancel_query_end(conn, endtime, false); } +/* + * Submit a cancel request to the given connection, waiting only until + * the given time. + * + * We sleep interruptibly until we receive confirmation that the cancel + * request has been accepted, and if it is, return true; if the timeout + * lapses without that, or the request fails for whatever reason, return + * false. + */ static bool -pgfdw_cancel_query_begin(PGconn *conn) +pgfdw_cancel_query_begin(PGconn *conn, TimestampTz endtime) { - PGcancel *cancel; - char errbuf[256]; + bool timed_out = false; + bool failed = false; + PGcancelConn *cancel_conn = PQcancelCreate(conn); - /* - * Issue cancel request. Unfortunately, there's no good way to limit the - * amount of time that we might block inside PQgetCancel(). - */ - if ((cancel = PQgetCancel(conn))) + if (!PQcancelStart(cancel_conn)) { - if (!PQcancel(cancel, errbuf, sizeof(errbuf))) + PG_TRY(); { ereport(WARNING, (errcode(ERRCODE_CONNECTION_FAILURE), errmsg("could not send cancel request: %s", - errbuf))); - PQfreeCancel(cancel); - return false; + pchomp(PQcancelErrorMessage(cancel_conn))))); } - PQfreeCancel(cancel); + PG_FINALLY(); + { + PQcancelFinish(cancel_conn); + } + PG_END_TRY(); + return false; } - return true; + /* In what follows, do not leak any PGcancelConn on an error. */ + PG_TRY(); + { + while (true) + { + PostgresPollingStatusType pollres = PQcancelPoll(cancel_conn); + TimestampTz now = GetCurrentTimestamp(); + long cur_timeout; + int waitEvents = WL_LATCH_SET | WL_TIMEOUT | WL_EXIT_ON_PM_DEATH; + + if (pollres == PGRES_POLLING_OK) + break; + + /* If timeout has expired, give up, else get sleep time. */ + cur_timeout = TimestampDifferenceMilliseconds(now, endtime); + if (cur_timeout <= 0) + { + timed_out = true; + failed = true; + goto exit; + } + + switch (pollres) + { + case PGRES_POLLING_READING: + waitEvents |= WL_SOCKET_READABLE; + break; + case PGRES_POLLING_WRITING: + waitEvents |= WL_SOCKET_WRITEABLE; + break; + default: + failed = true; + goto exit; + } + + /* Sleep until there's something to do */ + WaitLatchOrSocket(MyLatch, waitEvents, PQcancelSocket(cancel_conn), + cur_timeout, PG_WAIT_EXTENSION); + ResetLatch(MyLatch); + + CHECK_FOR_INTERRUPTS(); + } +exit: + if (failed) + { + if (timed_out) + ereport(WARNING, + (errmsg("could not cancel request due to timeout"))); + else + ereport(WARNING, + (errcode(ERRCODE_CONNECTION_FAILURE), + errmsg("could not send cancel request: %s", + pchomp(PQcancelErrorMessage(cancel_conn))))); + } + } + PG_FINALLY(); + { + PQcancelFinish(cancel_conn); + } + PG_END_TRY(); + + return !failed; } static bool @@ -1685,7 +1755,11 @@ pgfdw_abort_cleanup_begin(ConnCacheEntry *entry, bool toplevel, */ if (PQtransactionStatus(entry->conn) == PQTRANS_ACTIVE) { - if (!pgfdw_cancel_query_begin(entry->conn)) + TimestampTz endtime; + + endtime = TimestampTzPlusMilliseconds(GetCurrentTimestamp(), + CONNECTION_CLEANUP_TIMEOUT); + if (!pgfdw_cancel_query_begin(entry->conn, endtime)) return false; /* Unable to cancel running query */ *cancel_requested = lappend(*cancel_requested, entry); } diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out index 58a603ac56..e03160bd97 100644 --- a/contrib/postgres_fdw/expected/postgres_fdw.out +++ b/contrib/postgres_fdw/expected/postgres_fdw.out @@ -2739,6 +2739,21 @@ SELECT t1.c1, t2.c2 FROM v4 t1 LEFT JOIN ft5 t2 ON (t1.c1 = t2.c1) ORDER BY t1.c (10 rows) ALTER VIEW v4 OWNER TO regress_view_owner; +-- Make sure this big CROSS JOIN query is pushed down +EXPLAIN (VERBOSE, COSTS OFF) SELECT count(*) FROM ft1 CROSS JOIN ft2 CROSS JOIN ft4 CROSS JOIN ft5; + QUERY PLAN +--------------------------------------------------------------------------------------------------------------------------------------------------------------------- + Foreign Scan + Output: (count(*)) + Relations: Aggregate on ((((public.ft1) INNER JOIN (public.ft2)) INNER JOIN (public.ft4)) INNER JOIN (public.ft5)) + Remote SQL: SELECT count(*) FROM ((("S 1"."T 1" r1 INNER JOIN "S 1"."T 1" r2 ON (TRUE)) INNER JOIN "S 1"."T 3" r4 ON (TRUE)) INNER JOIN "S 1"."T 4" r6 ON (TRUE)) +(4 rows) + +-- Make sure query cancellation works +SET statement_timeout = '10ms'; +select count(*) from ft1 CROSS JOIN ft2 CROSS JOIN ft4 CROSS JOIN ft5; -- this takes very long +ERROR: canceling statement due to statement timeout +RESET statement_timeout; -- ==================================================================== -- Check that userid to use when querying the remote table is correctly -- propagated into foreign rels present in subqueries under an UNION ALL diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql index e3d147de6d..2626e68cc6 100644 --- a/contrib/postgres_fdw/sql/postgres_fdw.sql +++ b/contrib/postgres_fdw/sql/postgres_fdw.sql @@ -737,6 +737,13 @@ SELECT t1.c1, t2.c2 FROM v4 t1 LEFT JOIN ft5 t2 ON (t1.c1 = t2.c1) ORDER BY t1.c SELECT t1.c1, t2.c2 FROM v4 t1 LEFT JOIN ft5 t2 ON (t1.c1 = t2.c1) ORDER BY t1.c1, t2.c1 OFFSET 10 LIMIT 10; ALTER VIEW v4 OWNER TO regress_view_owner; +-- Make sure this big CROSS JOIN query is pushed down +EXPLAIN (VERBOSE, COSTS OFF) SELECT count(*) FROM ft1 CROSS JOIN ft2 CROSS JOIN ft4 CROSS JOIN ft5; +-- Make sure query cancellation works +SET statement_timeout = '10ms'; +select count(*) from ft1 CROSS JOIN ft2 CROSS JOIN ft4 CROSS JOIN ft5; -- this takes very long +RESET statement_timeout; + -- ==================================================================== -- Check that userid to use when querying the remote table is correctly -- propagated into foreign rels present in subqueries under an UNION ALL -- 2.39.2 --hlk45oagubnrswfe-- ^ permalink raw reply [nested|flat] 24+ messages in thread
end of thread, other threads:[~2024-03-18 18:37 UTC | newest] Thread overview: 24+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2023-04-27 16:13 Re: Possible regression setting GUCs on \connect Tom Lane <[email protected]> 2023-04-27 16:35 ` David Steele <[email protected]> 2023-04-27 18:16 ` Tom Lane <[email protected]> 2023-04-27 18:28 ` David Steele <[email protected]> 2023-04-27 18:43 ` Nathan Bossart <[email protected]> 2023-04-27 19:34 ` Tom Lane <[email protected]> 2023-04-27 19:22 ` Tom Lane <[email protected]> 2023-04-27 22:14 ` Nathan Bossart <[email protected]> 2023-04-27 22:38 ` Tom Lane <[email protected]> 2023-04-27 23:30 ` Alexander Korotkov <[email protected]> 2023-04-28 00:04 ` Alexander Korotkov <[email protected]> 2023-04-28 13:42 ` Jonathan S. Katz <[email protected]> 2023-05-17 06:47 ` Amit Kapila <[email protected]> 2023-05-17 11:56 ` Alexander Korotkov <[email protected]> 2023-05-17 12:08 ` Tom Lane <[email protected]> 2023-05-17 16:47 ` Nathan Bossart <[email protected]> 2023-05-17 16:57 ` Jonathan S. Katz <[email protected]> 2023-05-17 17:30 ` Alexander Korotkov <[email protected]> 2023-05-17 19:23 ` Jonathan S. Katz <[email protected]> 2023-05-18 15:47 ` Robert Haas <[email protected]> 2023-05-18 18:33 ` Tom Lane <[email protected]> 2024-03-18 18:37 [PATCH v39] postgres_fdw: Start using new libpq cancel APIs Jelte Fennema-Nio <[email protected]> 2024-03-18 18:37 [PATCH v39] postgres_fdw: Start using new libpq cancel APIs Jelte Fennema-Nio <[email protected]> 2024-03-18 18:37 [PATCH v39] postgres_fdw: Start using new libpq cancel APIs Jelte Fennema-Nio <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox