agora inbox for [email protected]help / color / mirror / Atom feed
\describe* 27+ messages / 16 participants [nested] [flat]
* \describe* @ 2018-01-26 01:11 Corey Huinker <[email protected]> 0 siblings, 3 replies; 27+ messages in thread From: Corey Huinker @ 2018-01-26 01:11 UTC (permalink / raw) To: pgsql-hackers <[email protected]> Some of the discussions about making psql more user friendly (more tab completions help, exit, etc) got me thinking about other ways that psql could be more friendly, and the one that comes to mind is our terse but cryptic \d* commands. I think it would be helpful and instructive to have corresponding long-form describe commands. Take describing schemas. Is \dn intuitive? Not really. In hindsight, you may think "yeah, a schema is a namespace", but you never guessed 'n' on the first try, or the second. Looking over exec_command_d() a bit, I think it's a bit of a stretch do have each command handle a long form like this: \describe table my_table or \describe table verbose my_table because then each \d-variant has to account for objects named "table" and "verbose" and that's a path to unhappiness. But if we dash-separated them, then all of the strcmps would be in the 'e' subsection, and each one would just have to know it's long to short translation, and call exec_command_d with the corresponding short command describe => d describe-verbose => d+ describe-aggregates-verbose => da+ describe-roles => du We could even presume the verbose flag in all cases (after all, the user was being verbose...), which would also cut down on tab-completion results, and we could check for interactive mode and display a message like \describe-schemas (short: \dn+) so that the person has the opportunity to learn the corresponding short command. In additional to aiding tab completion discovery of the commands (i.e. typing "\desc" and then hitting tab, it would also make scripts a little more self-documenting. Thoughts? ^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: \describe* @ 2018-01-26 01:58 Vik Fearing <[email protected]> parent: Corey Huinker <[email protected]> 2 siblings, 0 replies; 27+ messages in thread From: Vik Fearing @ 2018-01-26 01:58 UTC (permalink / raw) To: Corey Huinker <[email protected]>; pgsql-hackers <[email protected]> On 01/26/2018 02:11 AM, Corey Huinker wrote: > Some of the discussions about making psql more user friendly (more tab > completions help, exit, etc) got me thinking about other ways that psql > could be more friendly, and the one that comes to mind is our terse but > cryptic \d* commands. > > I think it would be helpful and instructive to have corresponding > long-form describe commands. > > Take describing schemas. Is \dn intuitive? Not really. In hindsight, you > may think "yeah, a schema is a namespace", but you never guessed 'n' on > the first try, or the second. At first blush, I support this idea. > Looking over exec_command_d() a bit, I think it's a bit of a stretch do > have each command handle a long form like this: > > \describe table my_table > or > \describe table verbose my_table > > because then each \d-variant has to account for objects named "table" > and "verbose" and that's a path to unhappiness. We're already being verbose so we can easily require \describe table table for the first case, and if you move "verbose" to before the object, then we can have \describe verbose table verbose So basically, the grammar would be "\describe [verbose] [system] object name" instead of "\dXS[+] name" where X is the object. One thing not addressed here is a long version of \ditvS+. Maybe something like \describe verbose system index, table, view <pattern> > But if we dash-separated them, then all of the strcmps would be in the > 'e' subsection, and each one would just have to know it's long to short > translation, and call exec_command_d with the corresponding short command > > describe => d > describe-verbose => d+ > describe-aggregates-verbose => da+ > describe-roles => du -1 > We could even presume the verbose flag in all cases (after all, the user > was being verbose...), which would also cut down on tab-completion > results, and we could check for interactive mode and display a message like > > \describe-schemas (short: \dn+) > > so that the person has the opportunity to learn the corresponding short > command. -1 on this, too. If we presume "verbose", we need to add a "terse". If the user is interested in the short forms, they can issue a \? like everybody else. > In additional to aiding tab completion discovery of the commands (i.e. > typing "\desc" and then hitting tab, it would also make scripts a little > more self-documenting. I always use long versions of options when writing scripts specifically because they are self-documenting (see 0be22457d7) so I certainly support this argument. Note: I am not volunteering to implement any of this, but I'll happily review it. -- Vik Fearing +33 6 46 75 15 36 http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support ^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: \describe* @ 2018-01-26 09:56 Laurenz Albe <[email protected]> parent: Corey Huinker <[email protected]> 2 siblings, 1 reply; 27+ messages in thread From: Laurenz Albe @ 2018-01-26 09:56 UTC (permalink / raw) To: Corey Huinker <[email protected]>; pgsql-hackers <[email protected]> Corey Huinker wrote: > Some of the discussions about making psql more user friendly (more tab completions help, exit, etc) got me thinking about other ways that psql could be more friendly, and the one that comes to mind is our terse but cryptic \d* commands. > > I think it would be helpful and instructive to have corresponding long-form describe commands. > > Take describing schemas. Is \dn intuitive? Not really. In hindsight, you may think "yeah, a schema is a namespace", but you never guessed 'n' on the first try, or the second. > > Looking over exec_command_d() a bit, I think it's a bit of a stretch do have each command handle a long form like this: > > \describe table my_table > or > \describe table verbose my_table > > because then each \d-variant has to account for objects named "table" and "verbose" and that's a path to unhappiness. > > But if we dash-separated them, then all of the strcmps would be in the 'e' subsection, and each one would just have to know it's long to short translation, and call exec_command_d with the corresponding short command > > describe => d > describe-verbose => d+ > describe-aggregates-verbose => da+ > describe-roles => du > > We could even presume the verbose flag in all cases (after all, the user was being verbose...), which would also cut down on tab-completion results, and we could check for interactive mode and display a message like > > \describe-schemas (short: \dn+) > > so that the person has the opportunity to learn the corresponding short command. > > In additional to aiding tab completion discovery of the commands (i.e. typing "\desc" and then hitting tab, it would also make scripts a little more self-documenting. > > Thoughts? I'm somewhat -1 on this. It would be about as hard to memorize \describe-schemas as it is to memorize \dn: You'd have to remember that it is "-" and not "_", that it is "describe", not "desc" and that it is "schemas", not "schema". Moreover, it would be as awkward to have \describe-schemas public as it would be to list all schemas with \describe-schema But my strongest criticism is that the \d* commands are for interactive use, and who wants to type in a long string like that? The beginner won't be able to guess the correct command, and the experienced user would refuse to use it. Having said all that, I can imagine that having \desc and \describe as an alternative to \d would help beginners who come e.g. from Oracle, but that would mean a change of the current behavior: test=> \describe List of foreign servers Name | Owner | Foreign-data wrapper --------+----------+---------------------- oracle | postgres | oracle_fdw (1 row) This is because \des lists foreign servers, and the rest of the command is ignored. Yours, Laurenz Albe ^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: \describe* @ 2018-01-26 14:49 David Fetter <[email protected]> parent: Corey Huinker <[email protected]> 2 siblings, 2 replies; 27+ messages in thread From: David Fetter @ 2018-01-26 14:49 UTC (permalink / raw) To: Corey Huinker <[email protected]>; +Cc: pgsql-hackers <[email protected]> On Thu, Jan 25, 2018 at 08:11:00PM -0500, Corey Huinker wrote: > Some of the discussions about making psql more user friendly (more > tab completions help, exit, etc) got me thinking about other ways > that psql could be more friendly, and the one that comes to mind is > our terse but cryptic \d* commands. They are indeed terse and cryptic, and what's worse, they're not available to clients other than psql, so I propose that we do what at least MySQL, Oracle, and DB2 do and implement DESCRIBE as its own command. Especially handy would be a variant DESCRIBE CREATE, which would do what it says on the label in a copy-and-paste-able form, but that's not strictly necessary for the first cut. Best, David. -- David Fetter <david(at)fetter(dot)org> http://fetter.org/ Phone: +1 415 235 3778 Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate ^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: \describe* @ 2018-01-26 14:54 Andreas Karlsson <[email protected]> parent: David Fetter <[email protected]> 1 sibling, 0 replies; 27+ messages in thread From: Andreas Karlsson @ 2018-01-26 14:54 UTC (permalink / raw) To: David Fetter <[email protected]>; Corey Huinker <[email protected]>; +Cc: pgsql-hackers <[email protected]> On 01/26/2018 03:49 PM, David Fetter wrote:> They are indeed terse and cryptic, and what's worse, they're not > available to clients other than psql, so I propose that we do what at > least MySQL, Oracle, and DB2 do and implement DESCRIBE as its own > command. > > Especially handy would be a variant DESCRIBE CREATE, which would do > what it says on the label in a copy-and-paste-able form, but that's > not strictly necessary for the first cut. I am not fan of this since I like how easy it is to explain to beginners that all backslash commands are processed by the client while everything else is handled by the server. Yes, "help" is an exception, but nobody really needs to know about that command. As for the actually proposal I do not care strongly either way. The \d commands are a bit cryptic and unfriendly to the occasional user, but I am not sure that having two ways to do it would be better. Andreas ^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: \describe* @ 2018-01-26 15:28 Vik Fearing <[email protected]> parent: David Fetter <[email protected]> 1 sibling, 1 reply; 27+ messages in thread From: Vik Fearing @ 2018-01-26 15:28 UTC (permalink / raw) To: David Fetter <[email protected]>; Corey Huinker <[email protected]>; +Cc: pgsql-hackers <[email protected]> On 01/26/2018 03:49 PM, David Fetter wrote: > I propose that we do what at least MySQL, Oracle, and DB2 do and > implement DESCRIBE as its own command. Hard pass. -- Vik Fearing +33 6 46 75 15 36 http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support ^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: \describe* @ 2018-01-26 18:57 Corey Huinker <[email protected]> parent: Laurenz Albe <[email protected]> 0 siblings, 0 replies; 27+ messages in thread From: Corey Huinker @ 2018-01-26 18:57 UTC (permalink / raw) To: Laurenz Albe <[email protected]>; +Cc: pgsql-hackers <[email protected]> > > It would be about as hard to memorize \describe-schemas as it is to > memorize \dn: > You'd have to remember that it is "-" and not "_", that it is "describe", > not "desc" > and that it is "schemas", not "schema". > You wouldn't memorize them. You'd discover them with tab completion. Type "\d<tab>" and you'll see \d \dA \dc \dd \ddp \des \deu \df \dFd \dFt \di \dL \dn \d0 \drds \dS \dT \dv \dy \da \db \dC \dD \dE \det \dew \dF \dFp \dg \dl \dm \do \dp \ds \dt \du \dx which is more heat than light. Yes, those are all the possibilites, but I, Joe Newguy, want to list schemas, and \ds and \dS look like the good guesses, neither of which is the right answer. If, with this feature, I typed \desc<tab>, I might see: \describe \describe-functions \describe-schemas \describe-tables ... So my voyage of discovery would have completed with having typed "\desc<tab>-sc<tab>" and if we add a note to interactive mode, I'd be shown the hint that \dn is the shortcut for that just above the list of schemas. ^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: \describe* @ 2018-01-27 16:39 David Fetter <[email protected]> parent: Vik Fearing <[email protected]> 0 siblings, 1 reply; 27+ messages in thread From: David Fetter @ 2018-01-27 16:39 UTC (permalink / raw) To: Vik Fearing <[email protected]>; +Cc: Corey Huinker <[email protected]>; pgsql-hackers <[email protected]> On Fri, Jan 26, 2018 at 04:28:24PM +0100, Vik Fearing wrote: > On 01/26/2018 03:49 PM, David Fetter wrote: > > I propose that we do what at least MySQL, Oracle, and DB2 do and > > implement DESCRIBE as its own command. > Hard pass. Would you be so kind as to expand on this? "Pass" might indicate a lack of interest in doing the work, but "hard pass" seems to indicate that you have reasons the work should not be done. Have I interpreted this correctly? Best, David. -- David Fetter <david(at)fetter(dot)org> http://fetter.org/ Phone: +1 415 235 3778 Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate ^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: \describe* @ 2018-01-27 21:54 Vik Fearing <[email protected]> parent: David Fetter <[email protected]> 0 siblings, 1 reply; 27+ messages in thread From: Vik Fearing @ 2018-01-27 21:54 UTC (permalink / raw) To: David Fetter <[email protected]>; +Cc: Corey Huinker <[email protected]>; pgsql-hackers <[email protected]> On 01/27/2018 05:39 PM, David Fetter wrote: > On Fri, Jan 26, 2018 at 04:28:24PM +0100, Vik Fearing wrote: >> On 01/26/2018 03:49 PM, David Fetter wrote: >>> I propose that we do what at least MySQL, Oracle, and DB2 do and >>> implement DESCRIBE as its own command. >> Hard pass. > > Would you be so kind as to expand on this? "Pass" might indicate a > lack of interest in doing the work, but "hard pass" seems to indicate > that you have reasons the work should not be done. Have I interpreted > this correctly? Andreas said it quite well. I don't like having client commands look like server commands. I don't mind exceptions for "help" and "quit", but I see no reason for anything more. -- Vik Fearing +33 6 46 75 15 36 http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support ^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: \describe* @ 2018-01-27 23:01 David Fetter <[email protected]> parent: Vik Fearing <[email protected]> 0 siblings, 1 reply; 27+ messages in thread From: David Fetter @ 2018-01-27 23:01 UTC (permalink / raw) To: Vik Fearing <[email protected]>; +Cc: Corey Huinker <[email protected]>; pgsql-hackers <[email protected]> On Sat, Jan 27, 2018 at 10:54:07PM +0100, Vik Fearing wrote: > On 01/27/2018 05:39 PM, David Fetter wrote: > > On Fri, Jan 26, 2018 at 04:28:24PM +0100, Vik Fearing wrote: > >> On 01/26/2018 03:49 PM, David Fetter wrote: > >>> I propose that we do what at least MySQL, Oracle, and DB2 do and > >>> implement DESCRIBE as its own command. > >> Hard pass. > > > > Would you be so kind as to expand on this? "Pass" might indicate a > > lack of interest in doing the work, but "hard pass" seems to indicate > > that you have reasons the work should not be done. Have I interpreted > > this correctly? > > Andreas said it quite well. I don't like having client commands look > like server commands. I don't mind exceptions for "help" and "quit", > but I see no reason for anything more. I did not propose a client command mimicking a server command. I thought I made that clear by mentioning that the \ commands are unavailable to clients other than psql, and offering an alternative. What I propose is in fact a server command, which at least three of the other popular RDBMSs already have. Best, David. -- David Fetter <david(at)fetter(dot)org> http://fetter.org/ Phone: +1 415 235 3778 Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate ^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: \describe* @ 2018-01-29 14:51 Ryan Murphy <[email protected]> parent: David Fetter <[email protected]> 0 siblings, 1 reply; 27+ messages in thread From: Ryan Murphy @ 2018-01-29 14:51 UTC (permalink / raw) To: David Fetter <[email protected]>; +Cc: Corey Huinker <[email protected]>; Vik Fearing <[email protected]>; pgsql-hackers <[email protected]> > > >What I propose is in fact a server command, >which at least three of > >the other popular RDBMSs already have. > Well to actually implement it, it would probably be a client command, because that's what \d* are. We would most likely want them implemented the same, to avoid needless complexity. I think people are more ok with \describe (with the backslash), which seems like what you're suggesting anyway. I read Vik's "hard pass" as being on having DESCRIBE which looks like an SQL command but would actually be implemented on the client. This seems simpler at first but could cause deep confusion later. But \describe gives a hint that it's different with the \, so it might be fine. Overall I agree with your idea: the \d* commands are cryptic and a longhand form would help people learning. Best, Ryan ^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: \describe* @ 2018-01-29 14:56 David Fetter <[email protected]> parent: Ryan Murphy <[email protected]> 0 siblings, 0 replies; 27+ messages in thread From: David Fetter @ 2018-01-29 14:56 UTC (permalink / raw) To: Ryan Murphy <[email protected]>; +Cc: Corey Huinker <[email protected]>; Vik Fearing <[email protected]>; pgsql-hackers <[email protected]> On Mon, Jan 29, 2018 at 02:51:53PM +0000, Ryan Murphy wrote: > > > > >What I propose is in fact a server command, >which at least three of > > >the other popular RDBMSs already have. > > > Well to actually implement it, it would probably be a client command, > because that's what \d* are. Why should this command be silo'ed off to the psql client? If it's a server command, it's available to all clients, not just psql. > We would most likely want them implemented the same, to avoid > needless complexity. We could certainly have \d call DESCRIBE for later versions of the server. \ commands which call different SQL depending on server version have long been a standard practice. > I think people are more ok with \describe (with the backslash), which seems > like what you're suggesting anyway. I read Vik's "hard pass" as being on > having DESCRIBE which looks like an SQL command but would actually be > implemented on the client. This seems simpler at first but could cause > deep confusion later. If we implement \d as DESCRIBE for server versions as of when DESCRIBE is actually implemented, we've got wins all around. Best, David. -- David Fetter <david(at)fetter(dot)org> http://fetter.org/ Phone: +1 415 235 3778 Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate ^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: Making sslrootcert=system work on Windows psql @ 2025-04-01 21:46 Jacob Champion <[email protected]> 1 sibling, 1 reply; 27+ messages in thread From: Jacob Champion @ 2025-04-01 21:46 UTC (permalink / raw) To: George MacKerron <[email protected]>; +Cc: pgsql-hackers On Tue, Apr 1, 2025 at 2:05 PM George MacKerron <[email protected]> wrote: > > I was very pleased to see the sslrootcert=system connection option added in Postgres 16 (I even blogged about it: https://neon.tech/blog/avoid-mitm-attacks-with-psql-postgres-16). But sslrootcert=system has not been widely supported by psql installations, perhaps because people compiling Postgres haven’t always been aware of the requirement to point OpenSSL in the direction of the system’s root CA certificates. > > I’ve recently been trying to get it more widely supported, with some success (details at end of this message). (Thank you!) > However, psql via the EnterpriseDB Windows installer still doesn’t support sslrootcert=system, Hm. I've been in contact with Kritika recently for the EDB macOS fixes; hopefully we can get something figured out for Windows too. > and I think a tiny patch is needed. The diff is attached, and can be seen in context here: https://github.com/postgres/postgres/compare/master...jawj:postgres:jawj-sslrootcert-system-windows > > Essentially, on Windows with OpenSSL 3.2+, it replaces SSL_CTX_set_default_verify_paths(SSL_context) with SSL_CTX_load_verify_store(SSL_context, "org.openssl.winstore:”). > > I’m not a Windows or OpenSSL expert, but so far the patched code seems to work in theory and in practice (sources below, and I’ve compiled and tested it working on Windows 11 x64). While this will get things working -- if you plan to use the Windows store! -- I worry that it's an incompatible change, and anyone who is actually happy with the way things currently work (i.e. not using the EDB installers) will be broken. The meaning of `sslrootcert=system` is "do whatever OpenSSL wants to do by default." That includes modification by the OpenSSL environment variables, which (I think) this patch disables. The winstore is new to me. Is there no way to get OpenSSL to switch its default store without code changes? --Jacob ^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: Making sslrootcert=system work on Windows psql @ 2025-04-02 07:32 Daniel Gustafsson <[email protected]> parent: Jacob Champion <[email protected]> 0 siblings, 1 reply; 27+ messages in thread From: Daniel Gustafsson @ 2025-04-02 07:32 UTC (permalink / raw) To: Jacob Champion <[email protected]>; +Cc: George MacKerron <[email protected]>; pgsql-hackers > On 1 Apr 2025, at 23:46, Jacob Champion <[email protected]> wrote: > On Tue, Apr 1, 2025 at 2:05 PM George MacKerron <[email protected]> wrote: >> I’ve recently been trying to get it more widely supported, with some success (details at end of this message). > > (Thank you!) +many >> I’m not a Windows or OpenSSL expert, but so far the patched code seems to work in theory and in practice (sources below, and I’ve compiled and tested it working on Windows 11 x64). > > While this will get things working -- if you plan to use the Windows > store! -- I worry that it's an incompatible change, and anyone who is > actually happy with the way things currently work (i.e. not using the > EDB installers) will be broken. The meaning of `sslrootcert=system` is > "do whatever OpenSSL wants to do by default." That includes > modification by the OpenSSL environment variables, which (I think) > this patch disables. Correct, this patch changes from using the defaults (directory, file and store) and thus the env variable overrides, to hardcoding the new winstore which came in 3.2 as the only option. While I agree that we probably should allow winstore (and other such stores for other platforms when/if they happen) I don't think making it the only option is the right way. > The winstore is new to me. Is there no way to get OpenSSL to switch > its default store without code changes? AFAIK one cannot change the default store in OpenSSL short of recompiling OpenSSL. -- Daniel Gustafsson ^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: Making sslrootcert=system work on Windows psql @ 2025-04-03 01:21 Jacob Champion <[email protected]> parent: Daniel Gustafsson <[email protected]> 0 siblings, 3 replies; 27+ messages in thread From: Jacob Champion @ 2025-04-03 01:21 UTC (permalink / raw) To: George MacKerron <[email protected]>; +Cc: Daniel Gustafsson <[email protected]>; pgsql-hackers On Wed, Apr 2, 2025 at 7:15 AM George MacKerron <[email protected]> wrote: > > But happily, I don’t think we need to choose. Can’t we just use the Windows system store if neither of the relevant environment variables is set? > > Thinking about this a little more, I guess the remaining concern is about people on Windows compiling their own psql from source, using an OpenSSL build that has a meaningful OPENSSLDIR baked in. Right. In a past life I shipped client stacks on Windows that looked kind of like that; I would have been less than happy if a client suddenly stopped using the certificate bundle I'd set up. > My preference would be for "org.openssl.winstore:" to be the compile-time default, though, because the option is called sslrootcert=system and it’s documented as using “the system’s trusted CA roots” (not sslrootcert=openssldir or documented as using OpenSSL’s default CA roots). If we'd decided to do that from the beginning, maybe... but it looks like the winstore URI wasn't released yet when we designed that, so "the system" couldn't have meant anything except for OpenSSL. Maybe the documentation needs to be more specific now that OpenSSL is supporting more stuff. Even if we want to change that definition sometime in the future, we'd still have to wait at least until OpenSSL 3.1 was no longer supported; I don't think it would be very helpful for our definition of "system" to change abruptly when upgrading OpenSSL past the 3.2 boundary. All this to say, I'd like to support the winstore, but I'm not convinced it should take over the existing meaning of "system". Even just adding it as a fallback has some risk to any packagers who have gotten it working. On Wed, Apr 2, 2025 at 12:33 AM Daniel Gustafsson <[email protected]> wrote: > AFAIK one cannot change the default store in OpenSSL short of recompiling > OpenSSL. I had hoped that a `system_default` entry in openssl.cnf would be able to override it, but no luck -- VerifyCAStore is explicitly forbidden in the default section :( Thanks, --Jacob ^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: Making sslrootcert=system work on Windows psql @ 2025-04-03 11:02 Sandeep Thakkar <[email protected]> 1 sibling, 0 replies; 27+ messages in thread From: Sandeep Thakkar @ 2025-04-03 11:02 UTC (permalink / raw) To: George MacKerron <[email protected]>; +Cc: pgsql-hackers On Wed, Apr 2, 2025 at 2:35 AM George MacKerron <[email protected]> wrote: > I was very pleased to see the sslrootcert=system connection option added > in Postgres 16 (I even blogged about it: > https://neon.tech/blog/avoid-mitm-attacks-with-psql-postgres-16). But > sslrootcert=system has not been widely supported by psql installations, > perhaps because people compiling Postgres haven’t always been aware of the > requirement to point OpenSSL in the direction of the system’s root CA > certificates. > > I’ve recently been trying to get it more widely supported, with some > success (details at end of this message). > > However, psql via the EnterpriseDB Windows installer still doesn’t support > sslrootcert=system, and I think a tiny patch is needed. The diff is > attached, and can be seen in context here: > https://github.com/postgres/postgres/compare/master...jawj:postgres:jawj-sslrootcert-system-windows > > Essentially, on Windows with OpenSSL 3.2+, it replaces > SSL_CTX_set_default_verify_paths(SSL_context) with > SSL_CTX_load_verify_store(SSL_context, "org.openssl.winstore:”). > > Please note the EDB Windows installers for PostgreSQL versions (upto v17) use OpenSSL v3.0, which is an LTS version. PostgreSQL 18 installer may use OpenSSL v3.5 depending on the release timeframe. > I’m not a Windows or OpenSSL expert, but so far the patched code seems to > work in theory and in practice (sources below, and I’ve compiled and tested > it working on Windows 11 x64). > > > # Sources > > https://stackoverflow.com/a/79461864/338196 > https://docs.openssl.org/master/man7/OSSL_STORE-winstore/ > https://docs.openssl.org/master/man3/SSL_CTX_load_verify_locations/ > > > # Status of sslrootcert=system in various packages providing psql > > ## Mac > Postgres.app — now fixed ( > https://github.com/PostgresApp/PostgresApp/issues/801) > MacPorts — now fixed (https://trac.macports.org/ticket/72080) > EDB installer — now fixed ( > https://github.com/EnterpriseDB/edb-installers/issues/264) > homebrew — was working already > > ## Linux > Debian/Ubuntu — now Recommends ca-certificates ( > https://salsa.debian.org/postgresql/postgresql/-/commit/96077ad61c36386646cdd9b5ce0e423a357ce73b > ) > > ## Windows > EDB installer — in progress > WSL1, WSL2 (Ubuntu, openSUSE) — was working already > > -- Sandeep Thakkar ^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: Making sslrootcert=system work on Windows psql @ 2025-04-03 13:28 Christoph Berg <[email protected]> parent: Jacob Champion <[email protected]> 2 siblings, 0 replies; 27+ messages in thread From: Christoph Berg @ 2025-04-03 13:28 UTC (permalink / raw) To: George MacKerron <[email protected]>; +Cc: Daniel Gustafsson <[email protected]>; Jacob Champion <[email protected]>; pgsql-hackers Re: George MacKerron > (3) Any other ideas? I'm not a fan of "security by adding more connection parameters". What are the chances of making "use the system/os default CA store" the default? "sslmode=require" would then already actually "require" a certificate if I'm reading the docs right. This would match user expectation for POLA. This default could then be pointed at the correct locations (plural) on all operating systems. (sslrootcert=system:wincert:otherlocation?) The "default default" would still be sslmode=prefer so it wouldn't break today's normal case. Users of sslmode=require will understand that supplying a CA certificate is no longer optional. Perhaps add a sslmode=require-weak could be added as a workaround. Christoph ^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: Making sslrootcert=system work on Windows psql @ 2025-04-04 08:39 Daniel Gustafsson <[email protected]> parent: Jacob Champion <[email protected]> 2 siblings, 0 replies; 27+ messages in thread From: Daniel Gustafsson @ 2025-04-04 08:39 UTC (permalink / raw) To: George MacKerron <[email protected]>; +Cc: Jacob Champion <[email protected]>; pgsql-hackers > On 3 Apr 2025, at 16:26, Daniel Gustafsson <[email protected]> wrote: >> On 3 Apr 2025, at 14:41, George MacKerron <[email protected]> wrote: >> Your diff certainly fixes (1b), so it’s definitely an improvement. > > Thanks, unless Jacob objects I propose to apply that backpatched down to when > sslrootcert=system went in. Hearing no objections I've committed this with a backpatch down to v16. -- Daniel Gustafsson ^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: Making sslrootcert=system work on Windows psql @ 2025-04-24 17:45 Jacob Champion <[email protected]> parent: Jacob Champion <[email protected]> 2 siblings, 0 replies; 27+ messages in thread From: Jacob Champion @ 2025-04-24 17:45 UTC (permalink / raw) To: George MacKerron <[email protected]>; +Cc: Daniel Gustafsson <[email protected]>; pgsql-hackers On Wed, Apr 23, 2025 at 8:47 AM George MacKerron <[email protected]> wrote: > I’d suggest two new special sslrootcert values: > > (1) sslrootcert=openssl > > This does exactly what sslrootcert=system does now, but is less confusingly named for Windows users. sslrootcert=system becomes a deprecated synonym for this option. Stealing the word "system" from the existing sslrootcert domain had at least two hazards: a) existing users might have a file named "system" that would now be ignored, and b) users might accidentally use sslrootcert=system on older versions of libpq, picking up an unexpected file named "system" and doing the Wrong Thing. Problem (a) can be worked around by saying "./system" instead, so honestly I wasn't too concerned about that, and I considered (b) to be more of a theoretical problem that was outweighed by the benefit of getting OpenSSL to just Do The Thing people wanted it to do. A couple years on, I think (b) is less theoretical than I had originally hoped. As evidence I point to Stack Overflow questions like [1], where both the asker and the answerer are a bit confused about how connection string versioning works. If we steal more words, I think that problem is going to get worse. So I'm leaning toward's Daniel's earlier position that sslrootcert has kind of run its course, and if you want to select OpenSSL stores, we need a more fully featured syntax and probably a completely new option to be able to pass that through safely. > (2) sslrootcert=os > > This does what I was proposing in my patch: it uses winstore on Windows and behaves the same as sslrootcert=openssl elsewhere, where openssl *is* the operating system SSL provider. Falling back to standard OpenSSL introduces the same hazard we're running into today, though -- what if someone creates a macstore [2] for OpenSSL, so that its behavior matches Safari's or whatever, and then everyone wonders why sslrootcert=os doesn't use that? If the abstraction must leak the details anyway, I think we should expose them directly instead. (As a small soapbox, I think "application-level" fallback for a trust chain is frequently going to lead to regret. You should ideally tell us what you want, and either get it or fail.) Thanks, --Jacob [1] https://stackoverflow.com/questions/77989772/psql-root-certificate-file-system-does-not-exist-why-ss... [2] https://github.com/openssl/openssl/issues/23460 ^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: pgsql: Ignore PlaceHolderVars when looking up statistics @ 2026-01-02 20:46 Robert Haas <[email protected]> 0 siblings, 1 reply; 27+ messages in thread From: Robert Haas @ 2026-01-02 20:46 UTC (permalink / raw) To: Richard Guo <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]> On Sun, Dec 28, 2025 at 9:45 PM Richard Guo <[email protected]> wrote: > Ignore PlaceHolderVars when looking up statistics > > Back-patch to v18. Although this issue exists before that, changes in > this version made it common enough to notice. Given the lack of field > reports for older versions, I am not back-patching further. Generally we don't back-patch fixes that could change plans, because it tends to produce user complaints. It's maybe more justifiable in this case because v18 is quite new and you didn't back-patch to older releases, but are we sure that it's warranted even there? -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: pgsql: Ignore PlaceHolderVars when looking up statistics @ 2026-01-02 21:20 Tom Lane <[email protected]> parent: Robert Haas <[email protected]> 0 siblings, 2 replies; 27+ messages in thread From: Tom Lane @ 2026-01-02 21:20 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: Richard Guo <[email protected]>; PostgreSQL Hackers <[email protected]> Robert Haas <[email protected]> writes: > On Sun, Dec 28, 2025 at 9:45 PM Richard Guo <[email protected]> wrote: >> Back-patch to v18. Although this issue exists before that, changes in >> this version made it common enough to notice. Given the lack of field >> reports for older versions, I am not back-patching further. > Generally we don't back-patch fixes that could change plans, because > it tends to produce user complaints. It's maybe more justifiable in > this case because v18 is quite new and you didn't back-patch to older > releases, but are we sure that it's warranted even there? It's a regression if we don't. See nearby complaint at https://www.postgresql.org/message-id/flat/b75866aa-bb54-456b-8f88-6b5bc52064ca%40app.fastmail.com That case was correctly handled in v17 and before. regards, tom lane ^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: pgsql: Ignore PlaceHolderVars when looking up statistics @ 2026-01-02 23:32 Robert Haas <[email protected]> parent: Tom Lane <[email protected]> 1 sibling, 0 replies; 27+ messages in thread From: Robert Haas @ 2026-01-02 23:32 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Richard Guo <[email protected]>; PostgreSQL Hackers <[email protected]> On Fri, Jan 2, 2026 at 4:20 PM Tom Lane <[email protected]> wrote: > It's a regression if we don't. See nearby complaint at > > https://www.postgresql.org/message-id/flat/b75866aa-bb54-456b-8f88-6b5bc52064ca%40app.fastmail.com > > That case was correctly handled in v17 and before. Oh, OK. Probably makes sense then, since v18 is new. -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: pgsql: Ignore PlaceHolderVars when looking up statistics @ 2026-01-05 00:52 Richard Guo <[email protected]> parent: Tom Lane <[email protected]> 1 sibling, 0 replies; 27+ messages in thread From: Richard Guo @ 2026-01-05 00:52 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Robert Haas <[email protected]>; Richard Guo <[email protected]>; PostgreSQL Hackers <[email protected]> On Sat, Jan 3, 2026 at 6:20 AM Tom Lane <[email protected]> wrote: > Robert Haas <[email protected]> writes: > > Generally we don't back-patch fixes that could change plans, because > > it tends to produce user complaints. It's maybe more justifiable in > > this case because v18 is quite new and you didn't back-patch to older > > releases, but are we sure that it's warranted even there? > It's a regression if we don't. That's right. I was actually of two minds for a while regarding the back-patch, but in the end decided to proceed. You can find the discussions and the reasoning behind this decision in the link below. https://postgr.es/m/CAMbWs49h5F66KZwLxaeXoLwHe_9jAB7Eu44UmJhhQpLA38tKhw@mail.gmail.com - Richard ^ permalink raw reply [nested|flat] 27+ messages in thread
* [PATCH v44b 4/4] hack deadlock detector @ 2026-03-25 19:35 Álvaro Herrera <[email protected]> 0 siblings, 0 replies; 27+ messages in thread From: Álvaro Herrera @ 2026-03-25 19:35 UTC (permalink / raw) --- src/backend/commands/cluster.c | 10 +++ src/backend/storage/lmgr/deadlock.c | 12 +++ src/include/storage/proc.h | 1 + src/test/modules/injection_points/Makefile | 1 + .../expected/repack_deadlock.out | 63 ++++++++++++++ src/test/modules/injection_points/meson.build | 1 + .../specs/repack_deadlock.spec | 83 +++++++++++++++++++ 7 files changed, 171 insertions(+) create mode 100644 src/test/modules/injection_points/expected/repack_deadlock.out create mode 100644 src/test/modules/injection_points/specs/repack_deadlock.spec diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c index 2c3058ba10d..d5b1dfbff69 100644 --- a/src/backend/commands/cluster.c +++ b/src/backend/commands/cluster.c @@ -372,6 +372,16 @@ ExecRepack(ParseState *pstate, RepackStmt *stmt, bool isTopLevel) /* Determine the lock mode to use. */ lockmode = RepackLockLevel((params.options & CLUOPT_CONCURRENT) != 0); + /* + * If in concurrent mode, set the PROC_IN_CONCURRENT_REPACK flag. This + * makes the deadlock checker cause anyone that would conflict with us + * to error out. It's important to set this flag ahead of actually locking + * the relation; it won't of course affect anyone until we do have a lock + * that others can conflict with. + */ + if ((params.options & CLUOPT_CONCURRENT) != 0) + MyProc->statusFlags |= PROC_IN_CONCURRENT_REPACK; + /* * If a single relation is specified, process it and we're done ... unless * the relation is a partitioned table, in which case we fall through. diff --git a/src/backend/storage/lmgr/deadlock.c b/src/backend/storage/lmgr/deadlock.c index b8962d875b6..10266e384aa 100644 --- a/src/backend/storage/lmgr/deadlock.c +++ b/src/backend/storage/lmgr/deadlock.c @@ -620,6 +620,18 @@ FindLockCycleRecurseMember(PGPROC *checkProc, proc->statusFlags & PROC_IS_AUTOVACUUM) blocking_autovacuum_proc = proc; + /* + * If we note that we're blocked by some process running + * REPACK (CONCURRENTLY), just fail. That process is + * going to upgrade its lock at some point, and it would + * be inappropriate for any other process to cause that + * to fail. + */ + if (checkProc == MyProc && + proc->statusFlags & PROC_IN_CONCURRENT_REPACK) + /* not a serious error message proposal */ + elog(ERROR, "oops, we're blocked by REPACK, time to die"); + /* We're done looking at this proclock */ break; } diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h index 1dad125706e..4e340ead455 100644 --- a/src/include/storage/proc.h +++ b/src/include/storage/proc.h @@ -69,6 +69,7 @@ struct XidCache #define PROC_AFFECTS_ALL_HORIZONS 0x20 /* this proc's xmin must be * included in vacuum horizons * in all databases */ +#define PROC_IN_CONCURRENT_REPACK 0x40 /* REPACK (CONCURRENTLY) */ /* flags reset at EOXact */ #define PROC_VACUUM_STATE_MASK \ diff --git a/src/test/modules/injection_points/Makefile b/src/test/modules/injection_points/Makefile index 2cd7d87c533..f7663859fe2 100644 --- a/src/test/modules/injection_points/Makefile +++ b/src/test/modules/injection_points/Makefile @@ -15,6 +15,7 @@ REGRESS_OPTS = --dlpath=$(top_builddir)/src/test/regress ISOLATION = basic \ inplace \ repack \ + repack_deadlock \ repack_toast \ syscache-update-pruned \ heap_lock_update diff --git a/src/test/modules/injection_points/expected/repack_deadlock.out b/src/test/modules/injection_points/expected/repack_deadlock.out new file mode 100644 index 00000000000..42505d10933 --- /dev/null +++ b/src/test/modules/injection_points/expected/repack_deadlock.out @@ -0,0 +1,63 @@ +Parsed test spec with 2 sessions + +starting permutation: wait_before_lock add_column wakeup_before_lock check1 +injection_points_attach +----------------------- + +(1 row) + +step wait_before_lock: + REPACK (CONCURRENTLY) repack_deadlock USING INDEX repack_deadlock_pkey; + <waiting ...> +step add_column: + alter table repack_deadlock add column noise text; + <waiting ...> +step add_column: <... completed> +ERROR: oops, we're blocked by REPACK, time to die +step wakeup_before_lock: + SELECT injection_points_wakeup('repack-concurrently-before-lock'); + +injection_points_wakeup +----------------------- + +(1 row) + +step wait_before_lock: <... completed> +step check1: + INSERT INTO relfilenodes(node) + SELECT relfilenode FROM pg_class WHERE relname='repack_deadlock'; + + SELECT count(DISTINCT node) FROM relfilenodes; + + SELECT i, j FROM repack_deadlock ORDER BY i, j; + + INSERT INTO data_s1(i, j) + SELECT i, j FROM repack_deadlock; + + SELECT count(*) + FROM data_s1 d1 FULL JOIN data_s2 d2 USING (i, j) + WHERE d1.i ISNULL OR d2.i ISNULL; + +count +----- + 1 +(1 row) + +i|j +-+- +1|1 +2|2 +3|3 +4|4 +(4 rows) + +count +----- + 4 +(1 row) + +injection_points_detach +----------------------- + +(1 row) + diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.build index a414abb924b..1cd88d6db65 100644 --- a/src/test/modules/injection_points/meson.build +++ b/src/test/modules/injection_points/meson.build @@ -46,6 +46,7 @@ tests += { 'basic', 'inplace', 'repack', + 'repack_deadlock', 'repack_toast', 'syscache-update-pruned', 'heap_lock_update', diff --git a/src/test/modules/injection_points/specs/repack_deadlock.spec b/src/test/modules/injection_points/specs/repack_deadlock.spec new file mode 100644 index 00000000000..9d23a6588c2 --- /dev/null +++ b/src/test/modules/injection_points/specs/repack_deadlock.spec @@ -0,0 +1,83 @@ +# Test REPACK with a concurrent transaction that would cause a deadlock +setup +{ + CREATE EXTENSION injection_points; + + CREATE TABLE repack_deadlock(i int PRIMARY KEY, j int); + INSERT INTO repack_deadlock(i, j) VALUES (1, 1), (2, 2), (3, 3), (4, 4); + + CREATE TABLE relfilenodes(node oid); + + CREATE TABLE data_s1(i int, j int); + CREATE TABLE data_s2(i int, j int); +} + +teardown +{ + DROP TABLE repack_deadlock; + DROP EXTENSION injection_points; + + DROP TABLE relfilenodes; + DROP TABLE data_s1; + DROP TABLE data_s2; +} + +session s1 +setup +{ + SELECT injection_points_set_local(); + SELECT injection_points_attach('repack-concurrently-before-lock', 'wait'); +} +# Perform the initial load and wait for s2 to do some data changes. +step wait_before_lock +{ + REPACK (CONCURRENTLY) repack_deadlock USING INDEX repack_deadlock_pkey; +} +# Check the table from the perspective of s1. +# +# Besides the contents, we also check that relfilenode has changed. + +# Have each session write the contents into a table and use FULL JOIN to check +# if the outputs are identical. +step check1 +{ + INSERT INTO relfilenodes(node) + SELECT relfilenode FROM pg_class WHERE relname='repack_deadlock'; + + SELECT count(DISTINCT node) FROM relfilenodes; + + SELECT i, j FROM repack_deadlock ORDER BY i, j; + + INSERT INTO data_s1(i, j) + SELECT i, j FROM repack_deadlock; + + SELECT count(*) + FROM data_s1 d1 FULL JOIN data_s2 d2 USING (i, j) + WHERE d1.i ISNULL OR d2.i ISNULL; +} +teardown +{ + SELECT injection_points_detach('repack-concurrently-before-lock'); +} + +session s2 +# Change the existing data. UPDATE changes both key and non-key columns. Also +# update one row twice to test whether tuple version generated by this session +# can be found. +step add_column +{ + alter table repack_deadlock add column noise text; +} + +step wakeup_before_lock +{ + SELECT injection_points_wakeup('repack-concurrently-before-lock'); +} + +# Test if data changes introduced while one session is performing REPACK +# CONCURRENTLY find their way into the table. +permutation + wait_before_lock + add_column + wakeup_before_lock + check1 -- 2.47.3 --fot5plqmb33ey33c-- ^ permalink raw reply [nested|flat] 27+ messages in thread
* [PATCH v44b 4/4] hack deadlock detector @ 2026-03-25 19:35 Álvaro Herrera <[email protected]> 0 siblings, 0 replies; 27+ messages in thread From: Álvaro Herrera @ 2026-03-25 19:35 UTC (permalink / raw) --- src/backend/commands/cluster.c | 10 +++ src/backend/storage/lmgr/deadlock.c | 12 +++ src/include/storage/proc.h | 1 + src/test/modules/injection_points/Makefile | 1 + .../expected/repack_deadlock.out | 63 ++++++++++++++ src/test/modules/injection_points/meson.build | 1 + .../specs/repack_deadlock.spec | 83 +++++++++++++++++++ 7 files changed, 171 insertions(+) create mode 100644 src/test/modules/injection_points/expected/repack_deadlock.out create mode 100644 src/test/modules/injection_points/specs/repack_deadlock.spec diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c index 2c3058ba10d..d5b1dfbff69 100644 --- a/src/backend/commands/cluster.c +++ b/src/backend/commands/cluster.c @@ -372,6 +372,16 @@ ExecRepack(ParseState *pstate, RepackStmt *stmt, bool isTopLevel) /* Determine the lock mode to use. */ lockmode = RepackLockLevel((params.options & CLUOPT_CONCURRENT) != 0); + /* + * If in concurrent mode, set the PROC_IN_CONCURRENT_REPACK flag. This + * makes the deadlock checker cause anyone that would conflict with us + * to error out. It's important to set this flag ahead of actually locking + * the relation; it won't of course affect anyone until we do have a lock + * that others can conflict with. + */ + if ((params.options & CLUOPT_CONCURRENT) != 0) + MyProc->statusFlags |= PROC_IN_CONCURRENT_REPACK; + /* * If a single relation is specified, process it and we're done ... unless * the relation is a partitioned table, in which case we fall through. diff --git a/src/backend/storage/lmgr/deadlock.c b/src/backend/storage/lmgr/deadlock.c index b8962d875b6..10266e384aa 100644 --- a/src/backend/storage/lmgr/deadlock.c +++ b/src/backend/storage/lmgr/deadlock.c @@ -620,6 +620,18 @@ FindLockCycleRecurseMember(PGPROC *checkProc, proc->statusFlags & PROC_IS_AUTOVACUUM) blocking_autovacuum_proc = proc; + /* + * If we note that we're blocked by some process running + * REPACK (CONCURRENTLY), just fail. That process is + * going to upgrade its lock at some point, and it would + * be inappropriate for any other process to cause that + * to fail. + */ + if (checkProc == MyProc && + proc->statusFlags & PROC_IN_CONCURRENT_REPACK) + /* not a serious error message proposal */ + elog(ERROR, "oops, we're blocked by REPACK, time to die"); + /* We're done looking at this proclock */ break; } diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h index 1dad125706e..4e340ead455 100644 --- a/src/include/storage/proc.h +++ b/src/include/storage/proc.h @@ -69,6 +69,7 @@ struct XidCache #define PROC_AFFECTS_ALL_HORIZONS 0x20 /* this proc's xmin must be * included in vacuum horizons * in all databases */ +#define PROC_IN_CONCURRENT_REPACK 0x40 /* REPACK (CONCURRENTLY) */ /* flags reset at EOXact */ #define PROC_VACUUM_STATE_MASK \ diff --git a/src/test/modules/injection_points/Makefile b/src/test/modules/injection_points/Makefile index 2cd7d87c533..f7663859fe2 100644 --- a/src/test/modules/injection_points/Makefile +++ b/src/test/modules/injection_points/Makefile @@ -15,6 +15,7 @@ REGRESS_OPTS = --dlpath=$(top_builddir)/src/test/regress ISOLATION = basic \ inplace \ repack \ + repack_deadlock \ repack_toast \ syscache-update-pruned \ heap_lock_update diff --git a/src/test/modules/injection_points/expected/repack_deadlock.out b/src/test/modules/injection_points/expected/repack_deadlock.out new file mode 100644 index 00000000000..42505d10933 --- /dev/null +++ b/src/test/modules/injection_points/expected/repack_deadlock.out @@ -0,0 +1,63 @@ +Parsed test spec with 2 sessions + +starting permutation: wait_before_lock add_column wakeup_before_lock check1 +injection_points_attach +----------------------- + +(1 row) + +step wait_before_lock: + REPACK (CONCURRENTLY) repack_deadlock USING INDEX repack_deadlock_pkey; + <waiting ...> +step add_column: + alter table repack_deadlock add column noise text; + <waiting ...> +step add_column: <... completed> +ERROR: oops, we're blocked by REPACK, time to die +step wakeup_before_lock: + SELECT injection_points_wakeup('repack-concurrently-before-lock'); + +injection_points_wakeup +----------------------- + +(1 row) + +step wait_before_lock: <... completed> +step check1: + INSERT INTO relfilenodes(node) + SELECT relfilenode FROM pg_class WHERE relname='repack_deadlock'; + + SELECT count(DISTINCT node) FROM relfilenodes; + + SELECT i, j FROM repack_deadlock ORDER BY i, j; + + INSERT INTO data_s1(i, j) + SELECT i, j FROM repack_deadlock; + + SELECT count(*) + FROM data_s1 d1 FULL JOIN data_s2 d2 USING (i, j) + WHERE d1.i ISNULL OR d2.i ISNULL; + +count +----- + 1 +(1 row) + +i|j +-+- +1|1 +2|2 +3|3 +4|4 +(4 rows) + +count +----- + 4 +(1 row) + +injection_points_detach +----------------------- + +(1 row) + diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.build index a414abb924b..1cd88d6db65 100644 --- a/src/test/modules/injection_points/meson.build +++ b/src/test/modules/injection_points/meson.build @@ -46,6 +46,7 @@ tests += { 'basic', 'inplace', 'repack', + 'repack_deadlock', 'repack_toast', 'syscache-update-pruned', 'heap_lock_update', diff --git a/src/test/modules/injection_points/specs/repack_deadlock.spec b/src/test/modules/injection_points/specs/repack_deadlock.spec new file mode 100644 index 00000000000..9d23a6588c2 --- /dev/null +++ b/src/test/modules/injection_points/specs/repack_deadlock.spec @@ -0,0 +1,83 @@ +# Test REPACK with a concurrent transaction that would cause a deadlock +setup +{ + CREATE EXTENSION injection_points; + + CREATE TABLE repack_deadlock(i int PRIMARY KEY, j int); + INSERT INTO repack_deadlock(i, j) VALUES (1, 1), (2, 2), (3, 3), (4, 4); + + CREATE TABLE relfilenodes(node oid); + + CREATE TABLE data_s1(i int, j int); + CREATE TABLE data_s2(i int, j int); +} + +teardown +{ + DROP TABLE repack_deadlock; + DROP EXTENSION injection_points; + + DROP TABLE relfilenodes; + DROP TABLE data_s1; + DROP TABLE data_s2; +} + +session s1 +setup +{ + SELECT injection_points_set_local(); + SELECT injection_points_attach('repack-concurrently-before-lock', 'wait'); +} +# Perform the initial load and wait for s2 to do some data changes. +step wait_before_lock +{ + REPACK (CONCURRENTLY) repack_deadlock USING INDEX repack_deadlock_pkey; +} +# Check the table from the perspective of s1. +# +# Besides the contents, we also check that relfilenode has changed. + +# Have each session write the contents into a table and use FULL JOIN to check +# if the outputs are identical. +step check1 +{ + INSERT INTO relfilenodes(node) + SELECT relfilenode FROM pg_class WHERE relname='repack_deadlock'; + + SELECT count(DISTINCT node) FROM relfilenodes; + + SELECT i, j FROM repack_deadlock ORDER BY i, j; + + INSERT INTO data_s1(i, j) + SELECT i, j FROM repack_deadlock; + + SELECT count(*) + FROM data_s1 d1 FULL JOIN data_s2 d2 USING (i, j) + WHERE d1.i ISNULL OR d2.i ISNULL; +} +teardown +{ + SELECT injection_points_detach('repack-concurrently-before-lock'); +} + +session s2 +# Change the existing data. UPDATE changes both key and non-key columns. Also +# update one row twice to test whether tuple version generated by this session +# can be found. +step add_column +{ + alter table repack_deadlock add column noise text; +} + +step wakeup_before_lock +{ + SELECT injection_points_wakeup('repack-concurrently-before-lock'); +} + +# Test if data changes introduced while one session is performing REPACK +# CONCURRENTLY find their way into the table. +permutation + wait_before_lock + add_column + wakeup_before_lock + check1 -- 2.47.3 --fot5plqmb33ey33c-- ^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: xact_rollback spikes when logical walsender exits @ 2026-07-08 17:37 Nikolay Samokhvalov <[email protected]> 0 siblings, 1 reply; 27+ messages in thread From: Nikolay Samokhvalov @ 2026-07-08 17:37 UTC (permalink / raw) To: Fujii Masao <[email protected]>; +Cc: pgsql-hackers <[email protected]>; Rafael Thofehrn Castro <[email protected]> Fujii-san, coming back to your questions in this thread, I should have addressed them before sending v2 -- apologies for jumping past them. On Mon, Apr 20, 2026 at 9:35 AM Fujii Masao <[email protected]> wrote: > Thanks for the report and patch! > > How to implement a solution depends on what xact_rollback in > pg_stat_database > is intended to mean. So at first we should consider which rollbacks should > it count? The documentation does not currently give an explicit definition. > > At present, xact_rollback appears to count all rollbacks, explicit or > implicit, > by any process connected to the database, including regular backends, > autovacuum workers, and logical walsenders. If that is the intended > definition, > then rollbacks implicitly performed by logical walsenders during logical > replication should also be counted. Of course, even if we keep that > definition, > the sudden increase in xact_rollback might still be a problem, so we might > need to call pgstat_report_stat() immediately after pgstat_flush_io() in > walsender, so the counters continue to be updated periodically during > logical replication. > > On the other hand, your patch seems to assume a different definition: that > xact_rollback should count all explicit and implicit rollbacks, except > those > performed by logical walsenders during logical replication. That would be > one possible approach, although it seems a bit odd to exclude only one > subset > of rollbacks. > > A third option would be to define xact_rollback more narrowly, counting > only > rollbacks by regular backends, and excluding rollbacks by processes such as > autovacuum or walsender. At least in my view, xact_commit and xact_rollback > in pg_stat_database are typically used by DBAs to check whether > client transactions are committing or rolling back as expected. From > that perspective, it seems intuitive for xact_rollback to count only > rollbacks > by regular backends. But others may reasonably see it differently. > On the definitional question: I'd argue the walsender case isn't a "subset of rollbacks being excluded". The transactions being decoded actually committed; the abort in ReorderBufferProcessTXN() is internal cleanup of the catalog-snapshot transaction, not a transaction outcome. Notably, xact_commit is not incremented for these, so on any publisher the commit/rollback pair becomes internally inconsistent in proportion to decoded throughput. So counting these as rollbacks seems like a miscount under any of the three definitions. On option 1 (periodic pgstat_report_stat() in walsender): that would trade the exit spike for a steady inflated rollback rate, which seems worse for the metric's usefulness. Periodic flushing may still be worth doing independently for other stats; happy to send that as a separate patch. On option 3: I agree this matches how DBAs actually use these counters, and I'd support it. Since it changes observable behavior for autovacuum etc., it seems like master-only material, together with explicitly documenting the definition. Concretely, I propose splitting: 1) 0001 (v3, attached): narrow fix for the walsender miscount, intended as a back-patchable bug fix -- this is what's paging people in production today. Rebased on current master; the TAP test now also verifies that xact_commit does not change as a function of decoded transactions (walsender shutdown has a small fixed bookkeeping delta, handled via a control run). 2) 0002 (attached, draft): master-only implementation of option 3 -- count xact_commit/xact_rollback only for regular client backends -- plus a doc change defining both columns explicitly. It also removes the now-redundant parallel argument from AtEOXact_PgStat() and AtEOXact_PgStat_Database(), since parallel workers are background workers and are excluded by the backend-type check. 0002 has no test yet; if the definition is agreed on, I'll extend the new TAP test to cover the autovacuum/walsender exclusion. One consequence of option 3 worth stating explicitly: logical replication apply workers are background workers, so transactions they replay on a subscriber would no longer be counted in the subscriber's xact_commit. That's arguably correct under the "client transactions" definition, but it does change what subscriber-side monitoring sees, so I wanted to flag it rather than have it discovered later. Added to the July commitfest: https://commitfest.postgresql.org/patch/6992/ Looking forward to your thoughts. Nik Attachments: [application/octet-stream] v3-0002-count-client-xacts-only.patch (8.0K, ../../CAM527d8Evz3BCffJeL=1RJi-7jjPYbKz_ZoUxm2Go2i-_N+g6w@mail.gmail.com/3-v3-0002-count-client-xacts-only.patch) download | inline diff: From df6f10bd9ddf2b5659bf19d928763530a9af685c Mon Sep 17 00:00:00 2001 From: Nikolay Samokhvalov <[email protected]> Date: Wed, 8 Jul 2026 08:04:58 -0700 Subject: [PATCH v3 2/2] Count xact_commit/xact_rollback only for client backends pg_stat_database.xact_commit and xact_rollback have historically counted every top-level transaction end in a database, including the internal transactions run by other server processes such as autovacuum workers and walsenders. Those transactions are not user-visible transaction outcomes, and counting them makes the ratio of commits to rollbacks harder to interpret for monitoring. Adopt the narrower definition discussed on the list: count a transaction in xact_commit/xact_rollback only when it is executed by a regular client backend (AmRegularBackendProcess()). The "parallel" argument, previously used to keep parallel workers out of these counters, is now redundant -- parallel workers are background workers and are excluded by the backend-type test -- so it is removed from both AtEOXact_PgStat() and AtEOXact_PgStat_Database(). This is a monitoring-visible behavior change beyond the server processes themselves: logical replication apply workers are background workers, so the transactions they replay on a subscriber are no longer counted in the subscriber's xact_commit. That is intended, and is called out here so the change in subscriber statistics is not mistaken for a regression. Because it changes long-standing counter semantics, this is master-only. The back-patchable fix for the specific logical-decoding case that prompted this is handled separately; this commit implements the general definition on top of it. Discussion: https://postgr.es/m/CAM527d_EbU5Li4a5FdKQjYsdF-4Lqr_i3jXmZOm7Wbb%3DQ2KzTw%40mail.gmail.com --- doc/src/sgml/monitoring.sgml | 12 ++++++++---- src/backend/access/transam/twophase.c | 2 +- src/backend/access/transam/xact.c | 4 ++-- src/backend/utils/activity/pgstat_database.c | 14 +++++++++++--- src/backend/utils/activity/pgstat_xact.c | 4 ++-- src/include/pgstat.h | 2 +- src/include/utils/pgstat_internal.h | 2 +- 7 files changed, 26 insertions(+), 14 deletions(-) diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml index 12b9ee20d4a..7e0a7550fa6 100644 --- a/doc/src/sgml/monitoring.sgml +++ b/doc/src/sgml/monitoring.sgml @@ -3895,8 +3895,10 @@ description | Waiting for a newly initialized WAL file to reach durable storage <structfield>xact_commit</structfield> <type>bigint</type> </para> <para> - Number of transactions in this database that have been - committed + Number of transactions in this database that have been committed by + regular client backends. Transactions performed by other server + processes, such as autovacuum workers, walsenders, and logical + replication apply workers, are not counted. </para></entry> </row> @@ -3905,8 +3907,10 @@ description | Waiting for a newly initialized WAL file to reach durable storage <structfield>xact_rollback</structfield> <type>bigint</type> </para> <para> - Number of transactions in this database that have been - rolled back + Number of transactions in this database that have been rolled back by + regular client backends. Transactions performed by other server + processes, such as autovacuum workers, walsenders, and logical + replication apply workers, are not counted. </para></entry> </row> diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c index 1035e8b3fc7..0408f8794b4 100644 --- a/src/backend/access/transam/twophase.c +++ b/src/backend/access/transam/twophase.c @@ -1676,7 +1676,7 @@ FinishPreparedTransaction(const char *gid, bool isCommit) LWLockRelease(TwoPhaseStateLock); /* Count the prepared xact as committed or aborted */ - AtEOXact_PgStat(isCommit, false); + AtEOXact_PgStat(isCommit); /* * And now we can clean up any files we may have left. diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index d74b231caf3..4e627e3306b 100644 --- a/src/backend/access/transam/xact.c +++ b/src/backend/access/transam/xact.c @@ -2515,7 +2515,7 @@ CommitTransaction(void) AtEOXact_ComboCid(); AtEOXact_HashTables(true); AtEOXact_RI(true); - AtEOXact_PgStat(true, is_parallel_worker); + AtEOXact_PgStat(true); AtEOXact_Snapshot(true, false); AtEOXact_ApplyLauncher(true); AtEOXact_LogicalRepWorkers(true); @@ -3042,7 +3042,7 @@ AbortTransaction(void) AtEOXact_ComboCid(); AtEOXact_HashTables(false); AtEOXact_RI(false); - AtEOXact_PgStat(false, is_parallel_worker); + AtEOXact_PgStat(false); AtEOXact_ApplyLauncher(false); AtEOXact_LogicalRepWorkers(false); AtEOXact_LogicalCtl(); diff --git a/src/backend/utils/activity/pgstat_database.c b/src/backend/utils/activity/pgstat_database.c index 7f3bc016593..efe546931e3 100644 --- a/src/backend/utils/activity/pgstat_database.c +++ b/src/backend/utils/activity/pgstat_database.c @@ -17,6 +17,7 @@ #include "postgres.h" +#include "miscadmin.h" #include "storage/standby.h" #include "utils/pgstat_internal.h" #include "utils/timestamp.h" @@ -292,10 +293,17 @@ pgstat_fetch_stat_dbentry(Oid dboid) } void -AtEOXact_PgStat_Database(bool isCommit, bool parallel) +AtEOXact_PgStat_Database(bool isCommit) { - /* Don't count parallel worker transaction stats */ - if (!parallel) + /* + * Only count transactions of regular client backends in + * xact_commit/xact_rollback. Transactions performed by other server + * processes -- autovacuum workers, walsenders, and, less obviously, logical + * replication apply workers -- are not user-visible transaction outcomes and + * would otherwise inflate these counters. (This subsumes the previous + * !parallel check, since parallel workers are bgworkers.) + */ + if (AmRegularBackendProcess()) { /* * Count transaction commit or abort. (We use counters, not just diff --git a/src/backend/utils/activity/pgstat_xact.c b/src/backend/utils/activity/pgstat_xact.c index d1be3733e4d..6c3944b5f22 100644 --- a/src/backend/utils/activity/pgstat_xact.c +++ b/src/backend/utils/activity/pgstat_xact.c @@ -38,7 +38,7 @@ static bool pgStatSkipXactCounters = false; * Called from access/transam/xact.c at top-level transaction commit/abort. */ void -AtEOXact_PgStat(bool isCommit, bool parallel) +AtEOXact_PgStat(bool isCommit) { PgStat_SubXactStatus *xact_state; bool skip_xact_counters = pgStatSkipXactCounters; @@ -49,7 +49,7 @@ AtEOXact_PgStat(bool isCommit, bool parallel) */ pgStatSkipXactCounters = false; if (!skip_xact_counters) - AtEOXact_PgStat_Database(isCommit, parallel); + AtEOXact_PgStat_Database(isCommit); /* handle transactional stats information */ xact_state = pgStatXactStack; diff --git a/src/include/pgstat.h b/src/include/pgstat.h index ff0a2e16467..4fa84061057 100644 --- a/src/include/pgstat.h +++ b/src/include/pgstat.h @@ -827,7 +827,7 @@ extern PgStat_StatSubEntry *pgstat_fetch_stat_subscription(Oid subid); * Functions in pgstat_xact.c */ -extern void AtEOXact_PgStat(bool isCommit, bool parallel); +extern void AtEOXact_PgStat(bool isCommit); extern void pgstat_suppress_xact_counters(void); extern void pgstat_clear_xact_counter_suppression(void); extern void AtEOSubXact_PgStat(bool isCommit, int nestDepth); diff --git a/src/include/utils/pgstat_internal.h b/src/include/utils/pgstat_internal.h index b3dc3ff7d8b..ad9bb649e34 100644 --- a/src/include/utils/pgstat_internal.h +++ b/src/include/utils/pgstat_internal.h @@ -737,7 +737,7 @@ extern void pgstat_checkpointer_snapshot_cb(void); extern void pgstat_report_disconnect(Oid dboid); extern void pgstat_update_dbstats(TimestampTz ts); -extern void AtEOXact_PgStat_Database(bool isCommit, bool parallel); +extern void AtEOXact_PgStat_Database(bool isCommit); extern PgStat_StatDBEntry *pgstat_prep_database_pending(Oid dboid); extern void pgstat_reset_database_timestamp(Oid dboid, TimestampTz ts); -- 2.50.1 (Apple Git-155) [application/octet-stream] v3-0001-xact_rollback-decoding-fix.patch (13.1K, ../../CAM527d8Evz3BCffJeL=1RJi-7jjPYbKz_ZoUxm2Go2i-_N+g6w@mail.gmail.com/4-v3-0001-xact_rollback-decoding-fix.patch) download | inline diff: From 69ffad6af7a8bbbabaf3030ca417463a96cfb0c8 Mon Sep 17 00:00:00 2001 From: Nikolay Samokhvalov <[email protected]> Date: Wed, 8 Jul 2026 06:11:24 +0000 Subject: [PATCH v3 1/2] Do not attribute logical decoding aborts to xact_rollback Logical decoding aborts the current transaction after decoding committed transactions to clean up catalog access and other transaction-local state. In a logical walsender this can be a top-level abort, so pg_stat_database.xact_rollback is incremented even though no user-visible transaction rolled back. Keep these internal cleanup aborts out of xact_rollback by adding AbortCurrentTransactionWithoutXactCounters(), a narrow wrapper around AbortCurrentTransaction() that suppresses only the next pg_stat_database xact_commit/xact_rollback counter update while preserving the rest of transaction cleanup. Add a TAP test that fails without the fix: five committed transactions decoded by a subscription produce a publisher xact_rollback delta of 5 when the walsender exits. With the fix, the xact_rollback delta remains 0. The test also verifies that decoded transactions do not affect xact_commit by comparing against a control walsender shutdown with no decoded transactions. Reported-by: Rafael Thofehrn Castro Discussion: https://postgr.es/m/CAM527d_EbU5Li4a5FdKQjYsdF-4Lqr_i3jXmZOm7Wbb%3DQ2KzTw%40mail.gmail.com --- src/backend/access/transam/xact.c | 23 ++++ .../replication/logical/reorderbuffer.c | 18 ++- src/backend/utils/activity/pgstat_xact.c | 32 ++++- src/include/access/xact.h | 1 + src/include/pgstat.h | 2 + src/test/subscription/meson.build | 1 + .../t/039_publisher_xact_rollback.pl | 124 ++++++++++++++++++ 7 files changed, 196 insertions(+), 5 deletions(-) create mode 100644 src/test/subscription/t/039_publisher_xact_rollback.pl diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index 3a89149016f..d74b231caf3 100644 --- a/src/backend/access/transam/xact.c +++ b/src/backend/access/transam/xact.c @@ -3512,6 +3512,29 @@ AbortCurrentTransaction(void) } } +/* + * AbortCurrentTransactionWithoutXactCounters + * + * Like AbortCurrentTransaction(), but don't count the abort in + * pg_stat_database.xact_rollback. For internal cleanup aborts that don't + * represent a user-visible transaction outcome. + */ +void +AbortCurrentTransactionWithoutXactCounters(void) +{ + pgstat_suppress_xact_counters(); + AbortCurrentTransaction(); + + /* + * AbortCurrentTransaction() is a no-op when already idle (e.g. an abort at + * TBLOCK_DEFAULT/TRANS_DEFAULT), in which case AtEOXact_PgStat() never runs + * to consume the flag. That can't happen at the current call sites, which + * always abort a live transaction, but clear it unconditionally anyway so + * set and clear stay paired defensively and the flag cannot leak. + */ + pgstat_clear_xact_counter_suppression(); +} + /* * AbortCurrentTransactionInternal - a function doing an iteration of work * regarding handling the current transaction abort. In the case of diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c index 059ed860314..49bb87ab1ec 100644 --- a/src/backend/replication/logical/reorderbuffer.c +++ b/src/backend/replication/logical/reorderbuffer.c @@ -2666,9 +2666,14 @@ ReorderBufferProcessTXN(ReorderBuffer *rb, ReorderBufferTXN *txn, * Aborting the current (sub-)transaction as a whole has the right * semantics. We want all locks acquired in here to be released, not * reassigned to the parent and we do not want any database access - * have persistent effects. + * have persistent effects. In the !using_subtxn case this is a + * top-level abort for internal cleanup; keep it out of + * pg_stat_database.xact_rollback. */ - AbortCurrentTransaction(); + if (using_subtxn) + AbortCurrentTransaction(); + else + AbortCurrentTransactionWithoutXactCounters(); /* make sure there's no cache pollution */ if (rbtxn_distr_inval_overflowed(txn)) @@ -2729,9 +2734,14 @@ ReorderBufferProcessTXN(ReorderBuffer *rb, ReorderBufferTXN *txn, /* * Force cache invalidation to happen outside of a valid transaction - * to prevent catalog access as we just caught an error. + * to prevent catalog access as we just caught an error. As above, + * keep the top-level internal cleanup abort out of + * pg_stat_database.xact_rollback. */ - AbortCurrentTransaction(); + if (using_subtxn) + AbortCurrentTransaction(); + else + AbortCurrentTransactionWithoutXactCounters(); /* make sure there's no cache pollution */ if (rbtxn_distr_inval_overflowed(txn)) diff --git a/src/backend/utils/activity/pgstat_xact.c b/src/backend/utils/activity/pgstat_xact.c index 3e1978775e1..d1be3733e4d 100644 --- a/src/backend/utils/activity/pgstat_xact.c +++ b/src/backend/utils/activity/pgstat_xact.c @@ -31,6 +31,7 @@ static void AtEOSubXact_PgStat_DroppedStats(PgStat_SubXactStatus *xact_state, bool isCommit, int nestDepth); static PgStat_SubXactStatus *pgStatXactStack = NULL; +static bool pgStatSkipXactCounters = false; /* @@ -40,8 +41,15 @@ void AtEOXact_PgStat(bool isCommit, bool parallel) { PgStat_SubXactStatus *xact_state; + bool skip_xact_counters = pgStatSkipXactCounters; - AtEOXact_PgStat_Database(isCommit, parallel); + /* + * Consume the suppression flag. Only the xact_commit/xact_rollback bump is + * skipped; transactional stats below must still be processed. + */ + pgStatSkipXactCounters = false; + if (!skip_xact_counters) + AtEOXact_PgStat_Database(isCommit, parallel); /* handle transactional stats information */ xact_state = pgStatXactStack; @@ -59,6 +67,28 @@ AtEOXact_PgStat(bool isCommit, bool parallel) pgstat_clear_snapshot(); } +/* + * Suppress the xact_commit/xact_rollback bump at the next top-level transaction + * end. For internal aborts that don't represent a user-visible transaction + * outcome; see AbortCurrentTransactionWithoutXactCounters(). + */ +void +pgstat_suppress_xact_counters(void) +{ + Assert(!pgStatSkipXactCounters); + pgStatSkipXactCounters = true; +} + +/* + * Clear the suppression flag set by pgstat_suppress_xact_counters(), in case + * the intervening abort never reached AtEOXact_PgStat() to consume it. + */ +void +pgstat_clear_xact_counter_suppression(void) +{ + pgStatSkipXactCounters = false; +} + /* * When committing, drop stats for objects dropped in the transaction. When * aborting, drop stats for objects created in the transaction. diff --git a/src/include/access/xact.h b/src/include/access/xact.h index a8cbdf247c8..9a22a2e9a67 100644 --- a/src/include/access/xact.h +++ b/src/include/access/xact.h @@ -467,6 +467,7 @@ extern void SaveTransactionCharacteristics(SavedTransactionCharacteristics *s); extern void RestoreTransactionCharacteristics(const SavedTransactionCharacteristics *s); extern void CommitTransactionCommand(void); extern void AbortCurrentTransaction(void); +extern void AbortCurrentTransactionWithoutXactCounters(void); extern void BeginTransactionBlock(void); extern bool EndTransactionBlock(bool chain); extern bool PrepareTransactionBlock(const char *gid); diff --git a/src/include/pgstat.h b/src/include/pgstat.h index 58a44857f13..ff0a2e16467 100644 --- a/src/include/pgstat.h +++ b/src/include/pgstat.h @@ -828,6 +828,8 @@ extern PgStat_StatSubEntry *pgstat_fetch_stat_subscription(Oid subid); */ extern void AtEOXact_PgStat(bool isCommit, bool parallel); +extern void pgstat_suppress_xact_counters(void); +extern void pgstat_clear_xact_counter_suppression(void); extern void AtEOSubXact_PgStat(bool isCommit, int nestDepth); extern void AtPrepare_PgStat(void); extern void PostPrepare_PgStat(void); diff --git a/src/test/subscription/meson.build b/src/test/subscription/meson.build index e71e95c6297..268fa8c3e9c 100644 --- a/src/test/subscription/meson.build +++ b/src/test/subscription/meson.build @@ -48,6 +48,7 @@ tests += { 't/036_sequences.pl', 't/037_except.pl', 't/038_walsnd_shutdown_timeout.pl', + 't/039_publisher_xact_rollback.pl', 't/100_bugs.pl', ], }, diff --git a/src/test/subscription/t/039_publisher_xact_rollback.pl b/src/test/subscription/t/039_publisher_xact_rollback.pl new file mode 100644 index 00000000000..c75634425d0 --- /dev/null +++ b/src/test/subscription/t/039_publisher_xact_rollback.pl @@ -0,0 +1,124 @@ +# Copyright (c) 2026, PostgreSQL Global Development Group + +# Check that pg_stat_database.xact_rollback on a logical-replication +# publisher is not inflated by the walsender's internal catalog-cleanup +# aborts. ReorderBufferProcessTXN() ends each decoded transaction with +# AbortCurrentTransaction(); in the walsender that is a top-level abort. +use strict; +use warnings FATAL => 'all'; +use PostgreSQL::Test::Cluster; +use PostgreSQL::Test::Utils; +use Test::More; + +sub get_xact_stats +{ + my ($node) = @_; + + return split /\|/, $node->safe_psql('template1', q{ + SELECT xact_commit, xact_rollback + FROM pg_stat_database + WHERE datname = 'postgres' + }); +} + +# Wait for this subscription's walsender to disappear from pg_stat_activity. +# +# A walsender registers its stats flush (pgstat_shutdown_hook) with +# before_shmem_exit() and clears its pg_stat_activity entry +# (pgstat_beshutdown_hook) with on_shmem_exit(). shmem_exit() runs all +# before_shmem_exit callbacks before any on_shmem_exit callback, so by the time +# the walsender is gone from pg_stat_activity its xact_commit/xact_rollback +# counts have already been flushed to shared stats. Observing its exit is +# therefore sufficient synchronization for reading the flushed counters. +sub wait_for_walsender_exit +{ + my ($node) = @_; + + $node->poll_query_until( + 'template1', q{ + SELECT count(*) = 0 FROM pg_stat_activity + WHERE backend_type = 'walsender' AND application_name = 's' + }) + or die 's walsender did not exit'; +} + +my $node_publisher = PostgreSQL::Test::Cluster->new('publisher'); +$node_publisher->init(allows_streaming => 'logical'); +# Autovacuum would commit on the postgres database and perturb the xact_commit +# delta this test compares between the control and decoding runs. +$node_publisher->append_conf('postgresql.conf', 'autovacuum = off'); +$node_publisher->start; + +my $node_subscriber = PostgreSQL::Test::Cluster->new('subscriber'); +$node_subscriber->init; +$node_subscriber->start; + +$node_publisher->safe_psql('postgres', + 'CREATE TABLE t (id int PRIMARY KEY)'); +$node_subscriber->safe_psql('postgres', + 'CREATE TABLE t (id int PRIMARY KEY)'); + +$node_publisher->safe_psql('postgres', 'CREATE PUBLICATION p FOR TABLE t'); + +my $publisher_connstr = $node_publisher->connstr . ' dbname=postgres'; +$node_subscriber->safe_psql('postgres', + "CREATE SUBSCRIPTION s CONNECTION '$publisher_connstr' PUBLICATION p"); + +$node_subscriber->wait_for_subscription_sync($node_publisher, 's'); + +# Measure a walsender shutdown without any decoded committed transactions. +# The absolute xact_commit delta is not asserted: walsender shutdown performs +# fixed session/transaction bookkeeping that may change independently of this +# test. The control run captures that non-decoding delta so the decoding run +# can assert it is unchanged by decoded transactions. +my ($control_base_commit, $control_base_rollback) = + get_xact_stats($node_publisher); + +$node_subscriber->safe_psql('postgres', 'ALTER SUBSCRIPTION s DISABLE'); +wait_for_walsender_exit($node_publisher); + +my ($control_final_commit, $control_final_rollback) = + get_xact_stats($node_publisher); + +my $control_commit_delta = $control_final_commit - $control_base_commit; +my $control_rollback_delta = $control_final_rollback - $control_base_rollback; + +$node_subscriber->safe_psql('postgres', 'ALTER SUBSCRIPTION s ENABLE'); + +# Five autocommit INSERTs: each becomes one decoded committed transaction on +# the walsender. Without the fix, those produce five spurious rollbacks after +# DISABLE. xact_commit may change due to fixed walsender bookkeeping, but it +# must not change as a function of decoded transactions. +my $n = 5; +$node_publisher->safe_psql('postgres', + join('', map { "INSERT INTO t VALUES ($_);\n" } 1 .. $n)); + +$node_publisher->wait_for_catchup('s'); + +# Baseline after catchup and before walsender exit, so the deltas measure only +# the stats flushed by this subscription's walsender during shutdown. +my ($base_commit, $base_rollback) = get_xact_stats($node_publisher); + +# Disabling the subscription terminates the walsender; its shutdown hook +# flushes pgstat counters to shared stats before it leaves pg_stat_activity. +$node_subscriber->safe_psql('postgres', 'ALTER SUBSCRIPTION s DISABLE'); +wait_for_walsender_exit($node_publisher); + +my ($final_commit, $final_rollback) = get_xact_stats($node_publisher); + +cmp_ok( + $control_rollback_delta, '==', 0, + 'walsender shutdown without decoded transactions does not inflate publisher xact_rollback' +); + +cmp_ok( + $final_rollback - $base_rollback, '==', 0, + 'walsender does not inflate publisher xact_rollback for decoded transactions' +); + +cmp_ok( + $final_commit - $base_commit, '==', $control_commit_delta, + 'walsender does not change publisher xact_commit as a function of decoded transactions' +); + +done_testing(); -- 2.50.1 (Apple Git-155) ^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: xact_rollback spikes when logical walsender exits @ 2026-07-09 04:56 jihyun bahn <[email protected]> parent: Nikolay Samokhvalov <[email protected]> 0 siblings, 0 replies; 27+ messages in thread From: jihyun bahn @ 2026-07-09 04:56 UTC (permalink / raw) To: [email protected]; +Cc: Nik Samokhvalov <[email protected]> The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: not tested Documentation: tested, passed Hi Nikolay, I tested the v3 patches on master (commit e01b23b) on Ubuntu 26.04, gcc 15.2.0, configured with --enable-cassert --enable-debug --enable-tap-tests. 0001: Applies cleanly and builds without new warnings. To verify that the new TAP test actually catches the bug, I first added only t/039_publisher_xact_rollback.pl on top of unpatched master: it fails as intended (the xact_rollback delta was 5, expected 0). With the full patch applied it passes 3/3. make check (245 tests) and the full subscription suite (40 files / 588 tests) pass with 0001 applied. I also reproduced the problem at a larger scale: 1000 autocommit INSERTs decoded by a subscription, then ALTER SUBSCRIPTION ... DISABLE. Publisher pg_stat_database (xact_commit, xact_rollback), read via template1 connections, autovacuum off: master 0001 before DISABLE 1013, 0 1013, 0 after DISABLE 1015, 1000 1015, 0 On unpatched master the walsender exit adds exactly one spurious rollback per decoded transaction; with 0001 the delta is 0 and every other counter is identical. Operationally this is exactly the false-positive alert pattern: the spike lands on routine actions (subscription disable, slot drop, restarts) while actual rollbacks are zero. Each decoded transaction was already counted once in xact_commit by the committing client backend, and the rollback that appears later belongs to the walsender's own internal catalog-access transaction -- so monitoring ends up seeing one commit plus one rollback for work that in fact committed cleanly. Since both option 2 and option 3 discussed upthread imply excluding these internal aborts, 0001 seems reasonable to proceed with as a narrow back-patchable fix while the wider definitional question is settled around 0002. 0002 (draft): Builds on top of 0001 and behaves as described; make installcheck-world also passes with both patches applied. I measured the subscriber-side visibility change the commit message calls out: between a baseline taken after initial table sync and a reading taken after catchup (before disabling the subscription), subscriber xact_commit moves by +1001 on master/0001 -- the 1000 replayed transactions plus one further commit from a non-client backend -- and by 0 with 0002. The subsequent ALTER SUBSCRIPTION ... DISABLE session adds +2 to xact_commit in all cases, as expected for a regular client backend. One more data point in favor of the narrower definition: on master/0001, disabling the subscription also adds +1 to subscriber xact_rollback. That appears to be maybe_reread_subscription(), which starts a transaction to reread pg_subscription, notices the subscription was disabled, and proc_exit()s from within that transaction, so the shutdown abort of an internal catalog-read transaction is counted -- the same pattern as the walsender case, on the subscriber side. 0002 removes that as well. As a DBA who watches these counters across a fleet: +1 to option 3. xact_commit/xact_rollback are read as "are application transactions healthy", and background-process noise makes the commit/rollback ratio harder to trust. One question on observability after 0002: subscriber xact_commit is today the only cheap proxy for logical-replication apply throughput in transactions. pg_stat_subscription_stats currently exposes error and conflict counts but no applied-commit count, and pg_stat_subscription is LSN-based. Would it make sense to point at an alternative in the docs, or could an apply-commit counter in pg_stat_subscription_stats be worth considering as a follow-up? If it helps, I would be happy to draft a TAP test for 0002 along the lines of 0001's test (client backend counted; autovacuum, walsender, and apply worker excluded). Tested-by: Jihyun Bahn <[email protected]> Best regards, Jihyun Bahn ^ permalink raw reply [nested|flat] 27+ messages in thread
end of thread, other threads:[~2026-07-09 04:56 UTC | newest] Thread overview: 27+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2018-01-26 01:11 \describe* Corey Huinker <[email protected]> 2018-01-26 01:58 ` Vik Fearing <[email protected]> 2018-01-26 09:56 ` Laurenz Albe <[email protected]> 2018-01-26 18:57 ` Corey Huinker <[email protected]> 2018-01-26 14:49 ` David Fetter <[email protected]> 2018-01-26 14:54 ` Andreas Karlsson <[email protected]> 2018-01-26 15:28 ` Vik Fearing <[email protected]> 2018-01-27 16:39 ` David Fetter <[email protected]> 2018-01-27 21:54 ` Vik Fearing <[email protected]> 2018-01-27 23:01 ` David Fetter <[email protected]> 2018-01-29 14:51 ` Ryan Murphy <[email protected]> 2018-01-29 14:56 ` David Fetter <[email protected]> 2025-04-01 21:46 ` Re: Making sslrootcert=system work on Windows psql Jacob Champion <[email protected]> 2025-04-02 07:32 ` Re: Making sslrootcert=system work on Windows psql Daniel Gustafsson <[email protected]> 2025-04-03 01:21 ` Re: Making sslrootcert=system work on Windows psql Jacob Champion <[email protected]> 2025-04-03 13:28 ` Re: Making sslrootcert=system work on Windows psql Christoph Berg <[email protected]> 2025-04-04 08:39 ` Re: Making sslrootcert=system work on Windows psql Daniel Gustafsson <[email protected]> 2025-04-24 17:45 ` Re: Making sslrootcert=system work on Windows psql Jacob Champion <[email protected]> 2025-04-03 11:02 ` Re: Making sslrootcert=system work on Windows psql Sandeep Thakkar <[email protected]> 2026-01-02 20:46 Re: pgsql: Ignore PlaceHolderVars when looking up statistics Robert Haas <[email protected]> 2026-01-02 21:20 ` Re: pgsql: Ignore PlaceHolderVars when looking up statistics Tom Lane <[email protected]> 2026-01-02 23:32 ` Re: pgsql: Ignore PlaceHolderVars when looking up statistics Robert Haas <[email protected]> 2026-01-05 00:52 ` Re: pgsql: Ignore PlaceHolderVars when looking up statistics Richard Guo <[email protected]> 2026-03-25 19:35 [PATCH v44b 4/4] hack deadlock detector Álvaro Herrera <[email protected]> 2026-03-25 19:35 [PATCH v44b 4/4] hack deadlock detector Álvaro Herrera <[email protected]> 2026-07-08 17:37 Re: xact_rollback spikes when logical walsender exits Nikolay Samokhvalov <[email protected]> 2026-07-09 04:56 ` Re: xact_rollback spikes when logical walsender exits jihyun bahn <[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