pgsql: Revert pg_wal_replay_wait() stored procedure
Revert pg_wal_replay_wait() stored procedure This commit reverts 3c5db1d6b0, and subsequent improvements and fixes including 8036d73ae3, 867d396ccd, 3ac3ec580c, 0868d7ae70, 85b98b8d5a, 2520226c95, 014f9f34d2, e658038772, e1555645d7, 5035172e4a, 6cfebfe88b, 73da6b8d1b, and e546989a26. The reason for reverting is a set of remaining issues. Most notably, the stored procedure appears to need more effort than the utility statement to turn the backend into a "snapshot-less" state. This makes an approach to use stored procedures questionable. Catversion is bumped. Discussion: https://postgr.es/m/Zyhj2anOPRKtb0xW%40paquier.xyz Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/3a7ae6b3d91e0d011dba1eb8a29e1836c6a33c75 Modified Files -- doc/src/sgml/func.sgml | 170 src/backend/access/transam/Makefile | 3 +- src/backend/access/transam/meson.build | 1 - src/backend/access/transam/xact.c | 6 - src/backend/access/transam/xlog.c | 7 - src/backend/access/transam/xlogfuncs.c | 116 +--- src/backend/access/transam/xlogrecovery.c | 11 - src/backend/access/transam/xlogwait.c | 337 src/backend/catalog/system_functions.sql| 5 - src/backend/lib/pairingheap.c | 18 +- src/backend/storage/ipc/ipci.c | 3 - src/backend/storage/lmgr/proc.c | 6 - src/backend/tcop/pquery.c | 9 +- src/backend/utils/activity/wait_event_names.txt | 2 - src/include/access/xlogwait.h | 89 --- src/include/catalog/catversion.h| 2 +- src/include/catalog/pg_proc.dat | 11 - src/include/lib/pairingheap.h | 3 - src/include/storage/lwlocklist.h| 1 - src/test/recovery/meson.build | 1 - src/test/recovery/t/043_wal_replay_wait.pl | 225 src/tools/pgindent/typedefs.list| 3 - 22 files changed, 9 insertions(+), 1020 deletions(-)
Re: pgsql: Implement pg_wal_replay_wait() stored procedure
On Mon, Oct 28, 2024 at 5:47 PM Robert Haas wrote: > Point of order: > > Discussions of commits and potential followup commits are best > redirected to -hackers rather than continuing to use -committers. Thank you for pointing this. My response to Heikki's last message will be redirected to -hackers. -- Regards, Alexander Korotkov Supabase
Re: pgsql: Implement pg_wal_replay_wait() stored procedure
Hi, Heikki!
On Fri, Oct 25, 2024 at 9:06 AM Heikki Linnakangas wrote:
> If you call this procedure on a stand-alone server, you get:
>
> postgres=# call pg_wal_replay_wait('1234/0');
> ERROR: recovery is not in progress
> DETAIL: Recovery ended before replaying target LSN 1234/0; last replay
> LSN 0/0.
>
> The DETAIL seems a bit misleading. Recovery never ended, because it
> never started in the first place. Last replay LSN is indeed 0/0, but
> that's not helpful.
>
> If a standby server has been promoted and you pass an LSN that's earlier
> than the last replay LSN, it returns successfully. That makes sense I
> guess; if you connect to a standby and wait for it to replay a commit
> that you made in the primary, and the standby gets promoted, that seems
> correct. But it's a little inconsistent: If the standby crashes
> immediately after promotion, and you call pg_wal_replay_wait() after
> recovery, it returns success. However, if you shut down the promoted
> server and restart it, then last replay LSN is 0/0, and the call will
> fail because no recovery happened.
>
> What is the use case for the 'no_error' argument? Why would you ever
> want to pass no_error=true ? The separate pg_wal_replay_wait_status()
> function feels weird to me. Also it surely shouldn't be marked IMMUTABLE
> nor parallel safe.
>
> This would benefit from more documentation, explaining how you would use
> this in practice. I believe the use case is that you want "read your
> writes" consistency between a primary and a standby. You commit a
> transaction in the primary, and you then want to run read-only queries
> in a standby, and you want to make sure that you see your own commit,
> but you're ok with slightly delayed results otherwise. It would be good
> to add a chapter somewhere in the docs to show how to do that in
> practice with these functions.
Thank you for your feedback!
I do agree that error reporting for "not in recovery" case needs to be
improved, as well, as the documentation.
I see that pg_wal_replay_wait_status() might look weird, but it seems
to me like the best of feasible solutions. Given that
pg_wal_replay_wait() procedure can't work concurrently to a query
involving pg_wal_replay_wait_status() function, I think
pg_wal_replay_wait_status() should be stable and parallel safe.
This is the brief answer. I will be able to come back with more
details on Monday.
--
Regards,
Alexander Korotkov
Supabase
pgsql: Fix concurrrently in typcache_rel_type_cache.sql
Fix concurrrently in typcache_rel_type_cache.sql All injection points there must be local. Otherwise it affects parallel tests. Reported-by: Andres Freund Discussion: https://postgr.es/m/b3ybc66l6lhmtzj2n7ypumz5yjz7njc46sddsqshdtstgj74ah%40qgtn6nzokj6a Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/aa1e898dea29bb5314fd25445545b9eb7b27 Modified Files -- src/test/modules/typcache/expected/typcache_rel_type_cache.out | 7 +++ src/test/modules/typcache/sql/typcache_rel_type_cache.sql | 3 +++ 2 files changed, 10 insertions(+)
pgsql: Move LSN waiting declarations and definitions to better place
Move LSN waiting declarations and definitions to better place
3c5db1d6b implemented the pg_wal_replay_wait() stored procedure. Due to
the patch development history, the implementation resided in
src/backend/commands/waitlsn.c (src/include/commands/waitlsn.h for headers).
014f9f34d moved pg_wal_replay_wait() itself to
src/backend/access/transam/xlogfuncs.c near to the WAL-manipulation functions.
But most of the implementation stayed in place.
The code in src/backend/commands/waitlsn.c has nothing to do with commands,
but is related to WAL. So, this commit moves this code into
src/backend/access/transam/xlogwait.c (src/include/access/xlogwait.h for
headers).
Reported-by: Peter Eisentraut
Discussion:
https://postgr.es/m/18c0fa64-0475-415e-a1bd-665d922c5201%40eisentraut.org
Reviewed-by: Pavel Borisov
Branch
--
master
Details
---
https://git.postgresql.org/pg/commitdiff/5035172e4ab58e4e8eef1bc60b0712fc59e0be31
Modified Files
--
src/backend/access/transam/Makefile | 3 ++-
src/backend/access/transam/meson.build| 1 +
src/backend/access/transam/xact.c | 2 +-
src/backend/access/transam/xlog.c | 2 +-
src/backend/access/transam/xlogfuncs.c| 2 +-
src/backend/access/transam/xlogrecovery.c | 2 +-
src/backend/{commands/waitlsn.c => access/transam/xlogwait.c} | 6 +++---
src/backend/commands/Makefile | 3 +--
src/backend/commands/meson.build | 1 -
src/backend/storage/ipc/ipci.c| 2 +-
src/backend/storage/lmgr/proc.c | 2 +-
src/include/{commands/waitlsn.h => access/xlogwait.h} | 10 +-
12 files changed, 18 insertions(+), 18 deletions(-)
pgsql: Refactor WaitForLSNReplay() to return the result of waiting
Refactor WaitForLSNReplay() to return the result of waiting Currently, WaitForLSNReplay() immediately throws an error if waiting for LSN replay is not successful. This commit teaches WaitForLSNReplay() to return the result of waiting, while making pg_wal_replay_wait() responsible for throwing an appropriate error. This is preparation to adding 'no_error' argument to pg_wal_replay_wait() and new function pg_wal_replay_wait_status(), which returns the last wait result status. Additionally, we stop distinguishing situations when we find our instance to be not in a recovery state before entering the waiting loop and inside the waiting loop. Standby promotion may happen at any moment, even between issuing a procedure call statement and pg_wal_replay_wait() doing a first check of recovery status. Thus, there is no pointing distinguishing these situations. Also, since we may exit the waiting loop and see our instance not in recovery without throwing an error, we need to deleteLSNWaiter() in that case. We do this unconditionally for the sake of simplicity, even if standby was already promoted after reaching the target LSN, the startup process surely already deleted us. Reported-by: Michael Paquier Discussion: https://postgr.es/m/ZtUF17gF0pNpwZDI%40paquier.xyz Reviewed-by: Michael Paquier, Pavel Borisov Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/73da6b8d1b3e8b7541961c3534e584243cb0470e Modified Files -- src/backend/access/transam/xlogfuncs.c | 31 ++- src/backend/access/transam/xlogwait.c | 33 +++-- src/include/access/xlogwait.h | 13 - src/tools/pgindent/typedefs.list | 1 + 4 files changed, 54 insertions(+), 24 deletions(-)
pgsql: Make WaitForLSNReplay() issue FATAL on postmaster death
Make WaitForLSNReplay() issue FATAL on postmaster death Reported-by: Michael Paquier Discussion: https://postgr.es/m/ZvY2C8N4ZqgCFaLu%40paquier.xyz Reviewed-by: Pavel Borisov Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/6cfebfe88b9a753152862b83d42d1103125b01bd Modified Files -- src/backend/access/transam/xlogwait.c | 12 +++- 1 file changed, 11 insertions(+), 1 deletion(-)
pgsql: Add 'no_error' argument to pg_wal_replay_wait()
Add 'no_error' argument to pg_wal_replay_wait() This argument allow skipping throwing an error. Instead, the result status can be obtained using pg_wal_replay_wait_status() function. Catversion is bumped. Reported-by: Michael Paquier Discussion: https://postgr.es/m/ZtUF17gF0pNpwZDI%40paquier.xyz Reviewed-by: Pavel Borisov Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/e546989a269d5d73d283901aadcfda8c6d98e87b Modified Files -- doc/src/sgml/func.sgml | 56 ++ src/backend/access/transam/xlogfuncs.c | 38 +--- src/backend/access/transam/xlogwait.c | 3 +- src/backend/catalog/system_functions.sql | 4 ++- src/include/catalog/catversion.h | 2 +- src/include/catalog/pg_proc.dat| 7 +++- src/test/recovery/t/043_wal_replay_wait.pl | 22 7 files changed, 118 insertions(+), 14 deletions(-)
pgsql: Update header comment for lookup_type_cache()
Update header comment for lookup_type_cache() Describe the way we handle concurrent invalidation messages. Discussion: https://postgr.es/m/CAPpHfdsQhwUrnB3of862j9RgHoJM--eRbifvBMvtQxpC57dxCA%40mail.gmail.com Reviewed-by: Andrei Lepikhov, Artur Zakirov, Pavel Borisov Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/c1500a1ba7e16ac9e98c6baf792f0be64a48e839 Modified Files -- src/backend/utils/cache/typcache.c | 9 + 1 file changed, 9 insertions(+)
pgsql: Avoid looping over all type cache entries in TypeCacheRelCallbac
Avoid looping over all type cache entries in TypeCacheRelCallback() Currently, when a single relcache entry gets invalidated, TypeCacheRelCallback() has to loop over all type cache entries to find appropriate typentry to invalidate. Unfortunately, using the syscache here is impossible, because this callback could be called outside a transaction and this makes impossible catalog lookups. This is why present commit introduces RelIdToTypeIdCacheHash to map relation OID to its composite type OID. We are keeping RelIdToTypeIdCacheHash entry while corresponding type cache entry have something to clean. Therefore, RelIdToTypeIdCacheHash shouldn't get bloat in the case of temporary tables flood. There are many places in lookup_type_cache() where syscache invalidation, user interruption, or even error could occur. In order to handle this, we keep an array of in-progress type cache entries. In the case of lookup_type_cache() interruption this array is processed to keep RelIdToTypeIdCacheHash in a consistent state. Discussion: https://postgr.es/m/5812a6e5-68ae-4d84-9d85-b443176966a1%40sigaev.ru Author: Teodor Sigaev Reviewed-by: Aleksander Alekseev, Tom Lane, Michael Paquier, Roman Zharkov Reviewed-by: Andrei Lepikhov, Pavel Borisov, Jian He, Alexander Lakhin Reviewed-by: Artur Zakirov Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/b85a9d046efdd27775cbe7db9e92aad96aab4ada Modified Files -- src/backend/access/transam/xact.c | 10 + src/backend/utils/cache/typcache.c | 359 ++--- src/include/utils/typcache.h | 4 + src/test/modules/Makefile | 4 +- src/test/modules/meson.build | 1 + src/test/modules/typcache/.gitignore | 4 + src/test/modules/typcache/Makefile | 28 ++ .../typcache/expected/typcache_rel_type_cache.out | 34 ++ src/test/modules/typcache/meson.build | 16 + .../typcache/sql/typcache_rel_type_cache.sql | 18 ++ src/tools/pgindent/typedefs.list | 1 + 11 files changed, 433 insertions(+), 46 deletions(-)
Re: pgsql: Implement pg_wal_replay_wait() stored procedure
On Wed, Oct 23, 2024 at 9:02 AM Pavel Borisov wrote: > On Wed, 23 Oct 2024 at 00:12, Alexander Korotkov wrote: >> 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. >> >> > 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. Thank you. I will push this patchset if now objections. -- Regards, Alexander Korotkov Supabase
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: 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(-)
Re: pgsql: Implement pg_wal_replay_wait() stored procedure
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. -- Regards, Alexander Korotkov Supabase v5-0002-Make-WaitForLSNReplay-issue-FATAL-on-postmaster-d.patch Description: Binary data v5-0001-Move-LSN-waiting-declarations-and-definitions-to-.patch Description: Binary data v5-0003-Refactor-WaitForLSNReplay-to-return-the-result-of.patch Description: Binary data v5-0004-Add-no_error-argument-to-pg_wal_replay_wait.patch Description: Binary data
Re: pgsql: Implement pg_wal_replay_wait() stored procedure
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 ? -- Regards, Alexander Korotkov Supabase
Re: pgsql: Create functions pg_set_relation_stats, pg_clear_relation_stats.
On Sun, Oct 13, 2024 at 6:21 PM Jeff Davis wrote: > Create functions pg_set_relation_stats, pg_clear_relation_stats. > > These functions are used to tweak statistics on any relation, provided > that the user has MAINTAIN privilege on the relation, or is the database > owner. +#include "utils/array.h" +#include "utils/builtins.h" +#include "utils/acl.h" +#include "utils/rel.h" Please, check the alphabetical order of includes in stat_utils.c. -- Regards, Alexander Korotkov Supabase
Re: pgsql: Implement pg_wal_replay_wait() stored procedure
Hi!
On Sun, Sep 29, 2024 at 1:40 PM Alexander Korotkov wrote:
>
> Thank you for your review.
>
> On Fri, Sep 27, 2024 at 7:35 AM Michael Paquier wrote:
> >
> > On Fri, Sep 20, 2024 at 03:00:20PM +0300, Alexander Korotkov wrote:
> > > Please, check the attached patchset for implementation of proposed
> > > approach.
> >
> > 0001 looks like it requires an indentation in its .h diffs.
> >
> > +typedef enum
> > +{
> > + WaitLSNResultSuccess, /* Target LSN is reached */
> > + WaitLSNResultTimeout, /* Timeout occured */
> >
> > Perhaps use WAIT_LSN_RESULT_SUCCESS, etc, rather than camel casing.
> >
> > + * Results statuses for WaitForLSNReplay().
> >
> > Too much plural here.
> >
> > What you are doing with WaitForLSNReplay() sounds kind of right.
> >
> > rc = WaitLatch(MyLatch, wake_events, delay_ms,
> > WAIT_EVENT_WAIT_FOR_WAL_REPLAY);
>
> Thank you, fixed in the attached patchset.
>
> > Question about this existing bit in waitlsn.c. Shouldn't this issue a
> > FATAL if rc reports a WL_POSTMASTER_DEATH? Or am I missing an
> > intention here. That was already the case before this patch set.
>
> Fixed in the separate patch.
>
> > pg_wal_replay_wait() is new to v18, meaning that we still have some
> > time to agree on its final shape for this release cycle. This
> > discussion shares similarities with the recent exercise done in
> > f593c5517d14, and I think that we should be more consistent between
> > both to not repeat the same mistakes as in the past, even if this case
> > if more complex because we have more reasons behind why a wait could
> > not have happened.
> >
> > I would suggest to keep things simple and have one single function
> > rather than introduce two more pg_proc entries with slight differences
> > in their error reporting, making the original function return a text
> > about the reason of the failure when waiting (be it a timeout,
> > success, promotion or outside recovery).
>
> I also like to keep things simple. Keeping this as a single function
> is not possible due to the reasons I described in [1]. However, it's
> possible to fit into one stored procedure. I made 'no_error' as an
> argument for the pg_wal_replay_wait() procedure. Done so in the
> attached patchset.
>
> > FWIW, I am confused regarding the need for WaitLSNResultNotInRecovery
> > and WaitLSNResultPromotedConcurrently in the error states. On a code
> > basis, they check the same thing: RecoveryInProgress(). I'd suggest
> > to cut one of them.
>
> Agreed. I initially intended to distinguish situations when the user
> mistakenly calls pg_wal_replay_wait() on replication leader and when
> concurrent promotion happens. However, given that the promotion could
> happen after the user issued the query and before waiting starts, it
> doesn't make much sense.
>
> > This also points to the fact that
> > WaitForLSNReplay() has some duplication that's not required. We could
> > have less GetXLogReplayRecPtr() calls and do all the checks in the for
> > loop. The current structure of the code in waitlsn.c is more complex
> > than it could be.
>
> Not sure about this. I'd like to keep the fast-path check before we
> call addLSNWaiter().
Please, check the revised patchset. It contains more tests for new
function pg_wal_replay_wait_status() and changes for documentation.
--
Regards,
Alexander Korotkov
Supabase
v4-0003-Add-no_error-argument-to-pg_wal_replay_wait.patch
Description: Binary data
v4-0002-Refactor-WaitForLSNReplay-to-return-the-result-of.patch
Description: Binary data
v4-0001-Make-WaitForLSNReplay-issue-FATAL-on-postmaster-d.patch
Description: Binary data
Re: pgsql: Fix expression list handling in ATExecAttachPartition()
On Thu, Oct 3, 2024 at 2:02 PM Amit Langote wrote: > On Thu, Oct 3, 2024 at 7:12 PM Amit Langote > wrote: > > On Thu, Oct 3, 2024 at 18:57 Alexander Korotkov > wrote: > >> > >> Hi, Amit! > >> > >> On Thu, Oct 3, 2024 at 6:01 AM Amit Langote > wrote: > >> > Fix expression list handling in ATExecAttachPartition() > >> > >> +* since it’s needed later to construct the constraint expression > for > >> > >> I don't think we're good about using unicode apostrophes. Could you, > please, switch to ascii? > > > > > > Oh, oops, will fix. Thanks for the heads up. > > Done. > Great, thank you! -- Regards, Alexander Korotkov Supabase
Re: pgsql: Fix expression list handling in ATExecAttachPartition()
Hi, Amit! On Thu, Oct 3, 2024 at 6:01 AM Amit Langote wrote: > Fix expression list handling in ATExecAttachPartition() +* since it’s needed later to construct the constraint expression for I don't think we're good about using unicode apostrophes. Could you, please, switch to ascii? ------ Regards, Alexander Korotkov Supabase
Re: pgsql: Implement pg_wal_replay_wait() stored procedure
Hi!
Thank you for your review.
On Fri, Sep 27, 2024 at 7:35 AM Michael Paquier wrote:
>
> On Fri, Sep 20, 2024 at 03:00:20PM +0300, Alexander Korotkov wrote:
> > Please, check the attached patchset for implementation of proposed approach.
>
> 0001 looks like it requires an indentation in its .h diffs.
>
> +typedef enum
> +{
> + WaitLSNResultSuccess, /* Target LSN is reached */
> + WaitLSNResultTimeout, /* Timeout occured */
>
> Perhaps use WAIT_LSN_RESULT_SUCCESS, etc, rather than camel casing.
>
> + * Results statuses for WaitForLSNReplay().
>
> Too much plural here.
>
> What you are doing with WaitForLSNReplay() sounds kind of right.
>
> rc = WaitLatch(MyLatch, wake_events, delay_ms,
> WAIT_EVENT_WAIT_FOR_WAL_REPLAY);
Thank you, fixed in the attached patchset.
> Question about this existing bit in waitlsn.c. Shouldn't this issue a
> FATAL if rc reports a WL_POSTMASTER_DEATH? Or am I missing an
> intention here. That was already the case before this patch set.
Fixed in the separate patch.
> pg_wal_replay_wait() is new to v18, meaning that we still have some
> time to agree on its final shape for this release cycle. This
> discussion shares similarities with the recent exercise done in
> f593c5517d14, and I think that we should be more consistent between
> both to not repeat the same mistakes as in the past, even if this case
> if more complex because we have more reasons behind why a wait could
> not have happened.
>
> I would suggest to keep things simple and have one single function
> rather than introduce two more pg_proc entries with slight differences
> in their error reporting, making the original function return a text
> about the reason of the failure when waiting (be it a timeout,
> success, promotion or outside recovery).
I also like to keep things simple. Keeping this as a single function
is not possible due to the reasons I described in [1]. However, it's
possible to fit into one stored procedure. I made 'no_error' as an
argument for the pg_wal_replay_wait() procedure. Done so in the
attached patchset.
> FWIW, I am confused regarding the need for WaitLSNResultNotInRecovery
> and WaitLSNResultPromotedConcurrently in the error states. On a code
> basis, they check the same thing: RecoveryInProgress(). I'd suggest
> to cut one of them.
Agreed. I initially intended to distinguish situations when the user
mistakenly calls pg_wal_replay_wait() on replication leader and when
concurrent promotion happens. However, given that the promotion could
happen after the user issued the query and before waiting starts, it
doesn't make much sense.
> This also points to the fact that
> WaitForLSNReplay() has some duplication that's not required. We could
> have less GetXLogReplayRecPtr() calls and do all the checks in the for
> loop. The current structure of the code in waitlsn.c is more complex
> than it could be.
Not sure about this. I'd like to keep the fast-path check before we
call addLSNWaiter().
Links.
1.
https://www.postgresql.org/message-id/CAPpHfdukVbJZntibZZ4HM7p92zN-QmAtD1%2BsAALRTFCsvpAq7A%40mail.gmail.com
--
Regards,
Alexander Korotkov
Supabase
v3-0002-Refactor-WaitForLSNReplay-to-return-the-result-of.patch
Description: Binary data
v3-0003-Add-no_error-argument-to-pg_wal_replay_wait.patch
Description: Binary data
v3-0001-Make-WaitForLSNReplay-issue-FATAL-on-postmaster-d.patch
Description: Binary data
Re: pgsql: Implement pg_wal_replay_wait() stored procedure
On Fri, Sep 27, 2024 at 7:57 AM Michael Paquier wrote: > On Fri, Sep 27, 2024 at 01:35:23PM +0900, Michael Paquier wrote: > > I would suggest to keep things simple and have one single function > > rather than introduce two more pg_proc entries with slight differences > > in their error reporting, making the original function return a text > > about the reason of the failure when waiting (be it a timeout, > > success, promotion or outside recovery). > > More to the point here. I am not sure what's the benefit of having a > procedure, while we have been using SQL functions for similar purposes > in xlogfuncs.c for years. And what you have here can also be coupled > with more complex logic in SQL queries. As I multiple times pointed in the thread [1] [2], this couldn't be done in SQL function. SQL-function should run within snapshot, which could prevent WAL from being replayed. In turn, depending on timeout settings that could lead to hidden deadlock (function waits for LSN to be replayed, replay process wait snapshot to be released) or an error. In the stored procedure, we're releasing active and catalog snapshots (similarly to VACUUM statement). Waiting without holding a snapshots allows us to workaround the problem described above. We can't do this in the SQL function, because that would leave the rest of SQL query without a snapshot. Links. 1. https://www.postgresql.org/message-id/CAPpHfduBSN8j5j5Ynn5x%3DThD%3D8ypNd53D608VXGweBsPzxPvqA%40mail.gmail.com 2. https://www.postgresql.org/message-id/CAPpHfdtiGgn0iS1KbW2HTam-1%2BoK%2BvhXZDAcnX9hKaA7Oe%3DF-A%40mail.gmail.com -- Regards, Alexander Korotkov Supabase
Re: pgsql: Implement pg_wal_replay_wait() stored procedure
On Thu, Sep 26, 2024 at 11:19 AM Michael Paquier wrote:
> On Fri, Aug 02, 2024 at 06:22:21PM +0000, Alexander Korotkov wrote:
> > Implement pg_wal_replay_wait() stored procedure
> >
> > pg_wal_replay_wait() is to be used on standby and specifies waiting for
> > the specific WAL location to be replayed. This option is useful when
> > the user makes some data changes on primary and needs a guarantee to see
> > these changes are on standby.
> >
> > The queue of waiters is stored in the shared memory as an LSN-ordered
> > pairing
> > heap, where the waiter with the nearest LSN stays on the top. During
> > the replay of WAL, waiters whose LSNs have already been replayed are deleted
> > from the shared memory pairing heap and woken up by setting their latches.
> >
> > pg_wal_replay_wait() needs to wait without any snapshot held. Otherwise,
> > the snapshot could prevent the replay of WAL records, implying a kind of
> > self-deadlock. This is why it is only possible to implement
> > pg_wal_replay_wait() as a procedure working without an active snapshot,
> > not a function.
> >
> > Catversion is bumped.
>
> I've spotted that this patch uses an OID that should not be used
> during the development cycle:
> +{ oid => '111',
> + descr => 'wait for the target LSN to be replayed on standby with an
> optional timeout',
>
> Please use something in the 8000- range, as required by
> 98eab30b93d5.
Fixed, sorry for messing this up.
I would appreciate if you have time to look at [0] to check if it
meets your expectations.
Links.
1.
https://www.postgresql.org/message-id/CAPpHfdsw9oq62Fvt65JApHJf1auUirdGJV7%3DnRyVnDL3M8z5xA%40mail.gmail.com
--
Regards,
Alexander Korotkov
Supabase
pgsql: Update oid for pg_wal_replay_wait() procedure
Update oid for pg_wal_replay_wait() procedure Use an oid from 8000- range, as required by 98eab30b93d5. Reported-by: Michael Paquier Discussion: https://postgr.es/m/ZvUY6bfTwB0GsyzP%40paquier.xyz Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/e658038772f59588c8be8b016c8a7e28e7705ab4 Modified Files -- src/include/catalog/catversion.h | 2 +- src/include/catalog/pg_proc.dat | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
Re: pgsql: Implement pg_wal_replay_wait() stored procedure
On Thu, Sep 19, 2024 at 3:47 PM Alexander Korotkov wrote:
> On Tue, Sep 3, 2024 at 4:07 PM Alexander Korotkov
> wrote:
> > On the other hand, I see that returning status could make sense for
> > certain use cases. I think I could write two patches to provide that.
> > 1. Make WaitForLSNReplay() return status, and make pg_wal_replay_wal()
> > be responsible for throwing all the errors.
> > 2. New procedure pg_wal_replay_wal_status() (or some better name?),
> > which returns status to the user instead of throwing errors.
> >
> > If no objections, I will push the patch moving code then go ahead
> > writing the two patches above.
>
> I attempted to implement the patchset as promised. The 0001 is easy
> and straighforward. The 0002 became tricky. Procedures can't return
> values. They can have OUTPUT parameters instead. However, even for
> output parameters you need to pass something in, and that couldn't be
> a default value. Additional difficulty is that having OUTPUT
> parameters seem to hold additional snapshot and prevents our
> snapshot-releasing trick from working
>
> smagen@postgres=# CALL pg_wal_replay_wait('0/0'::pg_lsn);
> CALL
> Time: 2.061 ms
> smagen@postgres=# CALL pg_wal_replay_wait_status(NULL, '0/0'::pg_lsn);
> ERROR: pg_wal_replay_wait_status() must be only called without an
> active or registered snapshot
> DETAIL: Make sure pg_wal_replay_wait_status() isn't called within a
> transaction with an isolation level higher than READ COMMITTED,
> another procedure, or a function.
> Time: 1.175 ms
>
> I'm thinking about following solution:
> 1. pg_wal_replay_wait_no_error() procedure, which doesn't return
> anything but throws no errors.
> 2. Make pg_wal_replay_wait()/pg_wal_replay_wait_no_error() save
> waiting result status to the local variable.
> 3. New function pg_wal_replay_wal_get_status() displaying the result
> status of the last pg_wal_replay_wait()/pg_wal_replay_wait_no_error()
> CALL.
>
> Then one could do.
> CALL pg_wal_replay_wait_no_error(...);
> SELECT pg_wal_replay_wal_get_status();
>
> Probably looks a bit excessive, but probably the best we can do.
Please, check the attached patchset for implementation of proposed approach.
--
Regards,
Alexander Korotkov
Supabase
v2-0001-Refactor-WaitForLSNReplay-to-return-the-result-of.patch
Description: Binary data
v2-0002-Implement-pg_wal_replay_wait_no_error.patch
Description: Binary data
Re: pgsql: Implement pg_wal_replay_wait() stored procedure
On Tue, Sep 3, 2024 at 4:07 PM Alexander Korotkov wrote:
> On the other hand, I see that returning status could make sense for
> certain use cases. I think I could write two patches to provide that.
> 1. Make WaitForLSNReplay() return status, and make pg_wal_replay_wal()
> be responsible for throwing all the errors.
> 2. New procedure pg_wal_replay_wal_status() (or some better name?),
> which returns status to the user instead of throwing errors.
>
> If no objections, I will push the patch moving code then go ahead
> writing the two patches above.
I attempted to implement the patchset as promised. The 0001 is easy
and straighforward. The 0002 became tricky. Procedures can't return
values. They can have OUTPUT parameters instead. However, even for
output parameters you need to pass something in, and that couldn't be
a default value. Additional difficulty is that having OUTPUT
parameters seem to hold additional snapshot and prevents our
snapshot-releasing trick from working
smagen@postgres=# CALL pg_wal_replay_wait('0/0'::pg_lsn);
CALL
Time: 2.061 ms
smagen@postgres=# CALL pg_wal_replay_wait_status(NULL, '0/0'::pg_lsn);
ERROR: pg_wal_replay_wait_status() must be only called without an
active or registered snapshot
DETAIL: Make sure pg_wal_replay_wait_status() isn't called within a
transaction with an isolation level higher than READ COMMITTED,
another procedure, or a function.
Time: 1.175 ms
I'm thinking about following solution:
1. pg_wal_replay_wait_no_error() procedure, which doesn't return
anything but throws no errors.
2. Make pg_wal_replay_wait()/pg_wal_replay_wait_no_error() save
waiting result status to the local variable.
3. New function pg_wal_replay_wal_get_status() displaying the result
status of the last pg_wal_replay_wait()/pg_wal_replay_wait_no_error()
CALL.
Then one could do.
CALL pg_wal_replay_wait_no_error(...);
SELECT pg_wal_replay_wal_get_status();
Probably looks a bit excessive, but probably the best we can do.
--
Regards,
Alexander Korotkov
Supabase
v1-0002-Attempt-to-implement-pg_wal_replay_wait_status.patch
Description: Binary data
v1-0001-Refactor-WaitForLSNReplay-to-return-the-result-of.patch
Description: Binary data
pgsql: Add UpgradeTaskProcessCB to typedefs.list
Add UpgradeTaskProcessCB to typedefs.list While it doesn't directly influence indentation right now, add it for uniformity. Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/4c57facbb16ec5f7002f0b87427897f6f9fa49ae Modified Files -- src/tools/pgindent/typedefs.list | 1 + 1 file changed, 1 insertion(+)
pgsql: Fix order of includes in src/bin/pg_upgrade/info.c
Fix order of includes in src/bin/pg_upgrade/info.c Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/a094e8b9e43e350798ea0c95b8967b8caf64bcbd Modified Files -- src/bin/pg_upgrade/info.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
pgsql: Move pg_wal_replay_wait() to xlogfuncs.c
Move pg_wal_replay_wait() to xlogfuncs.c This commit moves pg_wal_replay_wait() procedure to be a neighbor of WAL-related functions in xlogfuncs.c. The implementation of LSN waiting continues to reside in the same place. By proposal from Michael Paquier. Reported-by: Peter Eisentraut Discussion: https://postgr.es/m/18c0fa64-0475-415e-a1bd-665d922c5201%40eisentraut.org Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/014f9f34d2527c14cdf4150863c053d2a117c1c7 Modified Files -- src/backend/access/transam/xlogfuncs.c | 56 ++ src/backend/commands/waitlsn.c | 52 +-- src/include/commands/waitlsn.h | 1 + 3 files changed, 58 insertions(+), 51 deletions(-)
Re: pgsql: pg_upgrade: Parallelize retrieving relation information.
On Tue, Sep 17, 2024 at 12:11 AM Nathan Bossart wrote: > pg_upgrade: Parallelize retrieving relation information. > > This commit makes use of the new task framework in pg_upgrade to > parallelize retrieving relation and logical slot information. This > step will now process multiple databases concurrently when > pg_upgrade's --jobs option is provided a value greater than 1. > > Reviewed-by: Daniel Gustafsson, Ilya Gladyshev > Discussion: https://postgr.es/m/20240516211638.GA1688936%40nathanxps13 #include "access/transam.h" #include "catalog/pg_class_d.h" +#include "pqexpbuffer.h" #include "pg_upgrade.h" I think the alphabetic order of includes needs fixing. -- Regards, Alexander Korotkov Supabase
Re: pgsql: Introduce framework for parallelizing various pg_upgrade tasks.
Hi! On Tue, Sep 17, 2024 at 12:11 AM Nathan Bossart wrote: > Introduce framework for parallelizing various pg_upgrade tasks. > > A number of pg_upgrade steps require connecting to every database > in the cluster and running the same query in each one. When there > are many databases, these steps are particularly time-consuming, > especially since they are performed sequentially, i.e., we connect > to a database, run the query, and process the results before moving > on to the next database. > > This commit introduces a new framework that makes it easy to > parallelize most of these once-in-each-database tasks by processing > multiple databases concurrently. This framework manages a set of > slots that follow a simple state machine, and it uses libpq's > asynchronous APIs to establish the connections and run the queries. > The --jobs option is used to determine the number of slots to use. > To use this new task framework, callers simply need to provide the > query and a callback function to process its results, and the > framework takes care of the rest. A more complete description is > provided at the top of the new task.c file. > > None of the eligible once-in-each-database tasks are converted to > use this new framework in this commit. That will be done via > several follow-up commits. > > Reviewed-by: Jeff Davis, Robert Haas, Daniel Gustafsson, Ilya Gladyshev, > Corey Huinker > Discussion: https://postgr.es/m/20240516211638.GA1688936%40nathanxps13 Should we add UpgradeTaskProcessCB to the typedefs.list? I don't see this would directly influence indentation right now, but probably we should do for uniformity? -- Regards, Alexander Korotkov Supabase
pgsql: Ensure standby promotion point in 043_wal_replay_wait.pl
Ensure standby promotion point in 043_wal_replay_wait.pl This commit ensures standby will be promoted at least at the primary insert LSN we have just observed. We use pg_switch_wal() to force the insert LSN to be written then wait for standby to catchup. Reported-by: Alexander Lakhin Discussion: https://postgr.es/m/1d7b08f2-64a2-77fb-c666-c9a74c68eeda%40gmail.com Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/2520226c953c0b443791a185a8d1fb8b71d9fe9e Modified Files -- src/test/recovery/t/043_wal_replay_wait.pl | 6 ++ 1 file changed, 6 insertions(+)
pgsql: Minor cleanup related to pg_wal_replay_wait() procedure
Minor cleanup related to pg_wal_replay_wait() procedure * Rename $node_standby1 to $node_standby in 043_wal_replay_wait.pl as there is only one standby. * Remove useless debug printing in 043_wal_replay_wait.pl. * Fix typo in one check description in 043_wal_replay_wait.pl. * Fix some wording in comments and documentation. Reported-by: Alexander Lakhin Discussion: https://postgr.es/m/1d7b08f2-64a2-77fb-c666-c9a74c68eeda%40gmail.com Reviewed-by: Alexander Lakhin Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/85b98b8d5a48092acd03db76a8bf02d9c38c1d79 Modified Files -- doc/src/sgml/func.sgml | 2 +- src/backend/access/transam/xlog.c | 2 +- src/backend/commands/waitlsn.c | 4 +-- src/test/recovery/t/043_wal_replay_wait.pl | 48 ++ 4 files changed, 27 insertions(+), 29 deletions(-)
Re: pgsql: Implement pg_wal_replay_wait() stored procedure
On Tue, Sep 3, 2024 at 4:07 PM Alexander Korotkov wrote: > If no objections, I will push the patch moving code then go ahead > writing the two patches above. The patch for code movement missed couple of includes. Revised version is attached. -- Regards, Alexander Korotkov Supabase v2-0001-Move-pg_wal_replay_wait-to-xlogfuncs.c.patch Description: Binary data
Re: pgsql: Implement pg_wal_replay_wait() stored procedure
Hi, Michael! On Mon, Sep 2, 2024 at 3:25 AM Michael Paquier wrote: > > On Mon, Sep 02, 2024 at 02:55:50AM +0300, Alexander Korotkov wrote: > > Could you, please, check the attached patch? > > The patch moving the code looks correct at quick glance. Thank you for your feedback. > Now, I've been staring at this line, wondering why this is required > while WaitForLSNReplay() does not return any status: > + (void) WaitForLSNReplay(target_lsn, timeout); > > And this makes me question whether you have the right semantics > regarding the SQL function itself. Could it be more useful for > WaitForLSNReplay() to report an enum state that tells you the reason > why a wait may not have happened with a text or a tuple returned by > the function? There are quite a few reasons why a wait may or may not > happen: > - Recovery has ended, target LSN has been reached. > - We're not in recovery anymore. Is an error the most useful thing > here actually? > - Timeout may have been reached, where an error is also generated. > ERRCODE_QUERY_CANCELED is not a correct error state. > - Perhaps more of these in the future? > > My point is: this is a feature that can be used for monitoring as far > as I know, still it does not stick as a feature that could be most > useful for such applications. Wouldn't it be more useful for client > applications to deal with a state returned by the SQL function rather > than having to parse error strings to know what happened? What is > returned by pg_wal_replay_wal() reflects on the design of > WaitForLSNReplay() itself. By design, pg_wal_replay_wal() should be followed up with read-only query to standby. The read-only query gets guarantee that it's executed after the replay of LSN specified as pg_wal_replay_wal() design. Returning the status from pg_wal_replay_wal() would require additional cycle of its processing. But one of the main objectives of this feature was reducing roundtrips during waiting for LSN. On the other hand, I see that returning status could make sense for certain use cases. I think I could write two patches to provide that. 1. Make WaitForLSNReplay() return status, and make pg_wal_replay_wal() be responsible for throwing all the errors. 2. New procedure pg_wal_replay_wal_status() (or some better name?), which returns status to the user instead of throwing errors. If no objections, I will push the patch moving code then go ahead writing the two patches above. -- Regards, Alexander Korotkov Supabase
Re: pgsql: Implement pg_wal_replay_wait() stored procedure
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? -- Regards, Alexander Korotkov Supabase v1-0001-Move-pg_wal_replay_wait-to-xlogfuncs.c.patch Description: Binary data
Re: pgsql: Implement pg_wal_replay_wait() stored procedure
On Fri, Aug 30, 2024 at 10:42 PM Peter Eisentraut wrote: > On 02.08.24 20:22, Alexander Korotkov wrote: > > Implement pg_wal_replay_wait() stored procedure > > Why is this under src/backend/command/? Wouldn't it belong under > src/backend/utils/adt/? 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? -- Regards, Alexander Korotkov Supabase
pgsql: Revert: Avoid looping over all type cache entries in TypeCacheRe
Revert: Avoid looping over all type cache entries in TypeCacheRelCallback() This commit reverts c14d4acb8 as the patch design didn't take into account that TypeCacheEntry could be invalidated during the lookup_type_cache() call. Reported-by: Alexander Lakhin Discussion: https://postgr.es/m/1927cba4-177e-5c23-cbcc-d444a850304f%40gmail.com Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/8daa62a10c911c851f7e9ec5ef7b90cfd4b73212 Modified Files -- src/backend/utils/cache/typcache.c | 275 ++--- src/tools/pgindent/typedefs.list | 1 - 2 files changed, 44 insertions(+), 232 deletions(-)
pgsql: Avoid looping over all type cache entries in TypeCacheRelCallbac
Avoid looping over all type cache entries in TypeCacheRelCallback() Currently when a single relcache entry gets invalidated, TypeCacheRelCallback() has to loop over all type cache entries to find appropriate typentry to invalidate. Unfortunately, using the syscache here is impossible, because this callback could be called outside a transaction and this makes impossible catalog lookups. This is why present commit introduces RelIdToTypeIdCacheHash to map relation OID to its composite type OID. We are keeping RelIdToTypeIdCacheHash entry while corresponding type cache entry have something to clean. Therefore, RelIdToTypeIdCacheHash shouldn't get bloat in the case of temporary tables flood. Discussion: https://postgr.es/m/5812a6e5-68ae-4d84-9d85-b443176966a1%40sigaev.ru Author: Teodor Sigaev Reviewed-by: Aleksander Alekseev, Tom Lane, Michael Paquier, Roman Zharkov Reviewed-by: Andrei Lepikhov, Pavel Borisov Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/c14d4acb81348a34497b53a19dce2924cc4f6551 Modified Files -- src/backend/utils/cache/typcache.c | 275 +++-- src/tools/pgindent/typedefs.list | 1 + 2 files changed, 232 insertions(+), 44 deletions(-)
pgsql: Revert support for ALTER TABLE ... MERGE/SPLIT PARTITION(S) comm
Revert support for ALTER TABLE ... MERGE/SPLIT PARTITION(S) commands This commit reverts 1adf16b8fb, 87c21bb941, and subsequent fixes and improvements including df64c81ca9, c99ef1811a, 9dfcac8e15, 885742b9f8, 842c9b2705, fcf80c5d5f, 96c7381c4c, f4fc7cb54b, 60ae37a8bc, 259c96fa8f, 449cdcd486, 3ca43dbbb6, 2a679ae94e, 3a82c689fd, fbd4321fd5, d53a4286d7, c086896625, 4e5d6c4091, 04158e7fa3. The reason for reverting is security issues related to repeatable name lookups (CVE-2014-0062). Even though 04158e7fa3 solved part of the problem, there are still remaining issues, which aren't feasible to even carefully analyze before the RC deadline. Reported-by: Noah Misch, Robert Haas Discussion: https://postgr.es/m/20240808171351.a9.nmisch%40google.com Backpatch-through: 17 Branch -- REL_17_STABLE Details --- https://git.postgresql.org/pg/commitdiff/84f594da358861cceeaeb7a97bb58f3765eeb284 Modified Files -- doc/src/sgml/ddl.sgml | 38 - doc/src/sgml/ref/alter_table.sgml | 165 +- src/backend/commands/tablecmds.c | 819 +- src/backend/parser/gram.y | 61 +- src/backend/parser/parse_utilcmd.c | 193 +-- src/backend/partitioning/partbounds.c | 901 +-- src/backend/tcop/utility.c |6 - src/backend/utils/adt/ruleutils.c | 18 - src/bin/psql/tab-complete.c| 18 +- src/include/nodes/parsenodes.h | 14 +- src/include/parser/kwlist.h|2 - src/include/partitioning/partbounds.h | 11 - src/include/utils/ruleutils.h |2 - src/test/isolation/expected/partition-merge.out| 199 --- src/test/isolation/expected/partition-split.out| 190 --- src/test/isolation/isolation_schedule |2 - src/test/isolation/specs/partition-merge.spec | 54 - src/test/isolation/specs/partition-split.spec | 54 - .../modules/test_ddl_deparse/test_ddl_deparse.c|6 - src/test/regress/expected/partition_merge.out | 945 src/test/regress/expected/partition_split.out | 1589 src/test/regress/parallel_schedule |2 +- src/test/regress/sql/partition_merge.sql | 609 src/test/regress/sql/partition_split.sql | 962 src/tools/pgindent/typedefs.list |2 - 25 files changed, 40 insertions(+), 6822 deletions(-)
pgsql: Revert support for ALTER TABLE ... MERGE/SPLIT PARTITION(S) comm
Revert support for ALTER TABLE ... MERGE/SPLIT PARTITION(S) commands This commit reverts 1adf16b8fb, 87c21bb941, and subsequent fixes and improvements including df64c81ca9, c99ef1811a, 9dfcac8e15, 885742b9f8, 842c9b2705, fcf80c5d5f, 96c7381c4c, f4fc7cb54b, 60ae37a8bc, 259c96fa8f, 449cdcd486, 3ca43dbbb6, 2a679ae94e, 3a82c689fd, fbd4321fd5, d53a4286d7, c086896625, 4e5d6c4091, 04158e7fa3. The reason for reverting is security issues related to repeatable name lookups (CVE-2014-0062). Even though 04158e7fa3 solved part of the problem, there are still remaining issues, which aren't feasible to even carefully analyze before the RC deadline. Reported-by: Noah Misch, Robert Haas Discussion: https://postgr.es/m/20240808171351.a9.nmisch%40google.com Backpatch-through: 17 Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/3890d90c1508125729ed20038d90513694fc3a7b Modified Files -- doc/src/sgml/ddl.sgml | 38 - doc/src/sgml/ref/alter_table.sgml | 165 +- src/backend/commands/tablecmds.c | 819 +- src/backend/parser/gram.y | 61 +- src/backend/parser/parse_utilcmd.c | 193 +-- src/backend/partitioning/partbounds.c | 901 +-- src/backend/tcop/utility.c |6 - src/backend/utils/adt/ruleutils.c | 18 - src/bin/psql/tab-complete.c| 18 +- src/include/nodes/parsenodes.h | 16 - src/include/parser/kwlist.h|2 - src/include/partitioning/partbounds.h | 11 - src/include/utils/ruleutils.h |2 - src/test/isolation/expected/partition-merge.out| 199 --- src/test/isolation/expected/partition-split.out| 190 --- src/test/isolation/isolation_schedule |2 - src/test/isolation/specs/partition-merge.spec | 54 - src/test/isolation/specs/partition-split.spec | 54 - .../modules/test_ddl_deparse/test_ddl_deparse.c|6 - src/test/regress/expected/partition_merge.out | 945 src/test/regress/expected/partition_split.out | 1589 src/test/regress/parallel_schedule |2 +- src/test/regress/sql/partition_merge.sql | 609 src/test/regress/sql/partition_split.sql | 962 src/tools/pgindent/typedefs.list |2 - 25 files changed, 36 insertions(+), 6828 deletions(-)
pgsql: Avoid repeated table name lookups in createPartitionTable()
Avoid repeated table name lookups in createPartitionTable() Currently, createPartitionTable() opens newly created table using its name. This approach is prone to privilege escalation attack, because we might end up opening another table than we just created. This commit address the issue above by opening newly created table by its OID. It appears to be tricky to get a relation OID out of ProcessUtility(). We have to extend TableLikeClause with new newRelationOid field, which is filled within ProcessUtility() to be further accessed by caller. Security: CVE-2014-0062 Reported-by: Noah Misch Discussion: https://postgr.es/m/20240808171351.a9.nmisch%40google.com Reviewed-by: Pavel Borisov, Dmitry Koval Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/04158e7fa37c2dda9c3421ca922d02807b86df19 Modified Files -- src/backend/commands/tablecmds.c | 3 ++- src/backend/parser/gram.y| 1 + src/backend/tcop/utility.c | 6 ++ src/include/nodes/parsenodes.h | 1 + 4 files changed, 10 insertions(+), 1 deletion(-)
pgsql: Avoid repeated table name lookups in createPartitionTable()
Avoid repeated table name lookups in createPartitionTable() Currently, createPartitionTable() opens newly created table using its name. This approach is prone to privilege escalation attack, because we might end up opening another table than we just created. This commit address the issue above by opening newly created table by its OID. It appears to be tricky to get a relation OID out of ProcessUtility(). We have to extend TableLikeClause with new newRelationOid field, which is filled within ProcessUtility() to be further accessed by caller. Security: CVE-2014-0062 Reported-by: Noah Misch Discussion: https://postgr.es/m/20240808171351.a9.nmisch%40google.com Reviewed-by: Pavel Borisov, Dmitry Koval Branch -- REL_17_STABLE Details --- https://git.postgresql.org/pg/commitdiff/f636ab41aba215eaa3303e21a10f12d81357f1f6 Modified Files -- src/backend/commands/tablecmds.c | 3 ++- src/backend/parser/gram.y| 1 + src/backend/tcop/utility.c | 6 ++ src/include/nodes/parsenodes.h | 1 + 4 files changed, 10 insertions(+), 1 deletion(-)
Re: pgsql: Add injection-point test for new multixact CV usage
On Tue, Aug 20, 2024 at 9:35 PM Alvaro Herrera wrote: > Add injection-point test for new multixact CV usage > > Before commit a0e0fb1ba56f, multixact.c contained a case in the > multixact-read path where it would loop sleeping 1ms each time until > another multixact-create path completed, which was uncovered by any > tests. That commit changed the code to rely on a condition variable > instead. Add a test now, which relies on injection points and "loading" > thereof (because of it being in a critical section), per commit > 4b211003ecc2. > > Author: Andrey Borodin > Reviewed-by: Michaël Paquier > Discussion: > https://postgr.es/m/0925f9a9-4d53-4b27-a87e-3d83a757b...@yandex-team.ru > > Branch > -- > master > > Details > --- > https://git.postgresql.org/pg/commitdiff/768a9fd5535fddb781088b6f83132b9a1b1f5bd3 > > Modified Files > -- > src/backend/access/transam/multixact.c| 5 ++ > src/test/modules/test_slru/Makefile | 7 +- > src/test/modules/test_slru/meson.build| 9 ++ > src/test/modules/test_slru/t/001_multixact.pl | 124 ++ > src/test/modules/test_slru/test_multixact.c | 57 > src/test/modules/test_slru/test_slru--1.0.sql | 6 ++ > 6 files changed, 207 insertions(+), 1 deletion(-) It seems that header files aren't alphabetically ordered here. #include "storage/proc.h" #include "storage/procarray.h" #include "utils/fmgrprotos.h" +#include "utils/injection_point.h" #include "utils/guc_hooks.h" #include "utils/memutils.h" -- Regards, Alexander Korotkov Supabase
pgsql: Add missing wait_for_catchup() to pg_visibility tap test
Add missing wait_for_catchup() to pg_visibility tap test e2ed7e32271a introduced check of pg_visibility on standby. This commit adds missing wait_for_catchup() to synchronize standby before querying it. Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/e3ec9dc1bf4983fcedb6f43c71ea12ee26aefc7a Modified Files -- contrib/pg_visibility/t/001_concurrent_transaction.pl | 1 + 1 file changed, 1 insertion(+)
Re: pgsql: Fix GetStrictOldestNonRemovableTransactionId() on standby
On Fri, Aug 16, 2024 at 12:19 AM Alexander Korotkov wrote: > > Fix GetStrictOldestNonRemovableTransactionId() on standby > > e85662df44 implemented GetStrictOldestNonRemovableTransactionId() function > for computation of xid horizon that avoid reporting of false errors. > However, GetStrictOldestNonRemovableTransactionId() uses > GetRunningTransactionData() even on standby leading to an assertion failure. > > Given that we decided to ignore KnownAssignedXids and standby can't have > own running xids, we switch to use TransamVariables->nextXid as a xid horizon. > > Also, revise the comment regarding ignoring KnownAssignedXids with more > detailed reasoning provided by Heikki. I see the buildfarm failures. I will try to fix them shortly. -- Regards, Alexander Korotkov Supabase
pgsql: Fix GetStrictOldestNonRemovableTransactionId() on standby
Fix GetStrictOldestNonRemovableTransactionId() on standby e85662df44 implemented GetStrictOldestNonRemovableTransactionId() function for computation of xid horizon that avoid reporting of false errors. However, GetStrictOldestNonRemovableTransactionId() uses GetRunningTransactionData() even on standby leading to an assertion failure. Given that we decided to ignore KnownAssignedXids and standby can't have own running xids, we switch to use TransamVariables->nextXid as a xid horizon. Also, revise the comment regarding ignoring KnownAssignedXids with more detailed reasoning provided by Heikki. Reported-by: Heikki Linnakangas Discussion: https://postgr.es/m/42218c4f-2c8d-40a3-8743-4d34dd0e4cce%40iki.fi Reviewed-by: Heikki Linnakangas Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/e2ed7e32271a82179c3f8c7c93ce52ff93c6dd3c Modified Files -- contrib/pg_visibility/pg_visibility.c | 26 +++--- .../pg_visibility/t/001_concurrent_transaction.pl | 19 ++-- 2 files changed, 40 insertions(+), 5 deletions(-)
Re: pgsql: Fix more holes with SLRU code in need of int64 for segment numbe
On Thu, Aug 15, 2024 at 2:07 AM Michael Paquier wrote: > > On Aug 15, 2024, at 6:26, Alexander Korotkov wrote: > > I mean, I'd like to push it if you don't object. > > If you object and like to push it yourself, feel free to use this patch. > > I’d like to take a look at what you have here to get an opinion. Could you > wait for a few days? Sure, NP. -- Regards, Alexander Korotkov Supabase
Re: pgsql: Fix more holes with SLRU code in need of int64 for segment numbe
On Wed, Aug 14, 2024 at 5:13 AM Alexander Korotkov wrote: > On Tue, Aug 13, 2024 at 1:30 AM Michael Paquier wrote: > > > On Aug 13, 2024, at 6:35, Alexander Korotkov > > > wrote:. > > > > > > As pointed by Noah Misch [1], unlike the commit the patch [2] also > > > changed segment-returning functions to return int64. Thus, in the > > > patch output formats make much more sense, because they match the > > > input data types. Michael, are you intended to push the remaining > > > part of the patch [2]? > > > > Guess so. I could look at that next week, not before. > > OK. I made an attached patch from the missing parts. > Do you mind if I push that? If yes, feel free to use it. I mean, I'd like to push it if you don't object. If you object and like to push it yourself, feel free to use this patch. -- Regards, Alexander Korotkov Supabase
Re: pgsql: Fix more holes with SLRU code in need of int64 for segment numbe
On Tue, Aug 13, 2024 at 1:30 AM Michael Paquier wrote: > > On Aug 13, 2024, at 6:35, Alexander Korotkov wrote:. > > > > As pointed by Noah Misch [1], unlike the commit the patch [2] also > > changed segment-returning functions to return int64. Thus, in the > > patch output formats make much more sense, because they match the > > input data types. Michael, are you intended to push the remaining > > part of the patch [2]? > > Guess so. I could look at that next week, not before. OK. I made an attached patch from the missing parts. Do you mind if I push that? If yes, feel free to use it. -- Regards, Alexander Korotkov Supabase v1-0001-Fix-even-more-holes-with-SLRU-code-in-need-of-int.patch Description: Binary data
Re: pgsql: Fix more holes with SLRU code in need of int64 for segment numbe
On Fri, Aug 9, 2024 at 4:09 PM Peter Eisentraut wrote: > On 08.08.24 01:15, Michael Paquier wrote: > > > >> On Aug 8, 2024, at 5:05, Alexander Korotkov wrote: > >> On Wed, Aug 7, 2024 at 10:52 PM Peter Eisentraut > >> wrote: > >>> It looks like the commit I'm talking about here is a subset of v55-0001 > >>> from that thread? > >> > >> Yes, looks like this. > >> > >>> So why is some of this being committed now into v17? > >>> But as I wrote above, I think this approach is a bad idea. > >> > >> OK, I agree that might look annoying. So, it's better to revert now. > >> Michael, what do you think? > > > > The argument is two-fold here. The point of this change is that we were > > forcibly doing a cast to int with int64 values returned, so this commit > > limits the risks of missing paths in the future, while being consistent > > with all the SLRU code marking segment numbers with int64 for short *and* > > long segment file names. > > No, this is not what *this* patch does. (I suppose some of the related > patches might be doing that.) This patch just casts a few things that > are int to unsigned long long int before printing them. As pointed by Noah Misch [1], unlike the commit the patch [2] also changed segment-returning functions to return int64. Thus, in the patch output formats make much more sense, because they match the input data types. Michael, are you intended to push the remaining part of the patch [2]? Links 1. https://www.postgresql.org/message-id/20240810175055.cd.nmisch%40google.com 2. https://www.postgresql.org/message-id/ZqGvzSbW5TGKqZcE%40paquier.xyz -- Regards, Alexander Korotkov Supabase
pgsql: Add tests for pg_wal_replay_wait() errors
Add tests for pg_wal_replay_wait() errors Improve test coverage for pg_wal_replay_wait() procedure by adding test cases when it errors out. Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/0868d7ae70e57f4eeba43792db92ce2d9fe4340d Modified Files -- src/test/recovery/t/043_wal_replay_wait.pl | 42 -- 1 file changed, 40 insertions(+), 2 deletions(-)
pgsql: Adjust pg_wal_replay_wait() procedure behavior on promoted stand
Adjust pg_wal_replay_wait() procedure behavior on promoted standby pg_wal_replay_wait() is intended to be called on standby. However, standby can be promoted to primary at any moment, even concurrently with the pg_wal_replay_wait() call. If recovery is not currently in progress that doesn't mean the wait was unsuccessful. Thus, we always need to recheck if the target LSN is replayed. Reported-by: Kevin Hale Boyes Discussion: https://postgr.es/m/CAPpHfdu5QN%2BZGACS%2B7foxmr8_nekgA2PA%2B-G3BuOUrdBLBFb6Q%40mail.gmail.com Author: Alexander Korotkov Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/867d396ccd2a7f0ce55e1fa7ebda00bc8c81147b Modified Files -- doc/src/sgml/func.sgml | 9 +++ src/backend/commands/waitlsn.c | 42 +++--- src/test/recovery/t/043_wal_replay_wait.pl | 15 +-- 3 files changed, 55 insertions(+), 11 deletions(-)
pgsql: Improve header comment for WaitLSNSetLatches()
Improve header comment for WaitLSNSetLatches() Reflect the fact that we remove waiters from the heap, not just set their latches. Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/3ac3ec580c6f4f991d32252814e4b04c0e903a41 Modified Files -- src/backend/commands/waitlsn.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
Re: pgsql: Introduce hash_search_with_hash_value() function
On Thu, Aug 8, 2024 at 7:45 AM Pavel Stehule wrote: > st 7. 8. 2024 v 22:25 odesílatel Alexander Korotkov > napsal: >> >> On Wed, Aug 7, 2024 at 1:34 PM Pavel Stehule wrote: >> > st 7. 8. 2024 v 12:22 odesílatel Alexander Korotkov >> > napsal: >> >> Thank you. >> >> Please see 40064a8ee1 takes special efforts to match HTAB hash >> >> function to syscache hash function. By default, their hash values >> >> don't match and you can't simply use syscache hash value to search >> >> HTAB. This probably should be mentioned in the header comment of >> >> hash_seq_init_with_hash_value(). >> > >> > >> > yes, enhancing doc should be great + maybe assert >> >> Please check the patch, which adds a caveat to the function header >> comment. I don't particularly like an assert here, because there >> could be use-cases besides syscache callbacks, which could legally use >> default hash function. > > > looks well Thank you, pushed. -- Regards, Alexander Korotkov Supabase
pgsql: Add a caveat to hash_seq_init_with_hash_value() header comment
Add a caveat to hash_seq_init_with_hash_value() header comment The typical use-case for hash_seq_init_with_hash_value() is syscache callback. Add a caveat that the default hash function doesn't match syscache hash function. So, one needs to define a custom hash function. Reported-by: Pavel Stehule Discussion: https://postgr.es/m/CAFj8pRAXmv6eyYx%3DE_BTfyK%3DO_%2ByOF8sXB%3D0bn9eOBt90EgWRA%40mail.gmail.com Reviewed-by: Pavel Stehule Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/d0c8cf2a56fadb08705433bffb301559d97b0712 Modified Files -- src/backend/utils/hash/dynahash.c | 5 + 1 file changed, 5 insertions(+)
Re: pgsql: Introduce hash_search_with_hash_value() function
On Wed, Aug 7, 2024 at 1:34 PM Pavel Stehule wrote: > st 7. 8. 2024 v 12:22 odesílatel Alexander Korotkov > napsal: >> Thank you. >> Please see 40064a8ee1 takes special efforts to match HTAB hash >> function to syscache hash function. By default, their hash values >> don't match and you can't simply use syscache hash value to search >> HTAB. This probably should be mentioned in the header comment of >> hash_seq_init_with_hash_value(). > > > yes, enhancing doc should be great + maybe assert Please check the patch, which adds a caveat to the function header comment. I don't particularly like an assert here, because there could be use-cases besides syscache callbacks, which could legally use default hash function. -- Regards, Alexander Korotkov Supabase v1-0001-Add-a-caveat-to-hash_seq_init_with_hash_value-hea.patch Description: Binary data
Re: pgsql: Introduce hash_search_with_hash_value() function
On Wed, Aug 7, 2024 at 7:30 PM Robert Haas wrote: > On Wed, Aug 7, 2024 at 11:55 AM Alexander Korotkov > wrote: > > On Wed, Aug 7, 2024 at 3:24 PM Robert Haas wrote: > > > You may have already realized this, but the name of the function the > > > patch adds is not the same as the name that appears in the commit > > > message. > > > > :sigh: > > I didn't realize that before your message. That would be another item > > for my checklist: ensure entities referenced from commit message and > > comments didn't change their names. > > I really wish there was some way to fix commit messages. I had a typo > in mine today, too. +1, One of the scariest things that happened to me was forgetting to mention reviewers or even authors. People don't get credit for their work, and you can't fix that. -- Regards, Alexander Korotkov Supabase
Re: pgsql: Fix more holes with SLRU code in need of int64 for segment numbe
On Wed, Aug 7, 2024 at 10:52 PM Peter Eisentraut wrote: > On 07.08.24 17:53, Alexander Korotkov wrote: > > On Wed, Aug 7, 2024 at 12:07 PM Peter Eisentraut > > wrote: > >> On 27.07.24 00:24, Michael Paquier wrote: > >>> Fix more holes with SLRU code in need of int64 for segment numbers > >>> > >>> This is a continuation of 3937cadfd438, taking care of more areas I have > >>> managed to miss previously. > >>> > >>> Reported-by: Noah Misch > >>> Reviewed-by: Noah Misch > >>> Discussion: https://postgr.es/m/20240724130059.1f.nmi...@google.com > >>> Backpatch-through: 17 > >>> > >>> Branch > >>> -- > >>> master > >>> > >>> Details > >>> --- > >>> https://git.postgresql.org/pg/commitdiff/c9e24573905bef7fc3e4efb02bdb4d0cc8e43c51 > >> > >> I don't understand this patch. The previous patches that this > >> references changed various variables to int64 and made adjustments > >> following from that. But this patch takes variables and function > >> results that are of type int and casts them to unsigned long long before > >> printing. I don't see what that accomplishes, and it's not clear based > >> on just the explanation that this is a continuation of a previous patch > >> that doesn't do that. Is there a plan to change these things to int64 > >> as well at some point? > > > > There is a plan indeed. The patchset [1] should include conversion > > multixacts to 64-bit (It surely included that in earlier versions, I > > didn't look the last versions though). I doubt this will be ready for > > v18. So this commit might be quite preliminary. But I would prefer > > to leave it there as soon as it has already landed. Opinions? > > I think you should change the output formats at the same time as you > change the variable types. That way the compiler can cross-check this. > Otherwise, if you later forget to change a variable, these casts will > hide it. Or if the future patches turn out differently, then we have > this useless code. > > > Links. > > 1. > > https://www.postgresql.org/message-id/CAJ7c6TND0bCnwU1SmxTsFewK4XJGBep343vf%2BT%2BGQ-a5S5hC0w%40mail.gmail.com > > It looks like the commit I'm talking about here is a subset of v55-0001 > from that thread? Yes, looks like this. > So why is some of this being committed now into v17? > But as I wrote above, I think this approach is a bad idea. OK, I agree that might look annoying. So, it's better to revert now. Michael, what do you think? -- Regards, Alexander Korotkov Supabase
Re: pgsql: Introduce hash_search_with_hash_value() function
On Wed, Aug 7, 2024 at 3:24 PM Robert Haas wrote: > You may have already realized this, but the name of the function the > patch adds is not the same as the name that appears in the commit > message. :sigh: I didn't realize that before your message. That would be another item for my checklist: ensure entities referenced from commit message and comments didn't change their names. -- Regards, Alexander Korotkov Supabase
Re: pgsql: Fix more holes with SLRU code in need of int64 for segment numbe
On Wed, Aug 7, 2024 at 12:07 PM Peter Eisentraut wrote: > On 27.07.24 00:24, Michael Paquier wrote: > > Fix more holes with SLRU code in need of int64 for segment numbers > > > > This is a continuation of 3937cadfd438, taking care of more areas I have > > managed to miss previously. > > > > Reported-by: Noah Misch > > Reviewed-by: Noah Misch > > Discussion: https://postgr.es/m/20240724130059.1f.nmi...@google.com > > Backpatch-through: 17 > > > > Branch > > -- > > master > > > > Details > > --- > > https://git.postgresql.org/pg/commitdiff/c9e24573905bef7fc3e4efb02bdb4d0cc8e43c51 > > I don't understand this patch. The previous patches that this > references changed various variables to int64 and made adjustments > following from that. But this patch takes variables and function > results that are of type int and casts them to unsigned long long before > printing. I don't see what that accomplishes, and it's not clear based > on just the explanation that this is a continuation of a previous patch > that doesn't do that. Is there a plan to change these things to int64 > as well at some point? There is a plan indeed. The patchset [1] should include conversion multixacts to 64-bit (It surely included that in earlier versions, I didn't look the last versions though). I doubt this will be ready for v18. So this commit might be quite preliminary. But I would prefer to leave it there as soon as it has already landed. Opinions? Links. 1. https://www.postgresql.org/message-id/CAJ7c6TND0bCnwU1SmxTsFewK4XJGBep343vf%2BT%2BGQ-a5S5hC0w%40mail.gmail.com -- Regards, Alexander Korotkov Supabase
Re: pgsql: Introduce hash_search_with_hash_value() function
On Wed, Aug 7, 2024 at 1:03 PM Pavel Stehule wrote:
>
> st 7. 8. 2024 v 10:52 odesílatel Alexander Korotkov
> napsal:
>>
>> Hi, Pavel!
>>
>> On Wed, Aug 7, 2024 at 9:35 AM Pavel Stehule wrote:
>> > st 7. 8. 2024 v 6:08 odesílatel Alexander Korotkov
>> > napsal:
>> >>
>> >> Introduce hash_search_with_hash_value() function
>> >>
>> >> This new function iterates hash entries with given hash values. This
>> >> function
>> >> is designed to avoid full sequential hash search in the syscache
>> >> invalidation
>> >> callbacks.
>> >>
>> >> Discussion:
>> >> https://postgr.es/m/5812a6e5-68ae-4d84-9d85-b443176966a1%40sigaev.ru
>> >> Author: Teodor Sigaev
>> >> Reviewed-by: Aleksander Alekseev, Tom Lane, Michael Paquier, Roman Zharkov
>> >> Reviewed-by: Andrei Lepikhov
>> >
>> >
>> > I tried to use hash_seq_init_with_hash_value in session variables patch,
>> > but it doesn't work there.
>> >
>> > <-->if (!sessionvars)
>> > <--><-->return;
>> >
>> > elog(NOTICE, "%u", hashvalue);
>> >
>> >
>> > <-->hash_seq_init(&status, sessionvars);
>> >
>> > <-->while ((svar = (SVariable) hash_seq_search(&status)) != NULL)
>> > <-->{
>> > <--><-->if (hashvalue == 0 || svar->hashvalue == hashvalue)
>> > <--><-->{
>> > <--><--><-->elog(NOTICE, "FOUND OLD");
>> > <--><--><-->svar->is_valid = false;
>> > <--><-->}
>> > <-->}
>> >
>> >
>> >
>> > <-->/*
>> > <--> * If the hashvalue is not specified, we have to recheck all currently
>> > <--> * used session variables. Since we can't tell the exact session
>> > variable
>> > <--> * from its hashvalue, we have to iterate over all items in the hash
>> > bucket.
>> > <--> */
>> > <-->if (hashvalue == 0)
>> > <--><-->hash_seq_init(&status, sessionvars);
>> > <-->else
>> > <--><-->hash_seq_init_with_hash_value(&status, sessionvars, hashvalue);
>> >
>> > <-->while ((svar = (SVariable) hash_seq_search(&status)) != NULL)
>> > <-->{
>> > <--><-->Assert(hashvalue == 0 || svar->hashvalue == hashvalue);
>> >
>> > elog(NOTICE, "found");
>> >
>> > <--><-->svar->is_valid = false;
>> > <--><-->needs_validation = true;
>> > <-->}
>> > }
>> >
>> > Old methods found an entry, but new not.
>> >
>> > What am I doing wrong?
>>
>> I'm trying to check this. Applying this patch [1], but got conflicts.
>> Could you please, rebase the patch, so I can recheck the issue?
>
> I sent rebased patchset
Thank you.
Please see 40064a8ee1 takes special efforts to match HTAB hash
function to syscache hash function. By default, their hash values
don't match and you can't simply use syscache hash value to search
HTAB. This probably should be mentioned in the header comment of
hash_seq_init_with_hash_value().
--
Regards,
Alexander Korotkov
Supabase
Re: pgsql: Introduce hash_search_with_hash_value() function
Hi, Pavel!
On Wed, Aug 7, 2024 at 9:35 AM Pavel Stehule wrote:
> st 7. 8. 2024 v 6:08 odesílatel Alexander Korotkov
> napsal:
>>
>> Introduce hash_search_with_hash_value() function
>>
>> This new function iterates hash entries with given hash values. This
>> function
>> is designed to avoid full sequential hash search in the syscache invalidation
>> callbacks.
>>
>> Discussion:
>> https://postgr.es/m/5812a6e5-68ae-4d84-9d85-b443176966a1%40sigaev.ru
>> Author: Teodor Sigaev
>> Reviewed-by: Aleksander Alekseev, Tom Lane, Michael Paquier, Roman Zharkov
>> Reviewed-by: Andrei Lepikhov
>
>
> I tried to use hash_seq_init_with_hash_value in session variables patch, but
> it doesn't work there.
>
> <-->if (!sessionvars)
> <--><-->return;
>
> elog(NOTICE, "%u", hashvalue);
>
>
> <-->hash_seq_init(&status, sessionvars);
>
> <-->while ((svar = (SVariable) hash_seq_search(&status)) != NULL)
> <-->{
> <--><-->if (hashvalue == 0 || svar->hashvalue == hashvalue)
> <--><-->{
> <--><--><-->elog(NOTICE, "FOUND OLD");
> <--><--><-->svar->is_valid = false;
> <--><-->}
> <-->}
>
>
>
> <-->/*
> <--> * If the hashvalue is not specified, we have to recheck all currently
> <--> * used session variables. Since we can't tell the exact session variable
> <--> * from its hashvalue, we have to iterate over all items in the hash
> bucket.
> <--> */
> <-->if (hashvalue == 0)
> <--><-->hash_seq_init(&status, sessionvars);
> <-->else
> <--><-->hash_seq_init_with_hash_value(&status, sessionvars, hashvalue);
>
> <-->while ((svar = (SVariable) hash_seq_search(&status)) != NULL)
> <-->{
> <--><-->Assert(hashvalue == 0 || svar->hashvalue == hashvalue);
>
> elog(NOTICE, "found");
>
> <--><-->svar->is_valid = false;
> <--><-->needs_validation = true;
> <-->}
> }
>
> Old methods found an entry, but new not.
>
> What am I doing wrong?
I'm trying to check this. Applying this patch [1], but got conflicts.
Could you please, rebase the patch, so I can recheck the issue?
Links.
1.
https://www.postgresql.org/message-id/flat/CAFj8pRD053CY_N4%3D6SvPe7ke6xPbh%3DK50LUAOwjC3jm8Me9Obg%40mail.gmail.com
--
Regards,
Alexander Korotkov
Supabase
pgsql: Optimize InvalidateAttoptCacheCallback() and TypeCacheTypCallbac
Optimize InvalidateAttoptCacheCallback() and TypeCacheTypCallback() These callbacks are receiving hash values as arguments, which doesn't allow direct lookups for AttoptCacheHash and TypeCacheHash. This is why subject callbacks currently use full iteration over corresponding hashes. This commit avoids full hash iteration in InvalidateAttoptCacheCallback(), and TypeCacheTypCallback(). At first, we switch AttoptCacheHash and TypeCacheHash to use same hash function as syscache. As second, we use hash_seq_init_with_hash_value() to iterate only hash entries with matching hash value. Discussion: https://postgr.es/m/5812a6e5-68ae-4d84-9d85-b443176966a1%40sigaev.ru Author: Teodor Sigaev Reviewed-by: Aleksander Alekseev, Tom Lane, Michael Paquier, Roman Zharkov Reviewed-by: Andrei Lepikhov Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/40064a8ee1b34d8a128d6007416acd89077a2c11 Modified Files -- src/backend/utils/cache/attoptcache.c | 39 + src/backend/utils/cache/typcache.c| 55 +-- 2 files changed, 73 insertions(+), 21 deletions(-)
pgsql: Introduce hash_search_with_hash_value() function
Introduce hash_search_with_hash_value() function This new function iterates hash entries with given hash values. This function is designed to avoid full sequential hash search in the syscache invalidation callbacks. Discussion: https://postgr.es/m/5812a6e5-68ae-4d84-9d85-b443176966a1%40sigaev.ru Author: Teodor Sigaev Reviewed-by: Aleksander Alekseev, Tom Lane, Michael Paquier, Roman Zharkov Reviewed-by: Andrei Lepikhov Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/d0f020037e19c33c74d683eb7e0c7cc5725294b4 Modified Files -- src/backend/utils/hash/dynahash.c | 38 ++ src/include/utils/hsearch.h | 5 + 2 files changed, 43 insertions(+)
pgsql: pg_wal_replay_wait(): Fix typo in the doc
pg_wal_replay_wait(): Fix typo in the doc Reported-by: Kevin Hale Boyes Discussion: https://postgr.es/m/CADAecHWKpaPuPGXAMOH%3DwmhTpydHWGPOk9KWX97UYhp5GdqCWw%40mail.gmail.com Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/8036d73ae3d4014a9dde21b0746dc1ac139d4dc1 Modified Files -- doc/src/sgml/func.sgml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
pgsql: Implement pg_wal_replay_wait() stored procedure
Implement pg_wal_replay_wait() stored procedure pg_wal_replay_wait() is to be used on standby and specifies waiting for the specific WAL location to be replayed. This option is useful when the user makes some data changes on primary and needs a guarantee to see these changes are on standby. The queue of waiters is stored in the shared memory as an LSN-ordered pairing heap, where the waiter with the nearest LSN stays on the top. During the replay of WAL, waiters whose LSNs have already been replayed are deleted from the shared memory pairing heap and woken up by setting their latches. pg_wal_replay_wait() needs to wait without any snapshot held. Otherwise, the snapshot could prevent the replay of WAL records, implying a kind of self-deadlock. This is why it is only possible to implement pg_wal_replay_wait() as a procedure working without an active snapshot, not a function. Catversion is bumped. Discussion: https://postgr.es/m/eb12f9b03851bb2583adab5df9579b4b%40postgrespro.ru Author: Kartyshov Ivan, Alexander Korotkov Reviewed-by: Michael Paquier, Peter Eisentraut, Dilip Kumar, Amit Kapila Reviewed-by: Alexander Lakhin, Bharath Rupireddy, Euler Taveira Reviewed-by: Heikki Linnakangas, Kyotaro Horiguchi Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/3c5db1d6b01642bcd8dbf5e34b68f034365747bb Modified Files -- doc/src/sgml/func.sgml | 117 src/backend/access/transam/xact.c | 6 + src/backend/access/transam/xlog.c | 7 + src/backend/access/transam/xlogrecovery.c | 11 + src/backend/catalog/system_functions.sql| 3 + src/backend/commands/Makefile | 3 +- src/backend/commands/meson.build| 1 + src/backend/commands/waitlsn.c | 363 src/backend/lib/pairingheap.c | 18 +- src/backend/storage/ipc/ipci.c | 3 + src/backend/storage/lmgr/proc.c | 6 + src/backend/tcop/pquery.c | 9 +- src/backend/utils/activity/wait_event_names.txt | 2 + src/include/catalog/catversion.h| 2 +- src/include/catalog/pg_proc.dat | 6 + src/include/commands/waitlsn.h | 80 ++ src/include/lib/pairingheap.h | 3 + src/include/storage/lwlocklist.h| 1 + src/test/recovery/meson.build | 1 + src/test/recovery/t/043_wal_replay_wait.pl | 150 ++ src/tools/pgindent/typedefs.list| 2 + 21 files changed, 786 insertions(+), 8 deletions(-)
pgsql: amcheck: Optimize speed of checking for unique constraint violat
amcheck: Optimize speed of checking for unique constraint violation Currently, when amcheck validates a unique constraint, it visits the heap for each index tuple. This commit implements skipping keys, which have only one non-dedeuplicated index tuple (quite common case for unique indexes). That gives substantial economy on index checking time. Reported-by: Noah Misch Discussion: https://postgr.es/m/20240325020323.fd.nmisch%40google.com Author: Alexander Korotkov, Pavel Borisov Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/cdd6ab9d1f5396ec1097d51c21a224aa41118c9c Modified Files -- contrib/amcheck/verify_nbtree.c | 36 +--- 1 file changed, 33 insertions(+), 3 deletions(-)
Re: pgsql: Wait for WAL summarization to catch up before creating .partial
On Sun, Jul 28, 2024 at 7:12 AM Tom Lane wrote: > Alexander Korotkov writes: > > It appears that I was late with my review [1]. But the new tap test > > could still use pgperltidy. > > I believe our current policy is that we're asking committers to > maintain pgindent cleanliness, but not pgperltidy (which is why > BF member koel isn't checking pgperltidy). We might get to that > eventually, but we're not there yet. Got it, thank you. Sorry for noise, then. -- Regards, Alexander Korotkov Supabase
Re: pgsql: Wait for WAL summarization to catch up before creating .partial
On Fri, Jul 26, 2024 at 10:01 PM Robert Haas wrote: > Wait for WAL summarization to catch up before creating .partial file. > > When a standby is promoted, CleanupAfterArchiveRecovery() may decide > to rename the final WAL file from the old timeline by adding ".partial" > to the name. If WAL summarization is enabled and this file is renamed > before its partial contents are summarized, WAL summarization breaks: > the summarizer gets stuck at that point in the WAL stream and just > errors out. > > To fix that, first make the startup process wait for WAL summarization > to catch up before renaming the file. Generally, this should be quick, > and if it's not, the user can shut off summarize_wal and try again. > To make this fix work, also teach the WAL summarizer that after a > promotion has occurred, no more WAL can appear on the previous > timeline: previously, the WAL summarizer wouldn't switch to the new > timeline until we actually started writing WAL there, but that meant > that when the startup process was waiting for the WAL summarizer, it > was waiting for an action that the summarizer wasn't yet prepared to > take. > > In the process of fixing these bugs, I realized that the logic to wait > for WAL summarization to catch up was spread out in a way that made > it difficult to reuse properly, so this code refactors things to make > it easier. > > Finally, add a test case that would have caught this bug and the > previously-fixed bug that WAL summarization sometimes needs to back up > when the timeline changes. It appears that I was late with my review [1]. But the new tap test could still use pgperltidy. Links. 1. https://www.postgresql.org/message-id/CAPpHfduW3du0W%3D3noztdaJ6evGP9gqT1AGk_rwXrqDyus1zZoQ%40mail.gmail.com -- Regards, Alexander Korotkov Supabase
pgsql: Fix typo in GetRunningTransactionData()
Fix typo in GetRunningTransactionData() e85662df44 made GetRunningTransactionData() calculate the oldest running transaction id within the current database. However, because of the typo, the new code uses oldestRunningXid instead of oldestDatabaseRunningXid in comparison before updating oldestDatabaseRunningXid. This commit fixes that issue. Reported-by: Noah Misch Discussion: https://postgr.es/m/20240630231816.bf.nmisch%40google.com Backpatch-through: 17 Branch -- REL_17_STABLE Details --- https://git.postgresql.org/pg/commitdiff/619f76cce11dc51458eb4ea81b0e48d15d0b2d07 Modified Files -- src/backend/storage/ipc/procarray.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
pgsql: Fix typo in GetRunningTransactionData()
Fix typo in GetRunningTransactionData() e85662df44 made GetRunningTransactionData() calculate the oldest running transaction id within the current database. However, because of the typo, the new code uses oldestRunningXid instead of oldestDatabaseRunningXid in comparison before updating oldestDatabaseRunningXid. This commit fixes that issue. Reported-by: Noah Misch Discussion: https://postgr.es/m/20240630231816.bf.nmisch%40google.com Backpatch-through: 17 Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/6c1af5482e6943a5f29b7f4ca773c720ec8202b0 Modified Files -- src/backend/storage/ipc/procarray.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
pgsql: Optimize memory access in GetRunningTransactionData()
Optimize memory access in GetRunningTransactionData() e85662df44 made GetRunningTransactionData() calculate the oldest running transaction id within the current database. This commit optimized this calculation by performing a cheap transaction id comparison before fetching the process database id, while the latter could cause extra cache misses. Reported-by: Noah Misch Discussion: https://postgr.es/m/20240630231816.bf.nmisch%40google.com Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/6897f0ec024582a570868939d3f34a6853374723 Modified Files -- src/backend/storage/ipc/procarray.c | 17 +++-- 1 file changed, 11 insertions(+), 6 deletions(-)
pgsql: Remove extra comment at TableAmRoutine.scan_analyze_next_block
Remove extra comment at TableAmRoutine.scan_analyze_next_block The extra comment was accidentally copied here by 6377e12a from heapam_scan_analyze_next_block(). Reported-by: Matthias van de Meent Discussion: https://postgr.es/m/CAEze2WjC5PiweG-4oe0hB_Zr59iF3tRE1gURm8w4Cg5b6JEBGw%40mail.gmail.com Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/70a845c04a47645b58f8276a6b3ab201ea8ec426 Modified Files -- src/include/access/tableam.h | 10 -- 1 file changed, 10 deletions(-)
pgsql: Add doc entry for the new GUC paramenter enable_group_by_reorder
Add doc entry for the new GUC paramenter enable_group_by_reordering 0452b461bc4 adds alternative orderings of group-by keys during the query optimization. This new feature is controlled by the new GUC parameter enable_group_by_reordering, which accidentally came without the documentation. This commit adds the missing documentation for that GUC. Reported-by: Bruce Momjian Discussion: https://postgr.es/m/ZnDx2FYlba_OafQd%40momjian.us Author: Andrei Lepikhov Reviewed-by: Pavel Borisov, Alexander Korotkov Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/82e79ee46b1c880cb7376cf4399c9883c1ddfaea Modified Files -- doc/src/sgml/config.sgml | 19 +++ 1 file changed, 19 insertions(+)
pgsql: Fix asymmetry in setting EquivalenceClass.ec_sortref
Fix asymmetry in setting EquivalenceClass.ec_sortref 0452b461bc made get_eclass_for_sort_expr() always set EquivalenceClass.ec_sortref if it's not done yet. This leads to an asymmetric situation when whoever first looks for the EquivalenceClass sets the ec_sortref. It is also counterintuitive that get_eclass_for_sort_expr() performs modification of data structures. This commit makes make_pathkeys_for_sortclauses_extended() responsible for setting EquivalenceClass.ec_sortref. Now we set the EquivalenceClass.ec_sortref's needed to explore alternative GROUP BY ordering specifically during building pathkeys by the list of grouping clauses. Discussion: https://postgr.es/m/17037754-f187-4138-8285-0e2bfebd0dea%40postgrespro.ru Reported-by: Tom Lane Author: Andrei Lepikhov Reviewed-by: Alexander Korotkov, Pavel Borisov Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/199012a3d844c6283e0ab4b1139440840a91433d Modified Files -- src/backend/optimizer/path/equivclass.c | 13 + src/backend/optimizer/path/pathkeys.c| 18 ++-- src/backend/optimizer/plan/planner.c | 16 --- src/include/optimizer/paths.h| 3 +- src/test/regress/expected/aggregates.out | 47 src/test/regress/sql/aggregates.sql | 14 ++ 6 files changed, 92 insertions(+), 19 deletions(-)
pgsql: Add invariants check to get_useful_group_keys_orderings()
Add invariants check to get_useful_group_keys_orderings() This commit introduces invariants checking of generated orderings in get_useful_group_keys_orderings() for assert-enabled builds. Discussion: https://postgr.es/m/a663f0f6-cbf6-49aa-af2e-234dc6768a07%40postgrespro.ru Reported-by: Tom Lane Author: Andrei Lepikhov Reviewed-by: Alexander Korotkov, Pavel Borisov Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/91143c03d4ca36406a53e05cd421b19e47d131d1 Modified Files -- src/backend/optimizer/path/pathkeys.c | 28 1 file changed, 28 insertions(+)
pgsql: Rename PathKeyInfo to GroupByOrdering
Rename PathKeyInfo to GroupByOrdering 0452b461bc made optimizer explore alternative orderings of group-by pathkeys. The PathKeyInfo data structure was used to store the particular ordering of group-by pathkeys and corresponding clauses. It turns out that PathKeyInfo is not the best name for that purpose. This commit renames this data structure to GroupByOrdering, and revises its comment. Discussion: https://postgr.es/m/db0fc3a4-966c-4cec-a136-94024d39212d%40postgrespro.ru Reported-by: Tom Lane Author: Andrei Lepikhov Reviewed-by: Alexander Korotkov, Pavel Borisov Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/0c1af2c35c7b456bd2fc76bbc9df5aa9c7911bde Modified Files -- src/backend/optimizer/path/pathkeys.c | 18 +- src/backend/optimizer/plan/planner.c | 8 src/include/nodes/pathnodes.h | 13 ++--- src/tools/pgindent/typedefs.list | 2 +- 4 files changed, 24 insertions(+), 17 deletions(-)
pgsql: Restore preprocess_groupclause()
Restore preprocess_groupclause() 0452b461bc made optimizer explore alternative orderings of group-by pathkeys. It eliminated preprocess_groupclause(), which was intended to match items between GROUP BY and ORDER BY. Instead, get_useful_group_keys_orderings() function generates orderings of GROUP BY elements at the time of grouping paths generation. The get_useful_group_keys_orderings() function takes into account 3 orderings of GROUP BY pathkeys and clauses: original order as written in GROUP BY, matching ORDER BY clauses as much as possible, and matching the input path as much as possible. Given that even before 0452b461b, preprocess_groupclause() could change the original order of GROUP BY clauses we don't need to consider it apart from ordering matching ORDER BY clauses. This commit restores preprocess_groupclause() to provide an ordering of GROUP BY elements matching ORDER BY before generation of paths. The new version of preprocess_groupclause() takes into account an incremental sort. The get_useful_group_keys_orderings() function now takes into 2 orderings of GROUP BY elements: the order generated preprocess_groupclause() and the order matching the input path as much as possible. Discussion: https://postgr.es/m/CAPpHfdvyWLMGwvxaf%3D7KAp-z-4mxbSH8ti2f6mNOQv5metZFzg%40mail.gmail.com Author: Alexander Korotkov Reviewed-by: Andrei Lepikhov, Pavel Borisov Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/505c008ca37c4f6f2fffcde370b5d8354c4d4dc3 Modified Files -- src/backend/optimizer/path/pathkeys.c | 55 +-- src/backend/optimizer/plan/planner.c | 108 +++--- src/include/nodes/pathnodes.h | 6 +- src/test/regress/expected/partition_aggregate.out | 6 +- 4 files changed, 108 insertions(+), 67 deletions(-)
pgsql: amcheck: Fixes for right page check during unique constraint che
amcheck: Fixes for right page check during unique constraint check * Don't forget to pfree() the right page when it's to be ignored. * Report error on unexpected non-leaf right page even if this page is not to be ignored. This restores the logic which was unintendedly changed in 97e5b0026f. Reported-by: Pavel Borisov Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/945ec4c4bca1e1c4347cd3f93135e96770ac1b4c Modified Files -- contrib/amcheck/verify_nbtree.c | 22 -- 1 file changed, 12 insertions(+), 10 deletions(-)
pgsql: Provide deterministic order for catalog queries in partition_spl
Provide deterministic order for catalog queries in partition_split.sql System catalog tables are subject to modification by parallel tests. This is the source of instability when querying them without explicit ORDER BY. This commit adds explicit ORDER BY to system catalog queries in partition_split.sql to stabilize the result. Reported-by: Tom Lane Discussion: https://postgr.es/m/695264.1716578979%40sss.pgh.pa.us Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/d53a4286d772c50ad7a8ff72ca637de613532592 Modified Files -- src/test/regress/expected/partition_split.out | 34 +-- src/test/regress/sql/partition_split.sql | 34 +-- 2 files changed, 34 insertions(+), 34 deletions(-)
pgsql: Don't copy extended statistics during MERGE/SPLIT partition oper
Don't copy extended statistics during MERGE/SPLIT partition operations When MERGE/SPLIT created new partitions, it was cloning the extended statistics of the parent table. However, extended stats on partitioned tables don't behave like indexes on partitioned tables (which exist only to create physical indexes on child tables). Rather, extended stats on a parent 1) cause extended stats to be collected and computed across the whole partition hierarchy, and 2) do not cause extended stats to be computed for the individual partitions. "CREATE TABLE ... PARTITION OF" command doesn't copy extended statistics. This commit makes createPartitionTable() behave consistently. Reported-by: Justin Pryzby Discussion: https://postgr.es/m/ZiJW1g2nbQs9ekwK%40pryzbyj2023 Author: Alexander Korotkov, Justin Pryzby Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/fbd4321fd5b4400fbbf356d686af6ad6d3208c66 Modified Files -- doc/src/sgml/ref/alter_table.sgml | 9 +++-- src/backend/commands/tablecmds.c | 8 +--- 2 files changed, 12 insertions(+), 5 deletions(-)
pgsql: Fix the name collision detection in MERGE/SPLIT partition operat
Fix the name collision detection in MERGE/SPLIT partition operations Both MERGE and SPLIT partition operations support the case when the name of the new partition matches the name of the existing partition to be merged/split. But the name collision detection doesn't always work as intended. The SPLIT partition operation finds the namespace to search for an existing partition without taking into account the parent's persistence. The MERGE partition operation checks for the name collision with simple equal() on RangeVar's simply ignoring the search_path. This commit fixes this behavior as follows. 1. The SPLIT partition operation now finds the namespace to search for an existing partition according to the parent's persistence. 2. The MERGE partition operation now checks for the name collision similarly to the SPLIT partition operation using RangeVarGetAndCheckCreationNamespace() and get_relname_relid(). Reported-by: Alexander Lakhin Discussion: https://postgr.es/m/86b4f1e3-0b5d-315c-9225-19860d64d685%40gmail.com Author: Dmitry Koval, Alexander Korotkov Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/3a82c689fd1be9bdf9d60135e5db7d352c051269 Modified Files -- src/backend/commands/tablecmds.c | 62 ++- src/test/regress/expected/partition_merge.out | 3 +- src/test/regress/expected/partition_split.out | 8 src/test/regress/sql/partition_merge.sql | 3 +- src/test/regress/sql/partition_split.sql | 9 5 files changed, 72 insertions(+), 13 deletions(-)
pgsql: amcheck: Don't load the right sibling page into BtreeCheckState
amcheck: Don't load the right sibling page into BtreeCheckState 5ae2087202 implemented a cross-page unique constraint check by loading the right sibling to the BtreeCheckState.target variable. This is wrong, because bt_target_page_check() shouldn't change the target page. Also, BtreeCheckState.target shouldn't be changed alone without BtreeCheckState.targetblock. However, the above didn't cause any visible bugs for the following reasons. 1. We do a cross-page unique constraint check only for leaf index pages. 2. The only way target page get accessed after a cross-page unique constraint check is loading of the lowkey. 3. The only place lowkey is used is bt_child_highkey_check(), and that applies only to non-leafs. The reasons above don't diminish the fact that changing BtreeCheckState.target for a cross-page unique constraint check is wrong. This commit changes this check to temporarily store the right sibling to the local variable. Reported-by: Peter Geoghegan Discussion: https://postgr.es/m/CAH2-Wzk%2B2116uOXdOViA27SHcr31WKPgmjsxXLBs_aTxAeThzg%40mail.gmail.com Author: Pavel Borisov Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/0b5c161248110b164a006004c78f9529a109 Modified Files -- contrib/amcheck/verify_nbtree.c | 14 +- 1 file changed, 9 insertions(+), 5 deletions(-)
pgsql: amcheck: Refactoring the storage of the last visible entry
amcheck: Refactoring the storage of the last visible entry This commit introduces a new data structure BtreeLastVisibleEntry comprising information about the last visible heap entry with the current value of key. Usage of this data structure allows us to avoid passing all this information as individual function arguments. Reported-by: Alexander Korotkov Discussion: https://www.postgresql.org/message-id/CAPpHfdsVbB9ToriaB1UHuOKwjKxiZmTFQcEF%3DjuzzC_nby31uA%40mail.gmail.com Author: Pavel Borisov, Alexander Korotkov Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/532d94fec32ac11263b53932365560491d1fd50a Modified Files -- contrib/amcheck/verify_nbtree.c | 117 --- src/tools/pgindent/typedefs.list | 1 + 2 files changed, 60 insertions(+), 58 deletions(-)
pgsql: amcheck: Report an error when the next page to a leaf is not a l
amcheck: Report an error when the next page to a leaf is not a leaf This is a very unlikely condition during checking a B-tree unique constraint, meaning that the index structure is violated badly, and we shouldn't continue checking to avoid endless loops, etc. So it's worth immediately throwing an error. Reported-by: Peter Geoghegan Discussion: https://postgr.es/m/CAH2-Wzk%2B2116uOXdOViA27SHcr31WKPgmjsxXLBs_aTxAeThzg%40mail.gmail.com Author: Pavel Borisov Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/97e5b0026fc276ab1bcde58ae98ae1fcd9c3acc3 Modified Files -- contrib/amcheck/verify_nbtree.c | 22 -- 1 file changed, 16 insertions(+), 6 deletions(-)
pgsql: Fix regression tests conflict in 3ca43dbbb6
Fix regression tests conflict in 3ca43dbbb6 3ca43dbbb6 adds regression tests with permission checks. The conflict has been observed at buildfarm member piculet. This commit fixes the conflict in the following way. 1. partition_split.sql now uses role names regress_partition_split_alice and regress_partition_split_bob (it mistakenly used regress_partition_merge_alice and regress_partition_merge_bob before). 2. Permissions on schemas partitions_merge_schema and partition_split_schema are granted to corresponding roles. Before, the lack of permissions led to the creation of objects in the public schema and potential conflict. Reported-by: Daniel Gustafsson Discussion: https://postgr.es/m/03A07EF6-98D2-419B-A3AA-A111C64CC207%40yesql.se Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/2a679ae94e46ea2fa1ff5461c9d0767faecc6c8c Modified Files -- src/test/regress/expected/partition_merge.out | 4 src/test/regress/expected/partition_split.out | 25 +++-- src/test/regress/sql/partition_merge.sql | 4 src/test/regress/sql/partition_split.sql | 25 +++-- 4 files changed, 38 insertions(+), 20 deletions(-)
pgsql: Add permission check for MERGE/SPLIT partition operations
Add permission check for MERGE/SPLIT partition operations Currently, we check only owner permission for the parent table before MERGE/SPLIT partition operations. This leads to a security hole when users can get access to the data of partitions without permission. This commit fixes this problem by requiring owner permission on all the partitions involved. Reported-by: Alexander Lakhin Discussion: https://postgr.es/m/0520c72e-8d97-245e-53f9-173beca2ab2e%40gmail.com Author: Dmitry Koval, Alexander Korotkov Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/3ca43dbbb67fbfb96dec8de2e268b96790555148 Modified Files -- src/backend/parser/parse_utilcmd.c| 5 src/test/regress/expected/partition_merge.out | 29 +++ src/test/regress/expected/partition_split.out | 29 +++ src/test/regress/sql/partition_merge.sql | 33 +++ src/test/regress/sql/partition_split.sql | 33 +++ 5 files changed, 129 insertions(+)
pgsql: Revert: Remove useless self-joins
Revert: Remove useless self-joins This commit reverts d3d55ce5713 and subsequent fixes 2b26a694554, 93c85db3b5b, b44a1708abe, b7f315c9d7d, 8a8ed916f73, b5fb6736ed3, 0a93f803f45, e0477837ce4, a7928a57b9f, 5ef34a8fc38, 30b4955a466, 8c441c08279, 028b15405b4, fe093994db4, 489072ab7a9, and 466979ef031. We are quite late in the release cycle and new bugs continue to appear. Even though we have fixes for all known bugs, there is a risk of throwing many bugs to end users. The plan for self-join elimination would be to do more review and testing, then re-commit in the early v18 cycle. Reported-by: Tom Lane Discussion: https://postgr.es/m/2422119.1714691974%40sss.pgh.pa.us Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/d1d286d83c0eed695910cb20d970ea9bea2e5001 Modified Files -- doc/src/sgml/config.sgml | 16 - src/backend/optimizer/path/indxpath.c | 39 - src/backend/optimizer/plan/analyzejoins.c | 1284 ++--- src/backend/optimizer/plan/planmain.c |5 - src/backend/utils/misc/guc_tables.c | 10 - src/include/nodes/pathnodes.h | 35 +- src/include/optimizer/paths.h |3 - src/include/optimizer/planmain.h |6 - src/test/regress/expected/equivclass.out | 30 - src/test/regress/expected/join.out| 1032 --- src/test/regress/expected/sysviews.out|3 +- src/test/regress/sql/equivclass.sql | 16 - src/test/regress/sql/join.sql | 470 --- src/tools/pgindent/typedefs.list |3 - 14 files changed, 69 insertions(+), 2883 deletions(-)
pgsql: Stabilize regression tests introduced by 259c96fa8f
Stabilize regression tests introduced by 259c96fa8f Add the ORDER BY clause to new queries to avoid ordering ambiguity. Per buildfarm member rorqual. Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/449cdcd486bfc6864e4fa6784cc1526a94fe69db Modified Files -- src/test/regress/expected/partition_merge.out | 5 +++-- src/test/regress/expected/partition_split.out | 3 ++- src/test/regress/sql/partition_merge.sql | 3 ++- src/test/regress/sql/partition_split.sql | 3 ++- 4 files changed, 9 insertions(+), 5 deletions(-)
pgsql: Inherit parent's AM for partition MERGE/SPLIT operations
Inherit parent's AM for partition MERGE/SPLIT operations This commit makes new partitions created by ALTER TABLE ... SPLIT PARTITION and ALTER TABLE ... MERGE PARTITIONS commands inherit the paret table access method. Reported-by: Alexander Lakhin Discussion: https://postgr.es/m/84ada05b-be5c-473e-6d1c-ebe5dd21b190%40gmail.com Reviewed-by: Pavel Borisov Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/259c96fa8f78c0446eba1880850ea6fbe5092120 Modified Files -- doc/src/sgml/ref/alter_table.sgml | 2 ++ src/backend/commands/tablecmds.c | 6 ++ src/test/regress/expected/partition_merge.out | 18 ++ src/test/regress/expected/partition_split.out | 23 +-- src/test/regress/sql/partition_merge.sql | 13 + src/test/regress/sql/partition_split.sql | 14 ++ 6 files changed, 74 insertions(+), 2 deletions(-)
pgsql: Change the way ATExecMergePartitions() handles the name collisio
Change the way ATExecMergePartitions() handles the name collision The name collision happens when the name of the new partition is the same as the name of one of the merging partitions. Currently, ATExecMergePartitions() first gives the new partition a temporary name and then renames it when old partitions are deleted. That negatively influences the naming of related objects like indexes and constrains, which could inherit a temporary name. This commit changes the implementation in the following way. A merging partition gets renamed first, then the new partition is created with the right name immediately. This resolves the issue of the naming of related objects. Reported-by: Alexander Lakhin Discussion: https://postgr.es/m/edfbd846-dcc1-42d1-ac26-715691b687d3%40postgrespro.ru Author: Dmitry Koval, Alexander Korotkov Reviewed-by: Robert Haas, Justin Pryzby, Pavel Borisov Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/885742b9f88b9386368ee94df8c94d154677ffba Modified Files -- src/backend/commands/tablecmds.c | 63 +-- src/test/regress/expected/partition_merge.out | 25 +++ src/test/regress/sql/partition_merge.sql | 18 3 files changed, 73 insertions(+), 33 deletions(-)
pgsql: Add tab completion for partition MERGE/SPLIT operations
Add tab completion for partition MERGE/SPLIT operations This commit implements psql tab completion for ALTER TABLE ... SPLIT PARTITION and ALTER TABLE ... MERGE PARTITIONS commands. Reported-by: Alexander Lakhin Discussion: https://postgr.es/m/5dee3937-8e9f-cca4-11fb-737709a92b37%40gmail.com Author: Dagfinn Ilmari Mannsåker, Pavel Borisov Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/60ae37a8bc02f7a4beef442ee7b4656fba0e4e71 Modified Files -- src/bin/psql/tab-complete.c | 18 -- 1 file changed, 16 insertions(+), 2 deletions(-)
pgsql: Document the way partition MERGE/SPLIT operations create new par
Document the way partition MERGE/SPLIT operations create new partitions Reported-by: Justin Pryzby Discussion: https://postgr.es/m/ZilrByTp-pbz6Mvf%40pryzbyj2023 Reviewed-by: Justin Pryzby Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/842c9b27057e8ecea02b816e3ec6c208779b3d39 Modified Files -- doc/src/sgml/ref/alter_table.sgml | 12 1 file changed, 12 insertions(+)
pgsql: Rename tables in tests of partition MERGE/SPLIT operations
Rename tables in tests of partition MERGE/SPLIT operations Replace "salesman" with "salesperson", "salesmen" with "salespeople". The names are both gramatically correct and gender-neutral. Reported-by: Alexander Lakhin Discussion: https://postgr.es/m/fdaa003e-919c-cbc9-4f0c-e4546e96bd65%40gmail.com Reviewed-by: Robert Haas, Pavel Borisov Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/f4fc7cb54b6a9041f7064de71cb3ee3c4f48e061 Modified Files -- src/test/regress/expected/partition_merge.out | 666 ++--- src/test/regress/expected/partition_split.out | 1266 - src/test/regress/sql/partition_merge.sql | 160 ++-- src/test/regress/sql/partition_split.sql | 280 +++--- 4 files changed, 1186 insertions(+), 1186 deletions(-)
pgsql: Fix error message in check_partition_bounds_for_split_range()
Fix error message in check_partition_bounds_for_split_range() Currently, the error message is produced by a system of complex substitutions making it quite untranslatable and hard to read. This commit splits this into 4 plain error messages suitable for translation. Reported-by: Kyotaro Horiguchi Discussion: https://postgr.es/m/20240408.152402.1485994009160660141.horikyota.ntt%40gmail.com Reviewed-by: Pavel Borisov Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/96c7381c4ceee2ef7c0a7327beb71dec47cf855e Modified Files -- src/backend/partitioning/partbounds.c | 70 --- 1 file changed, 49 insertions(+), 21 deletions(-)
pgsql: Make new partitions with parent's persistence during MERGE/SPLIT
Make new partitions with parent's persistence during MERGE/SPLIT The createPartitionTable() function is responsible for creating new partitions for ALTER TABLE ... MERGE PARTITIONS, and ALTER TABLE ... SPLIT PARTITION commands. It emulates the behaviour of CREATE TABLE ... (LIKE ...), where new table persistence should be specified by the user. In the table partitioning persistent of the partition and its parent must match. So, this commit makes createPartitionTable() copy the persistence of the parent partition. Also, this commit makes createPartitionTable() recheck the persistence after the new table creation. This is needed because persistence might be affected by pg_temp in search_path. This commit also changes the signature of createPartitionTable() making it take the parent's Relation itself instead of the name of the parent relation, and return the Relation of new partition. That doesn't lead to complications, because both callers have the parent table open and need to open the new partition. Reported-by: Alexander Lakhin Discussion: https://postgr.es/m/dbc8b96c-3cf0-d1ee-860d-0e491da20485%40gmail.com Author: Dmitry Koval Reviewed-by: Alexander Korotkov, Robert Haas, Justin Pryzby, Pavel Borisov Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/fcf80c5d5f0f3787e70fca8fd029d2e08a923f91 Modified Files -- doc/src/sgml/ref/alter_table.sgml | 6 ++ src/backend/commands/tablecmds.c | 75 +- src/test/regress/expected/partition_merge.out | 134 ++ src/test/regress/expected/partition_split.out | 75 +- src/test/regress/sql/partition_merge.sql | 88 +++-- src/test/regress/sql/partition_split.sql | 43 - 6 files changed, 364 insertions(+), 57 deletions(-)
Re: pgsql: Fix failure to track role dependencies of pg_init_privs entries.
On Tue, Apr 30, 2024 at 2:26 AM Tom Lane wrote: > Fix failure to track role dependencies of pg_init_privs entries. I just noticed that test_pg_dump checks don't pass for me. And for most of the buildfarm members too. You must be already aware of this, just in case. -- Regards, Alexander Korotkov
pgsql: Refactoring for CommitTransactionCommand()/AbortCurrentTransacti
Refactoring for CommitTransactionCommand()/AbortCurrentTransaction() fefd9a3fed turned tail recursion of CommitTransactionCommand() and AbortCurrentTransaction() into iteration. However, it splits the handling of cases between different functions. This commit puts the handling of all the cases into AbortCurrentTransactionInternal() and CommitTransactionCommandInternal(). Now CommitTransactionCommand() and AbortCurrentTransaction() are just doing the repeated calls of internal functions. Reported-by: Andres Freund Discussion: https://postgr.es/m/20240415224834.w6piwtefskoh32mv%40awork3.anarazel.de Author: Andres Freund Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/40126ac68f2ff96351cd6071350eb2d5cbd50145 Modified Files -- src/backend/access/transam/xact.c | 155 +- 1 file changed, 71 insertions(+), 84 deletions(-)
pgsql: revert: Generalize relation analyze in table AM interface
revert: Generalize relation analyze in table AM interface This commit reverts 27bc1772fc and dd1f6b0c17. Per review by Andres Freund. Discussion: https://postgr.es/m/20240415201057.khoyxbwwxfgzomeo%40awork3.anarazel.de Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/6377e12a5a5278446bb0d215439b4825ef8996d1 Modified Files -- src/backend/access/heap/heapam_handler.c | 29 ++--- src/backend/access/table/tableamapi.c| 2 + src/backend/commands/analyze.c | 55 + src/include/access/heapam.h | 8 --- src/include/access/tableam.h | 101 --- src/include/commands/vacuum.h| 69 - src/include/foreign/fdwapi.h | 8 ++- src/tools/pgindent/typedefs.list | 3 - 8 files changed, 107 insertions(+), 168 deletions(-)
pgsql: Grammar fixes for split/merge partitions code
Grammar fixes for split/merge partitions code The fixes relate to comments, error messages, and corresponding expected output of regression tests. Discussion: https://postgr.es/m/CAMbWs49DDsknxyoycBqiE72VxzL_sYHF6zqL8dSeNehKPJhkKg%40mail.gmail.com Discussion: https://postgr.es/m/86bfd241-a58c-479a-9a72-2c67a02becf8%40postgrespro.ru Discussion: https://postgr.es/m/CAHewXNkGMPU50QG7V6Q60JGFORfo8LfYO1_GCkCa0VWbmB-fEw%40mail.gmail.com Author: Richard Guo, Dmitry Koval, Tender Wang Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/9dfcac8e15acc3b4d4d5bc02383a56ccb07229fe Modified Files -- doc/src/sgml/ddl.sgml | 2 +- doc/src/sgml/ref/alter_table.sgml | 6 +-- src/backend/commands/tablecmds.c | 21 +- src/backend/parser/parse_utilcmd.c| 6 +-- src/backend/partitioning/partbounds.c | 35 src/test/isolation/specs/partition-merge.spec | 2 +- src/test/regress/expected/partition_merge.out | 14 +++ src/test/regress/expected/partition_split.out | 58 +-- src/test/regress/sql/partition_merge.sql | 12 +++--- src/test/regress/sql/partition_split.sql | 48 +++--- 10 files changed, 104 insertions(+), 100 deletions(-)
