Re: pgsql: Implement pg_wal_replay_wait() stored procedure
Hi, Pavel! Thank you for your review. On Tue, Oct 22, 2024 at 4:30 PM Pavel Borisov wrote: > On Tue, 22 Oct 2024 at 13:26, Alexander Korotkov wrote: >> >> On Wed, Oct 16, 2024 at 11:20 PM Alexander Korotkov >> wrote: >> > >> > On Wed, Oct 16, 2024 at 10:35 PM Peter Eisentraut >> > wrote: >> > > On 02.09.24 01:55, Alexander Korotkov wrote: >> > > > On Mon, Sep 2, 2024 at 2:28 AM Michael Paquier >> > > > wrote: >> > > >> On Sun, Sep 01, 2024 at 10:35:27PM +0300, Alexander Korotkov wrote: >> > > >>> This path hasn't changes since the patch revision when it was a >> > > >>> utility command. I agree that this doesn't look like proper path for >> > > >>> stored procedure. But I don't think src/backend/utils/adt is >> > > >>> appropriate path either, because it's not really about data type. >> > > >>> pg_wal_replay_wait() looks a good neighbor for >> > > >>> pg_wal_replay_pause()/pg_wal_replay_resume() and other WAL-related >> > > >>> functions. So, what about moving it to src/backend/access/transam? >> > > >> >> > > >> Moving the new function to xlogfuncs.c while publishing >> > > >> WaitForLSNReplay() makes sense to me. >> > > > >> > > > Thank you for proposal. I like this. >> > > > >> > > > Could you, please, check the attached patch? >> > > >> > > We still have stuff in src/backend/commands/waitlsn.c that is nothing >> > > like a "command". You have moved some stuff elsewhere, but what are you >> > > planning to do with the rest? >> > >> > Thank you for spotting this another time. What about moving that >> > somewhere like src/backend/access/transam/xlogwait.c ? >> >> Implemented this as a separate patch (0001). Also rebased other >> pending patches on that. 0004 now revises header comment of >> xlogwait.c with new procedure signature. > > > I've looked at v5 of a patchset. > > 0001: > Looks completely ok. > > 0002: > > As stated in latch.c > > - WL_POSTMASTER_DEATH: Wait for postmaster to die > - WL_EXIT_ON_PM_DEATH: Exit immediately if the postmaster dies > > * wakeEvents must include either WL_EXIT_ON_PM_DEATH for automatic exit > * if the postmaster dies or WL_POSTMASTER_DEATH for a flag set in the > * return value if the postmaster dies > > It's not completely clear to me if these comments need some clarification > (not related to the patchset), or if we should look for WL_EXIT_ON_PM_DEATH > for immediately FATAL. Or waiting for postmaster to die on > WL_POSTMASTER_DEATH instead of just fatal immediately? As I get from the code, WL_EXIT_ON_PM_DEATH cause process to just proc_exit(1) without throwing FATAL. So, in the most of situations we do throw FATAL after seeing WL_POSTMASTER_DEATH event. So, it's reasonable to do the same here. But indeed, this is a question (not related to this patch) whether WL_EXIT_ON_PM_DEATH should cause process to throw FATAL. > 0003: > Besides refactor it looks that deleteLSNWaiter() is added in > WaitForLSNReplay. Maybe it's worth mentioning in the commit message. > Maybe refactoring for loop for assigning result variable and breaking a loop > instead of immediate return would look better and lead to natural call of > after the loop before returning. I don't think we would get much simplicity/readability by breaking loop instead of immediate return. However, I reflected the changes in the commit message. Also I reflected that we don't distinguish any more seeing !RecoveryInProgress() in different places. > 0004: > > Comment: > + * Waits until recovery replays the target LSN with optional timeout. Throw > + * an error on failure. > may need mentioning "Unless no_error provided throws an error on failure" Changed as you proposed. > Otherwise the patch looks good. Thanks! -- Regards, Alexander Korotkov Supabase v6-0001-Move-LSN-waiting-declarations-and-definitions-to-.patch Description: Binary data v6-0004-Add-no_error-argument-to-pg_wal_replay_wait.patch Description: Binary data v6-0002-Make-WaitForLSNReplay-issue-FATAL-on-postmaster-d.patch Description: Binary data v6-0003-Refactor-WaitForLSNReplay-to-return-the-result-of.patch Description: Binary data
pgsql: Add functions pg_set_attribute_stats() and pg_clear_attribute_st
Add functions pg_set_attribute_stats() and pg_clear_attribute_stats(). Enable manipulation of attribute statistics. Only superficial validation is performed, so it's possible to add nonsense, and it's up to the planner (or other users of statistics) to behave reasonably in that case. Bump catalog version. Author: Corey Huinker Discussion: https://postgr.es/m/CADkLM=eergzn7ecdpwfcptjkok9sxzek5pot4d94evtzsvj...@mail.gmail.com Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/ce207d2a79017713c0293bf70132dc6d41166b03 Modified Files -- doc/src/sgml/func.sgml | 72 +++ src/backend/catalog/system_functions.sql | 22 + src/backend/statistics/Makefile| 1 + src/backend/statistics/attribute_stats.c | 869 + src/backend/statistics/meson.build | 1 + src/backend/statistics/stat_utils.c| 73 +++ src/include/catalog/catversion.h | 2 +- src/include/catalog/pg_proc.dat| 14 + src/include/statistics/stat_utils.h| 7 + src/test/regress/expected/stats_import.out | 659 +- src/test/regress/sql/stats_import.sql | 545 ++ 11 files changed, 2263 insertions(+), 2 deletions(-)
pgsql: Change pg_*_relation_stats() functions to return type to void.
Change pg_*_relation_stats() functions to return type to void. These functions will either raise an ERROR or run to normal completion, so no return value is necessary. Bump catalog version. Author: Corey Huinker Discussion: https://postgr.es/m/CADkLM=cbf8rnphutyhfi3kyzb9bydgx57hwk9rz2yp7s+om...@mail.gmail.com Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/dbe6bd4343d8cdb1b3cf1f66d6f66dd876a6c09d Modified Files -- doc/src/sgml/func.sgml | 11 --- src/backend/catalog/system_functions.sql | 2 +- src/backend/statistics/relation_stats.c| 6 -- src/include/catalog/catversion.h | 2 +- src/include/catalog/pg_proc.dat| 4 ++-- src/test/regress/expected/stats_import.out | 16 6 files changed, 20 insertions(+), 21 deletions(-)
pgsql: ecpg: Fix out-of-bound read in DecodeDateTime()
ecpg: Fix out-of-bound read in DecodeDateTime() It was possible for the code to read out-of-bound data from the "day_tab" table with some crafted input data. Let's treat these as invalid input as the month number is incorrect. A test is added to test this case with a check on the errno returned by the decoding routine. A test close to the new one added in this commit was testing for a failure, but did not look at the errno generated, so let's use this commit to also change it, adding a check on the errno returned by DecodeDateTime(). Like the other test scripts, dt_test should likely be expanded to include more checks based on the errnos generated in these code paths. This is left as future work. This issue exists since 2e6f97560a83, so backpatch all the way down. Reported-by: Pavel Nekrasov Author: Bruce Momjian, Pavel Nekrasov Discussion: https://postgr.es/m/18614-6bbe001173523...@postgresql.org Backpatch-through: 12 Branch -- REL_17_STABLE Details --- https://git.postgresql.org/pg/commitdiff/2c37cb26f8af0aecb1b534585f3a0234f70e69ee Modified Files -- src/interfaces/ecpg/pgtypeslib/dt_common.c | 6 +- .../ecpg/test/expected/pgtypeslib-dt_test.c| 76 +++--- .../ecpg/test/expected/pgtypeslib-dt_test.stderr | 42 ++-- .../ecpg/test/expected/pgtypeslib-dt_test.stdout | 3 +- src/interfaces/ecpg/test/pgtypeslib/dt_test.pgc| 30 + 5 files changed, 109 insertions(+), 48 deletions(-)
pgsql: ecpg: Fix out-of-bound read in DecodeDateTime()
ecpg: Fix out-of-bound read in DecodeDateTime() It was possible for the code to read out-of-bound data from the "day_tab" table with some crafted input data. Let's treat these as invalid input as the month number is incorrect. A test is added to test this case with a check on the errno returned by the decoding routine. A test close to the new one added in this commit was testing for a failure, but did not look at the errno generated, so let's use this commit to also change it, adding a check on the errno returned by DecodeDateTime(). Like the other test scripts, dt_test should likely be expanded to include more checks based on the errnos generated in these code paths. This is left as future work. This issue exists since 2e6f97560a83, so backpatch all the way down. Reported-by: Pavel Nekrasov Author: Bruce Momjian, Pavel Nekrasov Discussion: https://postgr.es/m/18614-6bbe001173523...@postgresql.org Backpatch-through: 12 Branch -- REL_13_STABLE Details --- https://git.postgresql.org/pg/commitdiff/fcafbaadf7e507708d7257282e1ddcee1cc4c5eb Modified Files -- src/interfaces/ecpg/pgtypeslib/dt_common.c | 6 +- .../ecpg/test/expected/pgtypeslib-dt_test.c| 76 +++--- .../ecpg/test/expected/pgtypeslib-dt_test.stderr | 42 ++-- .../ecpg/test/expected/pgtypeslib-dt_test.stdout | 3 +- src/interfaces/ecpg/test/pgtypeslib/dt_test.pgc| 30 + 5 files changed, 109 insertions(+), 48 deletions(-)
pgsql: ecpg: Fix out-of-bound read in DecodeDateTime()
ecpg: Fix out-of-bound read in DecodeDateTime() It was possible for the code to read out-of-bound data from the "day_tab" table with some crafted input data. Let's treat these as invalid input as the month number is incorrect. A test is added to test this case with a check on the errno returned by the decoding routine. A test close to the new one added in this commit was testing for a failure, but did not look at the errno generated, so let's use this commit to also change it, adding a check on the errno returned by DecodeDateTime(). Like the other test scripts, dt_test should likely be expanded to include more checks based on the errnos generated in these code paths. This is left as future work. This issue exists since 2e6f97560a83, so backpatch all the way down. Reported-by: Pavel Nekrasov Author: Bruce Momjian, Pavel Nekrasov Discussion: https://postgr.es/m/18614-6bbe001173523...@postgresql.org Backpatch-through: 12 Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/a0bff38d133ac95fb56bbd868a128bba95ec46c9 Modified Files -- src/interfaces/ecpg/pgtypeslib/dt_common.c | 6 +- .../ecpg/test/expected/pgtypeslib-dt_test.c| 76 +++--- .../ecpg/test/expected/pgtypeslib-dt_test.stderr | 42 ++-- .../ecpg/test/expected/pgtypeslib-dt_test.stdout | 3 +- src/interfaces/ecpg/test/pgtypeslib/dt_test.pgc| 30 + 5 files changed, 109 insertions(+), 48 deletions(-)
pgsql: ecpg: Fix out-of-bound read in DecodeDateTime()
ecpg: Fix out-of-bound read in DecodeDateTime() It was possible for the code to read out-of-bound data from the "day_tab" table with some crafted input data. Let's treat these as invalid input as the month number is incorrect. A test is added to test this case with a check on the errno returned by the decoding routine. A test close to the new one added in this commit was testing for a failure, but did not look at the errno generated, so let's use this commit to also change it, adding a check on the errno returned by DecodeDateTime(). Like the other test scripts, dt_test should likely be expanded to include more checks based on the errnos generated in these code paths. This is left as future work. This issue exists since 2e6f97560a83, so backpatch all the way down. Reported-by: Pavel Nekrasov Author: Bruce Momjian, Pavel Nekrasov Discussion: https://postgr.es/m/18614-6bbe001173523...@postgresql.org Backpatch-through: 12 Branch -- REL_15_STABLE Details --- https://git.postgresql.org/pg/commitdiff/335501fb2b8007000d8c154628065355b5cd4366 Modified Files -- src/interfaces/ecpg/pgtypeslib/dt_common.c | 6 +- .../ecpg/test/expected/pgtypeslib-dt_test.c| 76 +++--- .../ecpg/test/expected/pgtypeslib-dt_test.stderr | 42 ++-- .../ecpg/test/expected/pgtypeslib-dt_test.stdout | 3 +- src/interfaces/ecpg/test/pgtypeslib/dt_test.pgc| 30 + 5 files changed, 109 insertions(+), 48 deletions(-)
pgsql: ecpg: Fix out-of-bound read in DecodeDateTime()
ecpg: Fix out-of-bound read in DecodeDateTime() It was possible for the code to read out-of-bound data from the "day_tab" table with some crafted input data. Let's treat these as invalid input as the month number is incorrect. A test is added to test this case with a check on the errno returned by the decoding routine. A test close to the new one added in this commit was testing for a failure, but did not look at the errno generated, so let's use this commit to also change it, adding a check on the errno returned by DecodeDateTime(). Like the other test scripts, dt_test should likely be expanded to include more checks based on the errnos generated in these code paths. This is left as future work. This issue exists since 2e6f97560a83, so backpatch all the way down. Reported-by: Pavel Nekrasov Author: Bruce Momjian, Pavel Nekrasov Discussion: https://postgr.es/m/18614-6bbe001173523...@postgresql.org Backpatch-through: 12 Branch -- REL_16_STABLE Details --- https://git.postgresql.org/pg/commitdiff/a1e613b81aa05b5e29dcb531016269b103208a7d Modified Files -- src/interfaces/ecpg/pgtypeslib/dt_common.c | 6 +- .../ecpg/test/expected/pgtypeslib-dt_test.c| 76 +++--- .../ecpg/test/expected/pgtypeslib-dt_test.stderr | 42 ++-- .../ecpg/test/expected/pgtypeslib-dt_test.stdout | 3 +- src/interfaces/ecpg/test/pgtypeslib/dt_test.pgc| 30 + 5 files changed, 109 insertions(+), 48 deletions(-)
pgsql: ecpg: Fix out-of-bound read in DecodeDateTime()
ecpg: Fix out-of-bound read in DecodeDateTime() It was possible for the code to read out-of-bound data from the "day_tab" table with some crafted input data. Let's treat these as invalid input as the month number is incorrect. A test is added to test this case with a check on the errno returned by the decoding routine. A test close to the new one added in this commit was testing for a failure, but did not look at the errno generated, so let's use this commit to also change it, adding a check on the errno returned by DecodeDateTime(). Like the other test scripts, dt_test should likely be expanded to include more checks based on the errnos generated in these code paths. This is left as future work. This issue exists since 2e6f97560a83, so backpatch all the way down. Reported-by: Pavel Nekrasov Author: Bruce Momjian, Pavel Nekrasov Discussion: https://postgr.es/m/18614-6bbe001173523...@postgresql.org Backpatch-through: 12 Branch -- REL_14_STABLE Details --- https://git.postgresql.org/pg/commitdiff/9a51d4af12a7add717f7f2e4b82985b3a7a092f4 Modified Files -- src/interfaces/ecpg/pgtypeslib/dt_common.c | 6 +- .../ecpg/test/expected/pgtypeslib-dt_test.c| 76 +++--- .../ecpg/test/expected/pgtypeslib-dt_test.stderr | 42 ++-- .../ecpg/test/expected/pgtypeslib-dt_test.stdout | 3 +- src/interfaces/ecpg/test/pgtypeslib/dt_test.pgc| 30 + 5 files changed, 109 insertions(+), 48 deletions(-)
pgsql: ecpg: Fix out-of-bound read in DecodeDateTime()
ecpg: Fix out-of-bound read in DecodeDateTime() It was possible for the code to read out-of-bound data from the "day_tab" table with some crafted input data. Let's treat these as invalid input as the month number is incorrect. A test is added to test this case with a check on the errno returned by the decoding routine. A test close to the new one added in this commit was testing for a failure, but did not look at the errno generated, so let's use this commit to also change it, adding a check on the errno returned by DecodeDateTime(). Like the other test scripts, dt_test should likely be expanded to include more checks based on the errnos generated in these code paths. This is left as future work. This issue exists since 2e6f97560a83, so backpatch all the way down. Reported-by: Pavel Nekrasov Author: Bruce Momjian, Pavel Nekrasov Discussion: https://postgr.es/m/18614-6bbe001173523...@postgresql.org Backpatch-through: 12 Branch -- REL_12_STABLE Details --- https://git.postgresql.org/pg/commitdiff/9ecfd8a48adf84ec5603a53a6e1a5add097ab56f Modified Files -- src/interfaces/ecpg/pgtypeslib/dt_common.c | 6 +- .../ecpg/test/expected/pgtypeslib-dt_test.c| 76 +++--- .../ecpg/test/expected/pgtypeslib-dt_test.stderr | 42 ++-- .../ecpg/test/expected/pgtypeslib-dt_test.stdout | 3 +- src/interfaces/ecpg/test/pgtypeslib/dt_test.pgc| 30 + 5 files changed, 109 insertions(+), 48 deletions(-)
pgsql: Improve parser's reporting of statement start locations.
Improve parser's reporting of statement start locations. Up to now, the parser's reporting of a statement's stmt_location included any preceding whitespace or comments. This isn't really desirable but was done to avoid accounting honestly for nonterminals that reduce to empty. It causes problems for pg_stat_statements, which partially compensates by manually stripping whitespace, but is not bright enough to strip /*-style comments. There will be more problems with an upcoming patch to improve reporting of errors in extension scripts, so it's time to do something about this. The thing we have to do to make it work right is to adjust YYLLOC_DEFAULT to scan the inputs of each production to find the first one that has a valid location (i.e., did not reduce to empty). In theory this adds a little bit of per-reduction overhead, but in practice it's negligible. I checked by measuring the time to run raw_parser() on the contents of information_schema.sql, and there was basically no change. Having done that, we can rely on any nonterminal that didn't reduce to completely empty to have a correct starting location, and we don't need the kluges the stmtmulti production formerly used. This should have a side benefit of allowing parse error reports to include an error position in some cases where they formerly failed to do so, due to trying to report the position of an empty nonterminal. I did not go looking for an example though. The one previously known case where that could happen (OptSchemaEltList) no longer needs the kluge it had; but I rather doubt that that was the only case. Discussion: https://postgr.es/m/zvv1clhnbjlcz...@msg.df7cb.de Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/14e5680eee19df8b41ef77330d0b3857f498e4f7 Modified Files -- contrib/pg_stat_statements/expected/select.out | 5 +- contrib/pg_stat_statements/sql/select.sql | 3 +- src/backend/nodes/queryjumblefuncs.c | 6 +++ src/backend/parser/gram.y | 66 +- 4 files changed, 34 insertions(+), 46 deletions(-)
pgsql: Improve reporting of errors in extension script files.
Improve reporting of errors in extension script files. Previously, CREATE/ALTER EXTENSION gave basically no useful context about errors reported while executing script files. I think the idea was that you could run the same commands manually to see the error, but that's often quite inconvenient. Let's improve that. If we get an error during raw parsing, we won't have a current statement identified by a RawStmt node, but we should always get a syntax error position. Show the portion of the script from the last semicolon-newline before the error position to the first one after it. There are cases where this might show only a fragment of a statement, but that should be uncommon, and it seems better than showing the whole script file. Without an error cursor, if we have gotten past raw parsing (which we probably have), we can report just the current SQL statement as an item of error context. In any case also report the script file name as error context, since it might not be entirely obvious which of a series of update scripts failed. We can also show an approximate script line number in case whatever we printed of the query isn't sufficiently identifiable. The error-context code path is already exercised by some test_extensions test cases, but add tests for the syntax-error path. Discussion: https://postgr.es/m/zvv1clhnbjlcz...@msg.df7cb.de Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/774171c4f640853b1cf8747a4762631d2f5d25be Modified Files -- src/backend/commands/extension.c | 167 - src/test/modules/test_extensions/Makefile | 4 +- .../test_extensions/expected/test_extensions.out | 42 ++ src/test/modules/test_extensions/meson.build | 2 + .../test_extensions/sql/test_extensions.sql| 4 + .../test_extensions/test_ext7--2.0--2.1bad.sql | 14 ++ .../test_extensions/test_ext7--2.0--2.2bad.sql | 13 ++ src/tools/pgindent/typedefs.list | 1 + 8 files changed, 244 insertions(+), 3 deletions(-)
Re: pgsql: Implement pg_wal_replay_wait() stored procedure
Hi, Alexander! On Tue, 22 Oct 2024 at 13:26, Alexander Korotkov wrote: > On Wed, Oct 16, 2024 at 11:20 PM Alexander Korotkov > wrote: > > > > On Wed, Oct 16, 2024 at 10:35 PM Peter Eisentraut > wrote: > > > On 02.09.24 01:55, Alexander Korotkov wrote: > > > > On Mon, Sep 2, 2024 at 2:28 AM Michael Paquier > wrote: > > > >> On Sun, Sep 01, 2024 at 10:35:27PM +0300, Alexander Korotkov wrote: > > > >>> This path hasn't changes since the patch revision when it was a > > > >>> utility command. I agree that this doesn't look like proper path > for > > > >>> stored procedure. But I don't think src/backend/utils/adt is > > > >>> appropriate path either, because it's not really about data type. > > > >>> pg_wal_replay_wait() looks a good neighbor for > > > >>> pg_wal_replay_pause()/pg_wal_replay_resume() and other WAL-related > > > >>> functions. So, what about moving it to src/backend/access/transam? > > > >> > > > >> Moving the new function to xlogfuncs.c while publishing > > > >> WaitForLSNReplay() makes sense to me. > > > > > > > > Thank you for proposal. I like this. > > > > > > > > Could you, please, check the attached patch? > > > > > > We still have stuff in src/backend/commands/waitlsn.c that is nothing > > > like a "command". You have moved some stuff elsewhere, but what are > you > > > planning to do with the rest? > > > > Thank you for spotting this another time. What about moving that > > somewhere like src/backend/access/transam/xlogwait.c ? > > Implemented this as a separate patch (0001). Also rebased other > pending patches on that. 0004 now revises header comment of > xlogwait.c with new procedure signature. > I've looked at v5 of a patchset. 0001: Looks completely ok. 0002: As stated in latch.c - WL_POSTMASTER_DEATH: Wait for postmaster to die - WL_EXIT_ON_PM_DEATH: Exit immediately if the postmaster dies * wakeEvents must include either WL_EXIT_ON_PM_DEATH for automatic exit * if the postmaster dies or WL_POSTMASTER_DEATH for a flag set in the * return value if the postmaster dies It's not completely clear to me if these comments need some clarification (not related to the patchset), or if we should look for WL_EXIT_ON_PM_DEATH for immediately FATAL. Or waiting for postmaster to die on WL_POSTMASTER_DEATH instead of just fatal immediately? 0003: Besides refactor it looks that deleteLSNWaiter() is added in WaitForLSNReplay. Maybe it's worth mentioning in the commit message. Maybe refactoring for loop for assigning result variable and breaking a loop instead of immediate return would look better and lead to natural call of after the loop before returning. 0004: Comment: + * Waits until recovery replays the target LSN with optional timeout. Throw + * an error on failure. may need mentioning "Unless no_error provided throws an error on failure" Otherwise the patch looks good. Regards, Pavel Borisov Supabase
Re: pgsql: Implement pg_wal_replay_wait() stored procedure
Hi, Alexander! On Wed, 23 Oct 2024 at 00:12, Alexander Korotkov wrote: > Hi, Pavel! > > Thank you for your review. > > On Tue, Oct 22, 2024 at 4:30 PM Pavel Borisov > wrote: > > On Tue, 22 Oct 2024 at 13:26, Alexander Korotkov > wrote: > >> > >> On Wed, Oct 16, 2024 at 11:20 PM Alexander Korotkov > >> wrote: > >> > > >> > On Wed, Oct 16, 2024 at 10:35 PM Peter Eisentraut < > pe...@eisentraut.org> wrote: > >> > > On 02.09.24 01:55, Alexander Korotkov wrote: > >> > > > On Mon, Sep 2, 2024 at 2:28 AM Michael Paquier < > mich...@paquier.xyz> wrote: > >> > > >> On Sun, Sep 01, 2024 at 10:35:27PM +0300, Alexander Korotkov > wrote: > >> > > >>> This path hasn't changes since the patch revision when it was a > >> > > >>> utility command. I agree that this doesn't look like proper > path for > >> > > >>> stored procedure. But I don't think src/backend/utils/adt is > >> > > >>> appropriate path either, because it's not really about data > type. > >> > > >>> pg_wal_replay_wait() looks a good neighbor for > >> > > >>> pg_wal_replay_pause()/pg_wal_replay_resume() and other > WAL-related > >> > > >>> functions. So, what about moving it to > src/backend/access/transam? > >> > > >> > >> > > >> Moving the new function to xlogfuncs.c while publishing > >> > > >> WaitForLSNReplay() makes sense to me. > >> > > > > >> > > > Thank you for proposal. I like this. > >> > > > > >> > > > Could you, please, check the attached patch? > >> > > > >> > > We still have stuff in src/backend/commands/waitlsn.c that is > nothing > >> > > like a "command". You have moved some stuff elsewhere, but what > are you > >> > > planning to do with the rest? > >> > > >> > Thank you for spotting this another time. What about moving that > >> > somewhere like src/backend/access/transam/xlogwait.c ? > >> > >> Implemented this as a separate patch (0001). Also rebased other > >> pending patches on that. 0004 now revises header comment of > >> xlogwait.c with new procedure signature. > > > > > > I've looked at v5 of a patchset. > > > 0002: > > > > As stated in latch.c > > > > - WL_POSTMASTER_DEATH: Wait for postmaster to die > > - WL_EXIT_ON_PM_DEATH: Exit immediately if the postmaster dies > > > > * wakeEvents must include either WL_EXIT_ON_PM_DEATH for automatic exit > > * if the postmaster dies or WL_POSTMASTER_DEATH for a flag set in the > > * return value if the postmaster dies > > > > It's not completely clear to me if these comments need some > clarification (not related to the patchset), or if we should look for > WL_EXIT_ON_PM_DEATH for immediately FATAL. Or waiting for postmaster to die > on WL_POSTMASTER_DEATH instead of just fatal immediately? > > As I get from the code, WL_EXIT_ON_PM_DEATH cause process to just > proc_exit(1) without throwing FATAL. So, in the most of situations we > do throw FATAL after seeing WL_POSTMASTER_DEATH event. So, it's > reasonable to do the same here. But indeed, this is a question (not > related to this patch) whether WL_EXIT_ON_PM_DEATH should cause > process to throw FATAL. > Libpq ends up with FATAL on WL_POSTMASTER_DEATH. In a backend code on WL_POSTMASTER_DEATH: SyncRepWaitForLSN() sets ProcDiePending but don't FATAL. Walsender exits proc_exit(1). I suppose WL_POSTMASTER_DEATH expected behavior is "Do whatever you want: wait for postmaster to die or end up immediately". I think path 0002 is good. I looked through patches v6 and I think they're all good now. Regards, Pavel Borisov Supabase.
Re: pgsql: Add functions pg_set_attribute_stats() and pg_clear_attribute_st
Jeff Davis writes: > Add functions pg_set_attribute_stats() and pg_clear_attribute_stats(). A couple of buildfarm animals, eg [1], are mildly unhappy: In file included from ../../../src/include/nodes/execnodes.h:37:0, from ../../../src/include/catalog/indexing.h:19, from attribute_stats.c:21: attribute_stats.c: In function \342\200\230text_to_stavalues\342\200\231: ../../../src/include/nodes/miscnodes.h:54:15: warning: the comparison will always evaluate as \342\200\230true\342\200\231 for the address of \342\200\230escontext\342\200\231 will never be NULL [-Waddress] ((escontext) != NULL && IsA(escontext, ErrorSaveContext) && \\ ^ attribute_stats.c:636:6: note: in expansion of macro \342\200\230SOFT_ERROR_OCCURRED\342\200\231 if (SOFT_ERROR_OCCURRED(&escontext)) ^ I think what we've done in other places is just to test escontext.error_occurred directly, since the rest of the macro is useless in this situation. regards, tom lane [1] https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=rhinoceros&dt=2024-10-23%2000%3A47%3A26&stg=make
pgsql: Make all Perl warnings fatal in 043_wal_replay_wait.pl
Make all Perl warnings fatal in 043_wal_replay_wait.pl This file was committed after c5385929593, but accidentally missed changing all warnings into fatal errors. Reported-by: Anton Voloshin Discussion: https://postgr.es/m/aa8a55d5-554a-4027-a491-1b0ca7c85f7a%40postgrespro.ru Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/e1555645d729e0b91f644954e83e1ed51baa58df Modified Files -- src/test/recovery/t/043_wal_replay_wait.pl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
pgsql: ecpg: Refactor ecpg_log() to skip unnecessary calls to ECPGget_s
ecpg: Refactor ecpg_log() to skip unnecessary calls to ECPGget_sqlca(). Previously, ecpg_log() always called ECPGget_sqlca() to retrieve sqlca, even though it was only needed for debug logging. This commit updates ecpg_log() to call ECPGget_sqlca() only when debug logging is enabled. Author: Yuto Sasaki Reviewed-by: Alvaro Herrera, Tom Lane, Fujii Masao Discussion: https://postgr.es/m/ty2pr01mb3628a85689649babc9a1c6c3c1...@ty2pr01mb3628.jpnprd01.prod.outlook.com Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/7c4d3fe272f3ab62b36f21e3fa6ae00751f00e31 Modified Files -- src/interfaces/ecpg/ecpglib/misc.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
Re: pgsql: Implement pg_wal_replay_wait() stored procedure
Fix a typo of myself: > Maybe refactoring for loop for assigning result variable and breaking a > loop instead of immediate return would look better and lead to natural call > of after the loop before returning. > Maybe refactoring for loop for assigning result variable and breaking a loop instead of immediate return would look better and lead to natural call of *deleteLSNWaiter()* after the loop before returning. Pavel.
pgsql: Restructure foreign key handling code for ATTACH/DETACH
Restructure foreign key handling code for ATTACH/DETACH ... to fix bugs when the referenced table is partitioned. The catalog representation we chose for foreign keys connecting partitioned tables (in commit f56f8f8da6af) is inconvenient, in the sense that a standalone table has a different way to represent the constraint when referencing a partitioned table, than when the same table becomes a partition (and vice versa). Because of this, we need to create additional catalog rows on detach (pg_constraint and pg_trigger), and remove them on attach. We were doing some of those things, but not all of them, leading to missing catalog rows in certain cases. The worst problem seems to be that we are missing action triggers after detaching a partition, which means that you could update/delete rows from the referenced partitioned table that still had referencing rows on that table, the server failing to throw the required errors. !!! Note that this means existing databases with FKs that reference partitioned tables might have rows that break relational integrity, on tables that were once partitions on the referencing side of the FK. Another possible problem is that trying to reattach a table that had been detached would fail indicating that internal triggers cannot be found, which from the user's point of view is nonsensical. In branches 15 and above, we fix this by creating a new helper function addFkConstraint() which is in charge of creating a standalone pg_constraint row, and repurposing addFkRecurseReferencing() and addFkRecurseReferenced() so that they're only the recursive routine for each side of the FK, and they call addFkConstraint() to create pg_constraint at each partitioning level and add the necessary triggers. These new routines can be used during partition creation, partition attach and detach, and foreign key creation. This reduces redundant code and simplifies the flow. In branches 14 and 13, we have a much simpler fix that consists on simply removing the constraint on detach. The reason is that those branches are missing commit f4566345cf40, which reworked the way this works in a way that we didn't consider back-patchable at the time. We opted to leave branch 12 alone, because it's different from branch 13 enough that the fix doesn't apply; and because it is going in EOL mode very soon, patching it now might be worse since there's no way to undo the damage if it goes wrong. Existing databases might need to be repaired. In the future we might want to rethink the catalog representation to avoid this problem, but for now the code seems to do what's required to make the constraints operate correctly. Co-authored-by: Jehan-Guillaume de Rorthais Co-authored-by: Tender Wang Co-authored-by: Alvaro Herrera Reported-by: Guillaume Lelarge Reported-by: Jehan-Guillaume de Rorthais Reported-by: Thomas Baehler (SBB CFF FFS) Discussion: https://postgr.es/m/20230420144344.40744130@karst Discussion: https://postgr.es/m/20230705233028.2f554f73@karst Discussion: https://postgr.es/m/gvap278mb02787e7134fd691861635a8bc9...@gvap278mb0278.chep278.prod.outlook.com Discussion: https://postgr.es/m/18541-628a61bc267cd...@postgresql.org Branch -- REL_16_STABLE Details --- https://git.postgresql.org/pg/commitdiff/2aaf2a28b87eca69b9799ad427f66ba8eddb00c5 Modified Files -- src/backend/commands/tablecmds.c | 658 +++--- src/test/regress/expected/foreign_key.out | 96 + src/test/regress/sql/foreign_key.sql | 57 +++ src/tools/pgindent/typedefs.list | 1 + 4 files changed, 564 insertions(+), 248 deletions(-)
pgsql: Restructure foreign key handling code for ATTACH/DETACH
Restructure foreign key handling code for ATTACH/DETACH ... to fix bugs when the referenced table is partitioned. The catalog representation we chose for foreign keys connecting partitioned tables (in commit f56f8f8da6af) is inconvenient, in the sense that a standalone table has a different way to represent the constraint when referencing a partitioned table, than when the same table becomes a partition (and vice versa). Because of this, we need to create additional catalog rows on detach (pg_constraint and pg_trigger), and remove them on attach. We were doing some of those things, but not all of them, leading to missing catalog rows in certain cases. The worst problem seems to be that we are missing action triggers after detaching a partition, which means that you could update/delete rows from the referenced partitioned table that still had referencing rows on that table, the server failing to throw the required errors. !!! Note that this means existing databases with FKs that reference partitioned tables might have rows that break relational integrity, on tables that were once partitions on the referencing side of the FK. Another possible problem is that trying to reattach a table that had been detached would fail indicating that internal triggers cannot be found, which from the user's point of view is nonsensical. In branches 15 and above, we fix this by creating a new helper function addFkConstraint() which is in charge of creating a standalone pg_constraint row, and repurposing addFkRecurseReferencing() and addFkRecurseReferenced() so that they're only the recursive routine for each side of the FK, and they call addFkConstraint() to create pg_constraint at each partitioning level and add the necessary triggers. These new routines can be used during partition creation, partition attach and detach, and foreign key creation. This reduces redundant code and simplifies the flow. In branches 14 and 13, we have a much simpler fix that consists on simply removing the constraint on detach. The reason is that those branches are missing commit f4566345cf40, which reworked the way this works in a way that we didn't consider back-patchable at the time. We opted to leave branch 12 alone, because it's different from branch 13 enough that the fix doesn't apply; and because it is going in EOL mode very soon, patching it now might be worse since there's no way to undo the damage if it goes wrong. Existing databases might need to be repaired. In the future we might want to rethink the catalog representation to avoid this problem, but for now the code seems to do what's required to make the constraints operate correctly. Co-authored-by: Jehan-Guillaume de Rorthais Co-authored-by: Tender Wang Co-authored-by: Alvaro Herrera Reported-by: Guillaume Lelarge Reported-by: Jehan-Guillaume de Rorthais Reported-by: Thomas Baehler (SBB CFF FFS) Discussion: https://postgr.es/m/20230420144344.40744130@karst Discussion: https://postgr.es/m/20230705233028.2f554f73@karst Discussion: https://postgr.es/m/gvap278mb02787e7134fd691861635a8bc9...@gvap278mb0278.chep278.prod.outlook.com Discussion: https://postgr.es/m/18541-628a61bc267cd...@postgresql.org Branch -- REL_17_STABLE Details --- https://git.postgresql.org/pg/commitdiff/5914a22f6ea5a3a8485cf626e76a63f3f6463de4 Modified Files -- src/backend/commands/tablecmds.c | 658 +++--- src/test/regress/expected/foreign_key.out | 96 + src/test/regress/sql/foreign_key.sql | 57 +++ src/tools/pgindent/typedefs.list | 1 + 4 files changed, 564 insertions(+), 248 deletions(-)
pgsql: Restructure foreign key handling code for ATTACH/DETACH
Restructure foreign key handling code for ATTACH/DETACH ... to fix bugs when the referenced table is partitioned. The catalog representation we chose for foreign keys connecting partitioned tables (in commit f56f8f8da6af) is inconvenient, in the sense that a standalone table has a different way to represent the constraint when referencing a partitioned table, than when the same table becomes a partition (and vice versa). Because of this, we need to create additional catalog rows on detach (pg_constraint and pg_trigger), and remove them on attach. We were doing some of those things, but not all of them, leading to missing catalog rows in certain cases. The worst problem seems to be that we are missing action triggers after detaching a partition, which means that you could update/delete rows from the referenced partitioned table that still had referencing rows on that table, the server failing to throw the required errors. !!! Note that this means existing databases with FKs that reference partitioned tables might have rows that break relational integrity, on tables that were once partitions on the referencing side of the FK. Another possible problem is that trying to reattach a table that had been detached would fail indicating that internal triggers cannot be found, which from the user's point of view is nonsensical. In branches 15 and above, we fix this by creating a new helper function addFkConstraint() which is in charge of creating a standalone pg_constraint row, and repurposing addFkRecurseReferencing() and addFkRecurseReferenced() so that they're only the recursive routine for each side of the FK, and they call addFkConstraint() to create pg_constraint at each partitioning level and add the necessary triggers. These new routines can be used during partition creation, partition attach and detach, and foreign key creation. This reduces redundant code and simplifies the flow. In branches 14 and 13, we have a much simpler fix that consists on simply removing the constraint on detach. The reason is that those branches are missing commit f4566345cf40, which reworked the way this works in a way that we didn't consider back-patchable at the time. We opted to leave branch 12 alone, because it's different from branch 13 enough that the fix doesn't apply; and because it is going in EOL mode very soon, patching it now might be worse since there's no way to undo the damage if it goes wrong. Existing databases might need to be repaired. In the future we might want to rethink the catalog representation to avoid this problem, but for now the code seems to do what's required to make the constraints operate correctly. Co-authored-by: Jehan-Guillaume de Rorthais Co-authored-by: Tender Wang Co-authored-by: Alvaro Herrera Reported-by: Guillaume Lelarge Reported-by: Jehan-Guillaume de Rorthais Reported-by: Thomas Baehler (SBB CFF FFS) Discussion: https://postgr.es/m/20230420144344.40744130@karst Discussion: https://postgr.es/m/20230705233028.2f554f73@karst Discussion: https://postgr.es/m/gvap278mb02787e7134fd691861635a8bc9...@gvap278mb0278.chep278.prod.outlook.com Discussion: https://postgr.es/m/18541-628a61bc267cd...@postgresql.org Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/53af9491a0439720094a11b72602952d79f59ac7 Modified Files -- src/backend/commands/tablecmds.c | 675 +++--- src/test/regress/expected/foreign_key.out | 96 + src/test/regress/sql/foreign_key.sql | 57 +++ src/tools/pgindent/typedefs.list | 1 + 4 files changed, 575 insertions(+), 254 deletions(-)
pgsql: Restructure foreign key handling code for ATTACH/DETACH
Restructure foreign key handling code for ATTACH/DETACH ... to fix bugs when the referenced table is partitioned. The catalog representation we chose for foreign keys connecting partitioned tables (in commit f56f8f8da6af) is inconvenient, in the sense that a standalone table has a different way to represent the constraint when referencing a partitioned table, than when the same table becomes a partition (and vice versa). Because of this, we need to create additional catalog rows on detach (pg_constraint and pg_trigger), and remove them on attach. We were doing some of those things, but not all of them, leading to missing catalog rows in certain cases. The worst problem seems to be that we are missing action triggers after detaching a partition, which means that you could update/delete rows from the referenced partitioned table that still had referencing rows on that table, the server failing to throw the required errors. !!! Note that this means existing databases with FKs that reference partitioned tables might have rows that break relational integrity, on tables that were once partitions on the referencing side of the FK. Another possible problem is that trying to reattach a table that had been detached would fail indicating that internal triggers cannot be found, which from the user's point of view is nonsensical. In branches 15 and above, we fix this by creating a new helper function addFkConstraint() which is in charge of creating a standalone pg_constraint row, and repurposing addFkRecurseReferencing() and addFkRecurseReferenced() so that they're only the recursive routine for each side of the FK, and they call addFkConstraint() to create pg_constraint at each partitioning level and add the necessary triggers. These new routines can be used during partition creation, partition attach and detach, and foreign key creation. This reduces redundant code and simplifies the flow. In branches 14 and 13, we have a much simpler fix that consists on simply removing the constraint on detach. The reason is that those branches are missing commit f4566345cf40, which reworked the way this works in a way that we didn't consider back-patchable at the time. We opted to leave branch 12 alone, because it's different from branch 13 enough that the fix doesn't apply; and because it is going in EOL mode very soon, patching it now might be worse since there's no way to undo the damage if it goes wrong. Existing databases might need to be repaired. In the future we might want to rethink the catalog representation to avoid this problem, but for now the code seems to do what's required to make the constraints operate correctly. Co-authored-by: Jehan-Guillaume de Rorthais Co-authored-by: Tender Wang Co-authored-by: Alvaro Herrera Reported-by: Guillaume Lelarge Reported-by: Jehan-Guillaume de Rorthais Reported-by: Thomas Baehler (SBB CFF FFS) Discussion: https://postgr.es/m/20230420144344.40744130@karst Discussion: https://postgr.es/m/20230705233028.2f554f73@karst Discussion: https://postgr.es/m/gvap278mb02787e7134fd691861635a8bc9...@gvap278mb0278.chep278.prod.outlook.com Discussion: https://postgr.es/m/18541-628a61bc267cd...@postgresql.org Branch -- REL_14_STABLE Details --- https://git.postgresql.org/pg/commitdiff/46a8c27a72269dcd631d30adc1f76cbee50bf969 Modified Files -- src/backend/commands/tablecmds.c | 160 +- src/test/regress/expected/foreign_key.out | 91 + src/test/regress/sql/foreign_key.sql | 56 +++ 3 files changed, 281 insertions(+), 26 deletions(-)
pgsql: Restructure foreign key handling code for ATTACH/DETACH
Restructure foreign key handling code for ATTACH/DETACH ... to fix bugs when the referenced table is partitioned. The catalog representation we chose for foreign keys connecting partitioned tables (in commit f56f8f8da6af) is inconvenient, in the sense that a standalone table has a different way to represent the constraint when referencing a partitioned table, than when the same table becomes a partition (and vice versa). Because of this, we need to create additional catalog rows on detach (pg_constraint and pg_trigger), and remove them on attach. We were doing some of those things, but not all of them, leading to missing catalog rows in certain cases. The worst problem seems to be that we are missing action triggers after detaching a partition, which means that you could update/delete rows from the referenced partitioned table that still had referencing rows on that table, the server failing to throw the required errors. !!! Note that this means existing databases with FKs that reference partitioned tables might have rows that break relational integrity, on tables that were once partitions on the referencing side of the FK. Another possible problem is that trying to reattach a table that had been detached would fail indicating that internal triggers cannot be found, which from the user's point of view is nonsensical. In branches 15 and above, we fix this by creating a new helper function addFkConstraint() which is in charge of creating a standalone pg_constraint row, and repurposing addFkRecurseReferencing() and addFkRecurseReferenced() so that they're only the recursive routine for each side of the FK, and they call addFkConstraint() to create pg_constraint at each partitioning level and add the necessary triggers. These new routines can be used during partition creation, partition attach and detach, and foreign key creation. This reduces redundant code and simplifies the flow. In branches 14 and 13, we have a much simpler fix that consists on simply removing the constraint on detach. The reason is that those branches are missing commit f4566345cf40, which reworked the way this works in a way that we didn't consider back-patchable at the time. We opted to leave branch 12 alone, because it's different from branch 13 enough that the fix doesn't apply; and because it is going in EOL mode very soon, patching it now might be worse since there's no way to undo the damage if it goes wrong. Existing databases might need to be repaired. In the future we might want to rethink the catalog representation to avoid this problem, but for now the code seems to do what's required to make the constraints operate correctly. Co-authored-by: Jehan-Guillaume de Rorthais Co-authored-by: Tender Wang Co-authored-by: Alvaro Herrera Reported-by: Guillaume Lelarge Reported-by: Jehan-Guillaume de Rorthais Reported-by: Thomas Baehler (SBB CFF FFS) Discussion: https://postgr.es/m/20230420144344.40744130@karst Discussion: https://postgr.es/m/20230705233028.2f554f73@karst Discussion: https://postgr.es/m/gvap278mb02787e7134fd691861635a8bc9...@gvap278mb0278.chep278.prod.outlook.com Discussion: https://postgr.es/m/18541-628a61bc267cd...@postgresql.org Branch -- REL_15_STABLE Details --- https://git.postgresql.org/pg/commitdiff/5d83bad6b87d0d65818ba631a254f9a74fa20601 Modified Files -- src/backend/commands/tablecmds.c | 661 +++--- src/test/regress/expected/foreign_key.out | 96 + src/test/regress/sql/foreign_key.sql | 57 +++ src/tools/pgindent/typedefs.list | 1 + 4 files changed, 565 insertions(+), 250 deletions(-)
pgsql: Restructure foreign key handling code for ATTACH/DETACH
Restructure foreign key handling code for ATTACH/DETACH ... to fix bugs when the referenced table is partitioned. The catalog representation we chose for foreign keys connecting partitioned tables (in commit f56f8f8da6af) is inconvenient, in the sense that a standalone table has a different way to represent the constraint when referencing a partitioned table, than when the same table becomes a partition (and vice versa). Because of this, we need to create additional catalog rows on detach (pg_constraint and pg_trigger), and remove them on attach. We were doing some of those things, but not all of them, leading to missing catalog rows in certain cases. The worst problem seems to be that we are missing action triggers after detaching a partition, which means that you could update/delete rows from the referenced partitioned table that still had referencing rows on that table, the server failing to throw the required errors. !!! Note that this means existing databases with FKs that reference partitioned tables might have rows that break relational integrity, on tables that were once partitions on the referencing side of the FK. Another possible problem is that trying to reattach a table that had been detached would fail indicating that internal triggers cannot be found, which from the user's point of view is nonsensical. In branches 15 and above, we fix this by creating a new helper function addFkConstraint() which is in charge of creating a standalone pg_constraint row, and repurposing addFkRecurseReferencing() and addFkRecurseReferenced() so that they're only the recursive routine for each side of the FK, and they call addFkConstraint() to create pg_constraint at each partitioning level and add the necessary triggers. These new routines can be used during partition creation, partition attach and detach, and foreign key creation. This reduces redundant code and simplifies the flow. In branches 14 and 13, we have a much simpler fix that consists on simply removing the constraint on detach. The reason is that those branches are missing commit f4566345cf40, which reworked the way this works in a way that we didn't consider back-patchable at the time. We opted to leave branch 12 alone, because it's different from branch 13 enough that the fix doesn't apply; and because it is going in EOL mode very soon, patching it now might be worse since there's no way to undo the damage if it goes wrong. Existing databases might need to be repaired. In the future we might want to rethink the catalog representation to avoid this problem, but for now the code seems to do what's required to make the constraints operate correctly. Co-authored-by: Jehan-Guillaume de Rorthais Co-authored-by: Tender Wang Co-authored-by: Alvaro Herrera Reported-by: Guillaume Lelarge Reported-by: Jehan-Guillaume de Rorthais Reported-by: Thomas Baehler (SBB CFF FFS) Discussion: https://postgr.es/m/20230420144344.40744130@karst Discussion: https://postgr.es/m/20230705233028.2f554f73@karst Discussion: https://postgr.es/m/gvap278mb02787e7134fd691861635a8bc9...@gvap278mb0278.chep278.prod.outlook.com Discussion: https://postgr.es/m/18541-628a61bc267cd...@postgresql.org Branch -- REL_13_STABLE Details --- https://git.postgresql.org/pg/commitdiff/d20194cead7576dc53fc19e54ea2ec8547bae6d2 Modified Files -- src/backend/commands/tablecmds.c | 160 +- src/test/regress/expected/foreign_key.out | 91 + src/test/regress/sql/foreign_key.sql | 56 +++ 3 files changed, 281 insertions(+), 26 deletions(-)
