Re: pg_ctl start may return 0 even if the postmaster has been already started on Windows
At Wed, 20 Sep 2023 14:18:41 +0900 (JST), Kyotaro Horiguchi
wrote in
> I was able to see the trouble in the CI environment, but not
> locally. I'll delve deeper into this. Thanks you for bringing it to my
> attention.
I found two instances with multiple child processes.
# child-pid / parent-pid / given-pid : exec name
process parent PID child PID target PID exec file
shell 1228 64721228 cmd.exe
child 5184 12281228 cmd.exe
child 6956 12281228 postgres.exe
> launcher shell executed multiple processes
process parent PID child PID target PID exec file
shell 4296 58804296 cmd.exe
child 5156 42964296 agent.exe
child 5640 42964296 postgres.exe
> launcher shell executed multiple processes
It looks like the environment has autorun setups for cmd.exe. There's
another known issue related to auto-launching chcp at
startup. Ideally, we would avoid such behavior in the
postmaster-launcher shell. I think we should add "/D" flag to cmd.exe
command line, perhaps in a separate patch.
Even after making that change, I still see something being launched from the
launcher cmd.exe...
process parent PID child PID target PID exec file
shell 2784 66682784 cmd.exe
child 6140 27842784 MicrosoftEdgeUpdate.exe
child 6260 27842784 postgres.exe
> launcher shell executed multiple processes
I'm not sure what triggers this; perhaps some other kind of hooks? If
we cannot avoid this behavior, we'll have to verify the executable
file name. It should be fine, given that the file name is constant,
but I'm not fully convinced that this is the ideal solution.
Another issue is.. that I haven't been able to cause the false
positive of pg_ctl start.. Do you have a concise reproducer of the
issue?
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
>From c6de7b541be55b01bbd0d2e8e8d95aac6799a82c Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi
Date: Thu, 21 Sep 2023 16:54:33 +0900
Subject: [PATCH 1/3] Disable autoruns for cmd.exe on Windows
On Windows, we use cmd.exe to launch the postmaster process for easy
redirection setup. However, cmd.exe may execute other programs at
startup due to autorun configurations. This patch adds /D flag to the
launcher cmd.exe command line to disable autorun settings written in
the registry.
---
src/bin/pg_ctl/pg_ctl.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
index 807d7023a9..3ac2fcc004 100644
--- a/src/bin/pg_ctl/pg_ctl.c
+++ b/src/bin/pg_ctl/pg_ctl.c
@@ -553,11 +553,11 @@ start_postmaster(void)
else
close(fd);
- cmd = psprintf("\"%s\" /C \"\"%s\" %s%s < \"%s\" >> \"%s\" 2>&1\"",
+ cmd = psprintf("\"%s\" /D /C \"\"%s\" %s%s < \"%s\" >> \"%s\" 2>&1\"",
comspec, exec_path, pgdata_opt, post_opts, DEVNULL, log_file);
}
else
- cmd = psprintf("\"%s\" /C \"\"%s\" %s%s < \"%s\" 2>&1\"",
+ cmd = psprintf("\"%s\" /D /C \"\"%s\" %s%s < \"%s\" 2>&1\"",
comspec, exec_path, pgdata_opt, post_opts, DEVNULL);
if (!CreateRestrictedProcess(cmd, &pi, false))
--
2.39.3
>From f1484974f5bcf14d5ac3c6a702dcb02d0eb45f9f Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi
Date: Wed, 20 Sep 2023 13:16:35 +0900
Subject: [PATCH 2/3] Improve pg_ctl postmaster process check on Windows
Currently pg_ctl on Windows does not verify that it actually executed
a postmaster process due to lack of process ID knowledge. This can
lead to false positives in cases where another pg_ctl instance starts
a different server simultaneously.
This patch adds the capability to identify the process ID of the
launched postmaster on Windows, similar to other OS versions, ensuring
more reliable detection of concurrent server startups.
---
src/bin/pg_ctl/pg_ctl.c | 109
1 file changed, 100 insertions(+), 9 deletions(-)
diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
index 3ac2fcc004..ed1b0c43fc 100644
--- a/src/bin/pg_ctl/pg_ctl.c
+++ b/src/bin/pg_ctl/pg_ctl.c
@@ -132,6 +132,7 @@ static void adjust_data_dir(void);
#ifdef WIN32
#include
+#include
static bool pgwin32_IsInstalled(SC_HANDLE);
static char *pgwin32_CommandLine(bool);
static void pgwin32_doRegister(void);
@@ -142,6 +143,7 @@ static void WINAPI pgwin32_ServiceMain(DWORD, LPTSTR *);
static void pgwin32_doRunAsService(void);
static int CreateRestrictedProcess(char *cmd, PROCESS_INFORMATION *processInfo, bool as_service);
static PTOKEN_PRIVILEGES GetPrivilegesToDelete(HANDLE hToken);
+static pid_t pgwin32_find_postmaster_pid(pid_t shell_pid);
#endif
static pid_t get_pgpid(bool is_status_request);
@@ -609,7 +611,11 @@ wait_for_postmaster_start(pid_t pm_pid, bool do_checkpoint)
/* File is complete enough for us, parse it */
pid_t pmpid;
time_t pmstart;
Re: Infinite Interval
On Fri, 22 Sept 2023 at 08:49, Ashutosh Bapat wrote: > > Following code in ExecInterpExpr makes it clear that the > deserialization function is be executed in per tuple memory context. > Whereas the aggregate's context is different from this context and may > lives longer that the context in which deserialization is expected to > happen. > Right. I was about to reply, saying much the same thing, but it's always better when you see it for yourself. > Hence I have changed interval_avg_deserialize() in 0007 to use > CurrentMemoryContext instead of aggcontext. +1. And consistency with other deserialisation functions is good. > Rest of the patches are > same as previous set. > OK, I'll take a look. Regards, Dean
Re: [PATCH] Add extra statistics to explain for Nested Loop
On 31/7/2022 10:49, Julien Rouhaud wrote: On Sat, Jul 30, 2022 at 08:54:33PM +0800, Julien Rouhaud wrote: Anyway, 1% is in my opinion still too much overhead for extensions that won't get any extra information. I have read all the thread and still can't understand something. What valuable data can I find with these extra statistics if no one parameterized node in the plan exists? Also, thinking about min/max time in the explain, I guess it would be necessary in rare cases. Usually, the execution time will correlate to the number of tuples scanned, won't it? So, maybe skip the time boundaries in the instrument structure? In my experience, it is enough to know the total number of tuples bubbled up from a parameterized node to decide further optimizations. Maybe simplify this feature up to the one total_rows field in the case of nloops > 1 and in the presence of parameters? And at the end. If someone wants a lot of additional statistics, why not give them that by extension? It is only needed to add a hook into the point of the node explanation and some efforts to make instrumentation extensible. But here, honestly, I don't have code/ideas so far. -- regards, Andrey Lepikhov Postgres Professional
Re: Row pattern recognition
Op 9/22/23 om 07:16 schreef Tatsuo Ishii: Attached is the fix against v6 patch. I will include this in upcoming v7 patch. Attached is the v7 patch. It includes the fix mentioned above. Also (Champion's address bounced; removed) Hi, In my hands, make check fails on the rpr test; see attached .diff file. In these two statements: -- using NEXT -- using AFTER MATCH SKIP TO NEXT ROW result of first_value(price) and next_value(price) are empty. Erik Rijkers this time the pattern matching engine is enhanced: previously it recursed to row direction, which means if the number of rows in a frame is large, it could exceed the limit of stack depth. The new version recurses over matched pattern variables in a row, which is at most 26 which should be small enough. Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: Row pattern recognition
Op 9/22/23 om 10:23 schreef Erik Rijkers: Op 9/22/23 om 07:16 schreef Tatsuo Ishii: Attached is the fix against v6 patch. I will include this in upcoming v7 patch. Attached is the v7 patch. It includes the fix mentioned above. Also (Champion's address bounced; removed) Sorry, I forgot to re-attach the regression.diffs with resend... Erik Hi, In my hands, make check fails on the rpr test; see attached .diff file. In these two statements: -- using NEXT -- using AFTER MATCH SKIP TO NEXT ROW result of first_value(price) and next_value(price) are empty. Erik Rijkers this time the pattern matching engine is enhanced: previously it recursed to row direction, which means if the number of rows in a frame is large, it could exceed the limit of stack depth. The new version recurses over matched pattern variables in a row, which is at most 26 which should be small enough. Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp diff -U3 /home/aardvark/pg_stuff/pg_sandbox/pgsql.rpr/src/test/regress/expected/rpr.out /home/aardvark/pg_stuff/pg_sandbox/pgsql.rpr/src/test/regress/results/rpr.out --- /home/aardvark/pg_stuff/pg_sandbox/pgsql.rpr/src/test/regress/expected/rpr.out 2023-09-22 09:04:17.770392635 +0200 +++ /home/aardvark/pg_stuff/pg_sandbox/pgsql.rpr/src/test/regress/results/rpr.out 2023-09-22 09:38:23.826458109 +0200 @@ -253,23 +253,23 @@ ); company | tdate| price | first_value | last_value --++---+-+ - company1 | 07-01-2023 | 100 | 100 |200 + company1 | 07-01-2023 | 100 | | company1 | 07-02-2023 | 200 | | company1 | 07-03-2023 | 150 | | - company1 | 07-04-2023 | 140 | 140 |150 + company1 | 07-04-2023 | 140 | | company1 | 07-05-2023 | 150 | | company1 | 07-06-2023 |90 | | - company1 | 07-07-2023 | 110 | 110 |130 + company1 | 07-07-2023 | 110 | | company1 | 07-08-2023 | 130 | | company1 | 07-09-2023 | 120 | | company1 | 07-10-2023 | 130 | | - company2 | 07-01-2023 |50 | 50 | 2000 + company2 | 07-01-2023 |50 | | company2 | 07-02-2023 | 2000 | | company2 | 07-03-2023 | 1500 | | - company2 | 07-04-2023 | 1400 |1400 | 1500 + company2 | 07-04-2023 | 1400 | | company2 | 07-05-2023 | 1500 | | company2 | 07-06-2023 |60 | | - company2 | 07-07-2023 | 1100 |1100 | 1300 + company2 | 07-07-2023 | 1100 | | company2 | 07-08-2023 | 1300 | | company2 | 07-09-2023 | 1200 | | company2 | 07-10-2023 | 1300 | | @@ -290,23 +290,23 @@ ); company | tdate| price | first_value | last_value --++---+-+ - company1 | 07-01-2023 | 100 | 100 |200 + company1 | 07-01-2023 | 100 | | company1 | 07-02-2023 | 200 | | company1 | 07-03-2023 | 150 | | - company1 | 07-04-2023 | 140 | 140 |150 + company1 | 07-04-2023 | 140 | | company1 | 07-05-2023 | 150 | | company1 | 07-06-2023 |90 | | - company1 | 07-07-2023 | 110 | 110 |130 + company1 | 07-07-2023 | 110 | | company1 | 07-08-2023 | 130 | | company1 | 07-09-2023 | 120 | | company1 | 07-10-2023 | 130 | | - company2 | 07-01-2023 |50 | 50 | 2000 + company2 | 07-01-2023 |50 | | company2 | 07-02-2023 | 2000 | | company2 | 07-03-2023 | 1500 | | - company2 | 07-04-2023 | 1400 |1400 | 1500 + company2 | 07-04-2023 | 1400 | | company2 | 07-05-2023 | 1500 | | company2 | 07-06-2023 |60 | | - company2 | 07-07-2023 | 1100 |1100 | 1300 + company2 | 07-07-2023 | 1100 | | company2 | 07-08-2023 | 1300 | | company2 | 07-09-2023 | 1200 | | company2 | 07-10-2023 | 1300 | |
Re: Questions about the new subscription parameter: password_required
On 9/21/23 20:29, Robert Haas wrote: Which one? I see 2 ALTER SUBSCRIPTION ... OWNER commands in password_required.log and 1 more in password_required2.log, but they're all performed by the superuser, who is entitled to do anything they want. Thank you for taking the time to respond! I expected the ALTER SUBSCRIPTION ... OWNER command in password_required.log to fail because the end result of the command is a non-superuser owning a subscription with password_required=true, but the connection string has no password keyword, and the authentication scheme used doesn't require one anyway. The description of the password_required parameter doesn't clearly state what will fail or when the configuration is enforced (during CREATE SUBSCRIPTION and ALTER SUBSCRIPTION .. CONNECTION): """ https://www.postgresql.org/docs/16/sql-createsubscription.html Specifies whether connections to the publisher made as a result of this subscription must use password authentication. This setting is ignored when the subscription is owned by a superuser. The default is true. Only superusers can set this value to false. """ The description of pg_subscription.subpasswordrequired doesn't either: """ https://www.postgresql.org/docs/16/catalog-pg-subscription.html If true, the subscription will be required to specify a password for authentication """ Can we consider adding something like this to clarify? """ This parameter is enforced when the CREATE SUBSCRIPTION or ALTER SUBSCRIPTION .. CONNECTION commands are executed. Therefore, it's possible to alter the ownership of a subscription with password_required=true to a non-superuser. """ Is the DROP SUBSCRIPTION failure in password_required.log expected for both superuser and non-superuser? Is the DROP SUBSCRIPTION success in password_required2.log expected? (i.e., with password_require=false, the only action a non-superuser can perform is dropping the subscription. Since they own it, it is understandable). -- Benoit Lobréau Consultant http://dalibo.com
Re: [PoC] pg_upgrade: allow to upgrade publisher node
On Fri, Sep 22, 2023 at 10:57 AM Bharath Rupireddy wrote: > > On Thu, Sep 21, 2023 at 5:45 PM Amit Kapila wrote: > > > > > Thanks for the patch. I have some comments on v42: > > > > Probably trying to keep it similar with > > binary_upgrade_create_empty_extension(). I think it depends what > > behaviour we expect for NULL input. > > confirmed_flush_lsn for a logical slot can be null (for instance, > before confirmed_flush is updated for a newly created logical slot if > someone calls pg_stat_replication -> pg_get_replication_slots) and > when it is so, the binary_upgrade_create_empty_extension errors out. > Is this behaviour wanted? I think the function returning null on null > input is a better behaviour here. > I think if we do return null on null behavior then the caller needs to add a special case for null value as this function returns bool. We can probably return false in that case. Does that help to address your concern? -- With Regards, Amit Kapila.
Re: Infinite Interval
On Fri, Sep 22, 2023 at 3:49 PM Ashutosh Bapat wrote: > > On Thu, Sep 21, 2023 at 7:21 PM Ashutosh Bapat > wrote: > > > Hence I have changed interval_avg_deserialize() in 0007 to use > CurrentMemoryContext instead of aggcontext. Rest of the patches are > same as previous set. > > -- > Best Wishes, > Ashutosh Bapat /* TODO: Handle NULL inputs? */ since interval_avg_serialize is strict, so handle null would be like: if (PG_ARGISNULL(0)) then PG_RETURN_NULL();
Re: bug fix and documentation improvement about vacuumdb
> > No worries at all. If you look at the page now you will see all green > checkmarks indicating that the patch was tested in CI. So now we know that > your tests fail without the fix and work with the fix applied, so all is > well. > Thank you for your kind words! And it seems to me that all concerns are resolved. I'll change the patch status to Ready for Committer. If you have or find any flaw, let me know that. Best Regards, Masaki Kuwamura
Re: Fix error handling in be_tls_open_server()
> On 20 Sep 2023, at 10:58, Daniel Gustafsson wrote: >> On 20 Sep 2023, at 10:55, Sergey Shinderuk >> wrote: >> There is a typo: upon en error. Otherwise, looks good to me. Thank you. > > Thanks, will fix before pushing. I've applied this down to v15 now and the buildfarm have green builds on all three branches, thanks for the submission! -- Daniel Gustafsson
Re: Row pattern recognition
Op 9/22/23 om 07:16 schreef Tatsuo Ishii: Attached is the fix against v6 patch. I will include this in upcoming v7 patch. Attached is the v7 patch. It includes the fix mentioned above. Also Hi, In my hands, make check fails on the rpr test; see attached .diff file. In these two statements: -- using NEXT -- using AFTER MATCH SKIP TO NEXT ROW result of first_value(price) and next_value(price) are empty. Erik Rijkers this time the pattern matching engine is enhanced: previously it recursed to row direction, which means if the number of rows in a frame is large, it could exceed the limit of stack depth. The new version recurses over matched pattern variables in a row, which is at most 26 which should be small enough. Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jpdiff -U3 /home/aardvark/pg_stuff/pg_sandbox/pgsql.rpr/src/test/regress/expected/rpr.out /home/aardvark/pg_stuff/pg_sandbox/pgsql.rpr/src/test/regress/results/rpr.out --- /home/aardvark/pg_stuff/pg_sandbox/pgsql.rpr/src/test/regress/expected/rpr.out 2023-09-22 09:04:17.770392635 +0200 +++ /home/aardvark/pg_stuff/pg_sandbox/pgsql.rpr/src/test/regress/results/rpr.out 2023-09-22 09:38:23.826458109 +0200 @@ -253,23 +253,23 @@ ); company | tdate| price | first_value | last_value --++---+-+ - company1 | 07-01-2023 | 100 | 100 |200 + company1 | 07-01-2023 | 100 | | company1 | 07-02-2023 | 200 | | company1 | 07-03-2023 | 150 | | - company1 | 07-04-2023 | 140 | 140 |150 + company1 | 07-04-2023 | 140 | | company1 | 07-05-2023 | 150 | | company1 | 07-06-2023 |90 | | - company1 | 07-07-2023 | 110 | 110 |130 + company1 | 07-07-2023 | 110 | | company1 | 07-08-2023 | 130 | | company1 | 07-09-2023 | 120 | | company1 | 07-10-2023 | 130 | | - company2 | 07-01-2023 |50 | 50 | 2000 + company2 | 07-01-2023 |50 | | company2 | 07-02-2023 | 2000 | | company2 | 07-03-2023 | 1500 | | - company2 | 07-04-2023 | 1400 |1400 | 1500 + company2 | 07-04-2023 | 1400 | | company2 | 07-05-2023 | 1500 | | company2 | 07-06-2023 |60 | | - company2 | 07-07-2023 | 1100 |1100 | 1300 + company2 | 07-07-2023 | 1100 | | company2 | 07-08-2023 | 1300 | | company2 | 07-09-2023 | 1200 | | company2 | 07-10-2023 | 1300 | | @@ -290,23 +290,23 @@ ); company | tdate| price | first_value | last_value --++---+-+ - company1 | 07-01-2023 | 100 | 100 |200 + company1 | 07-01-2023 | 100 | | company1 | 07-02-2023 | 200 | | company1 | 07-03-2023 | 150 | | - company1 | 07-04-2023 | 140 | 140 |150 + company1 | 07-04-2023 | 140 | | company1 | 07-05-2023 | 150 | | company1 | 07-06-2023 |90 | | - company1 | 07-07-2023 | 110 | 110 |130 + company1 | 07-07-2023 | 110 | | company1 | 07-08-2023 | 130 | | company1 | 07-09-2023 | 120 | | company1 | 07-10-2023 | 130 | | - company2 | 07-01-2023 |50 | 50 | 2000 + company2 | 07-01-2023 |50 | | company2 | 07-02-2023 | 2000 | | company2 | 07-03-2023 | 1500 | | - company2 | 07-04-2023 | 1400 |1400 | 1500 + company2 | 07-04-2023 | 1400 | | company2 | 07-05-2023 | 1500 | | company2 | 07-06-2023 |60 | | - company2 | 07-07-2023 | 1100 |1100 | 1300 + company2 | 07-07-2023 | 1100 | | company2 | 07-08-2023 | 1300 | | company2 | 07-09-2023 | 1200 | | company2 | 07-10-2023 | 1300 | |
Re: Report planning memory in EXPLAIN ANALYZE
Hi Andy, Thanks for your feedback. On Fri, Sep 22, 2023 at 8:22 AM Andy Fan wrote: > > 1). The commit message of patch 1 just says how it does but doesn't > say why it does. After reading through the discussion, I suggest making > it clearer to others. > > ... > After the planning is done, it may still occupy lots of memory which is > allocated but not pfree-d. All of these memories can't be used in the later > stage. This patch will report such memory usage in order to making some > future mistakes can be found in an easier way. > ... That's a good summary of how the memory report can be used. Will include a line about usage in the commit message. > > Planning Memory: used=15088 bytes allocated=16384 bytes > > Word 'Planning' is kind of a time duration, so the 'used' may be the > 'used' during the duration or 'used' after the duration. Obviously you > means the later one but it is not surprising others think it in the other > way. So I'd like to reword the metrics to: We report "PLanning Time" hence used "Planning memory". Would "Planner" be good instead of "Planning"? > > Memory Occupied (Now): Parser: 1k Planner: 10k > > 'Now' is a cleaner signal that is a time point rather than a time > duration. 'Parser' and 'Planner' also show a timeline about the > important time point. At the same time, it cost very little coding > effort based on patch 01. Different people may have different > feelings about these words, I just hope I describe the goal clearly. Parsing happens before planning and that memory is not measured by this patch. May be separately but it's out of scope of this work. "used" and "allocated" are MemoryContext terms indicating memory actually used vs memory allocated. > > Personally I am pretty like patch 1, we just need to refactor some words > to make everything clearer. Thanks. -- Best Wishes, Ashutosh Bapat
Re: Row pattern recognition
> Op 9/22/23 om 07:16 schreef Tatsuo Ishii: >>> Attached is the fix against v6 patch. I will include this in upcoming >>> v7 patch. >> Attached is the v7 patch. It includes the fix mentioned above. Also > (Champion's address bounced; removed) On my side his adress bounced too:-< > Hi, > > In my hands, make check fails on the rpr test; see attached .diff > file. > In these two statements: > -- using NEXT > -- using AFTER MATCH SKIP TO NEXT ROW > result of first_value(price) and next_value(price) are empty. Strange. I have checked out fresh master branch and applied the v7 patches, then ran make check. All tests including the rpr test passed. This is Ubuntu 20.04. Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: Infinite Interval
On Fri, Sep 22, 2023 at 2:35 PM jian he wrote: > > /* TODO: Handle NULL inputs? */ > since interval_avg_serialize is strict, so handle null would be like: > if (PG_ARGISNULL(0)) then PG_RETURN_NULL(); That's automatically taken care of by the executor. Functions need to handle NULL inputs if they are *not* strict. #select proisstrict from pg_proc where proname = 'interval_avg_serialize'; proisstrict - t (1 row) #select proisstrict from pg_proc where proname = 'interval_avg_deserialize'; proisstrict - t (1 row) -- Best Wishes, Ashutosh Bapat
Re: Synchronizing slots from primary to standby
On Thu, Sep 21, 2023 at 9:16 AM shveta malik wrote: > > On Tue, Sep 19, 2023 at 10:29 AM shveta malik wrote: > > Currently in patch001, synchronize_slot_names is a GUC on both primary > and physical standby. This GUC tells which all logical slots need to > be synced on physical standbys from the primary. Ideally it should be > a GUC on physical standby alone and each physical standby should be > able to communicate the value to the primary (considering the value > may vary for different physical replicas of the same primary). The > primary on the other hand should be able to take UNION of these values > and let the logical walsenders (belonging to the slots in UNION > synchronize_slots_names) wait for physical standbys for confirmation > before sending those changes to logical subscribers. The intent is > logical subscribers should never be ahead of physical standbys. > Before getting into the details of 'synchronize_slot_names', I would like to know whether we really need the second GUC 'standby_slot_names'. Can't we simply allow all the logical wal senders corresponding to 'synchronize_slot_names' to wait for just the physical standby(s) (physical slot corresponding to such physical standby) that have sent ' synchronize_slot_names'list? We should have one physical standby slot corresponding to one physical standby. -- With Regards, Amit Kapila.
Re: Dump-restore loosing 'attnotnull' bit for DEFERRABLE PRIMARY KEY column(s).
On Wed, Sep 20, 2023 at 8:29 PM Alvaro Herrera wrote: > On 2023-Sep-20, Amul Sul wrote: > > > On the latest master head, I can see a $subject bug that seems to be > related > > commit #b0e96f311985: > > > > Here is the table definition: > > create table foo(i int, j int, CONSTRAINT pk PRIMARY KEY(i) DEFERRABLE); > > Interesting, thanks for the report. Your attribution to that commit is > correct. The table is dumped like this: > > CREATE TABLE public.foo ( > i integer CONSTRAINT pgdump_throwaway_notnull_0 NOT NULL NO INHERIT, > j integer > ); > ALTER TABLE ONLY public.foo > ADD CONSTRAINT pk PRIMARY KEY (i) DEFERRABLE; > ALTER TABLE ONLY public.foo DROP CONSTRAINT pgdump_throwaway_notnull_0; > > so the problem here is that the deferrable PK is not considered a reason > to keep attnotnull set, so we produce a throwaway constraint that we > then drop. This is already bogus, but what is more bogus is the fact > that the backend accepts the DROP CONSTRAINT at all. > > The pg_dump failing should be easy to fix, but fixing the backend to > error out sounds more critical. So, the reason for this behavior is > that RelationGetIndexList doesn't want to list an index that isn't > marked indimmediate as a primary key. I can easily hack around that by > doing > > diff --git a/src/backend/utils/cache/relcache.c > b/src/backend/utils/cache/relcache.c > index 7234cb3da6..971d9c8738 100644 > --- a/src/backend/utils/cache/relcache.c > +++ b/src/backend/utils/cache/relcache.c > @@ -4794,7 +4794,6 @@ RelationGetIndexList(Relation relation) > * check them. > */ > if (!index->indisunique || > - !index->indimmediate || > !heap_attisnull(htup, > Anum_pg_index_indpred, NULL)) > continue; > > @@ -4821,6 +4820,9 @@ RelationGetIndexList(Relation relation) > relation->rd_rel->relkind == > RELKIND_PARTITIONED_TABLE)) > pkeyIndex = index->indexrelid; > > + if (!index->indimmediate) > + continue; > + > if (!index->indisvalid) > continue; > > > But of course this is not great, since it impacts unrelated bits of code > that are relying on relation->pkindex or RelationGetIndexAttrBitmap > having their current behavior with non-immediate index. > True, but still wondering why would relation->rd_pkattr skipped for a deferrable primary key, which seems to be a bit incorrect to me since it sensing that relation doesn't have PK at all. Anyway, that is unrelated. > I think a real solution is to stop relying on RelationGetIndexAttrBitmap > in ATExecDropNotNull(). (And, again, pg_dump needs some patch as well > to avoid printing a throwaway NOT NULL constraint at this point.) > I might not have understood this, but I think, if it is ok to skip throwaway NOT NULL for deferrable PK then that would be enough for the reported issue to be fixed. I quickly tried with the attached patch which looks sufficient to skip that, but, TBH, I haven't thought carefully about this change. Regards, Amul diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index f7b61766921..5a4e915dc62 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -8543,7 +8543,7 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables) appendPQExpBufferStr(q, "LEFT JOIN pg_catalog.pg_constraint copk ON " "(copk.conrelid = src.tbloid\n" - " AND copk.contype = 'p' AND " + " AND copk.contype = 'p' AND copk.condeferrable = false AND " "copk.conkey @> array[a.attnum])\n" "WHERE a.attnum > 0::pg_catalog.int2\n" "ORDER BY a.attrelid, a.attnum");
Re: pg_upgrade --check fails to warn about abstime
On 2023-Sep-21, Tom Lane wrote:
> Bruce Momjian writes:
> > Wow, I never added code to pg_upgrade to check for that, and no one
> > complained either.
>
> Yeah, so most people had indeed listened to warnings and moved away
> from those datatypes. I'm inclined to think that adding code for this
> at this point is a bit of a waste of time.
The migrations from versions prior to 12 have not stopped yet, and I did
receive a complaint about it. Because the change is so simple, I'm
inclined to patch it anyway, late though it is.
I decided to follow Tristan's advice to add the version number as a
parameter to the new function; this way, the knowledge of where was what
dropped is all in the callsite and none in the function. It
looked a bit schizoid otherwise.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"Postgres is bloatware by design: it was built to house
PhD theses." (Joey Hellerstein, SIGMOD annual conference 2002)
>From 5258379693db55618c6451ce8df4971521eb8e66 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera
Date: Tue, 19 Sep 2023 12:30:11 +0200
Subject: [PATCH v2] pg_upgrade: check for types removed in pg12
Commit cda6a8d01d39 removed a few datatypes, but didn't update
pg_upgrade --check to throw error if these types are used. So the users
find that pg_upgrade --check tells them that everything is fine, only to
fail when the real upgrade is attempted.
---
src/bin/pg_upgrade/check.c | 51 +-
1 file changed, 50 insertions(+), 1 deletion(-)
diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index 56e313f562..21a0ff9e42 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -26,6 +26,9 @@ static void check_for_tables_with_oids(ClusterInfo *cluster);
static void check_for_composite_data_type_usage(ClusterInfo *cluster);
static void check_for_reg_data_type_usage(ClusterInfo *cluster);
static void check_for_aclitem_data_type_usage(ClusterInfo *cluster);
+static void check_for_removed_data_type_usage(ClusterInfo *cluster,
+ const char *version,
+ const char *datatype);
static void check_for_jsonb_9_4_usage(ClusterInfo *cluster);
static void check_for_pg_role_prefix(ClusterInfo *cluster);
static void check_for_new_tablespace_dir(void);
@@ -111,6 +114,16 @@ check_and_dump_old_cluster(bool live_check)
if (GET_MAJOR_VERSION(old_cluster.major_version) <= 1500)
check_for_aclitem_data_type_usage(&old_cluster);
+ /*
+ * PG 12 removed types abstime, reltime, tinterval.
+ */
+ if (GET_MAJOR_VERSION(old_cluster.major_version) <= 1100)
+ {
+ check_for_removed_data_type_usage(&old_cluster, "12", "abstime");
+ check_for_removed_data_type_usage(&old_cluster, "12", "reltime");
+ check_for_removed_data_type_usage(&old_cluster, "12", "tinterval");
+ }
+
/*
* PG 14 changed the function signature of encoding conversion functions.
* Conversions from older versions cannot be upgraded automatically
@@ -1215,7 +1228,8 @@ check_for_aclitem_data_type_usage(ClusterInfo *cluster)
{
char output_path[MAXPGPATH];
- prep_status("Checking for incompatible \"aclitem\" data type in user tables");
+ prep_status("Checking for incompatible \"%s\" data type in user tables",
+"aclitem");
snprintf(output_path, sizeof(output_path), "tables_using_aclitem.txt");
@@ -1233,6 +1247,41 @@ check_for_aclitem_data_type_usage(ClusterInfo *cluster)
check_ok();
}
+/*
+ * check_for_removed_data_type_usage
+ *
+ * Check for in-core data types that have been removed. Callers know
+ * the exact list.
+ */
+static void
+check_for_removed_data_type_usage(ClusterInfo *cluster, const char *version,
+ const char *datatype)
+{
+ char output_path[MAXPGPATH];
+ char typename[NAMEDATALEN];
+
+ prep_status("Checking for removed \"%s\" data type in user tables",
+datatype);
+
+ snprintf(output_path, sizeof(output_path), "tables_using_%s.txt",
+ datatype);
+ snprintf(typename, sizeof(typename), "pg_catalog.%s", datatype);
+
+ if (check_for_data_type_usage(cluster, typename, output_path))
+ {
+ pg_log(PG_REPORT, "fatal");
+ pg_fatal("Your installation contains the \"%s\" data type in user tables.\n"
+ "The \"%s\" type has been removed in PostgreSQL version %s,\n"
+ "so this cluster cannot currently be upgraded. You can drop the\n"
+ "problem columns, or change them to another data type, and restart\n"
+ "the upgrade. A list of the problem columns is in the file:\n"
+ "%s", datatype, datatype, version, output_path);
+ }
+ else
+ check_ok();
+}
+
+
/*
* check_for_jsonb_9_4_usage()
*
--
2.39.2
Re: logical decoding and replication of sequences, take 2
On Wed, Sep 20, 2023 at 3:23 PM Dilip Kumar wrote:
>
> On Wed, Aug 16, 2023 at 7:57 PM Tomas Vondra
> wrote:
> >
>
> I was reading through 0001, I noticed this comment in
> ReorderBufferSequenceIsTransactional() function
>
> + * To decide if a sequence change should be handled as transactional or
> applied
> + * immediately, we track (sequence) relfilenodes created by each transaction.
> + * We don't know if the current sub-transaction was already assigned to the
> + * top-level transaction, so we need to check all transactions.
>
> It says "We don't know if the current sub-transaction was already
> assigned to the top-level transaction, so we need to check all
> transactions". But IIRC as part of the steaming of in-progress
> transactions we have ensured that whenever we are logging the first
> change by any subtransaction we include the top transaction ID in it.
>
> Refer this code
>
> LogicalDecodingProcessRecord(LogicalDecodingContext *ctx,
> XLogReaderState *record)
> {
> ...
> /*
> * If the top-level xid is valid, we need to assign the subxact to the
> * top-level xact. We need to do this for all records, hence we do it
> * before the switch.
> */
> if (TransactionIdIsValid(txid))
> {
> ReorderBufferAssignChild(ctx->reorder,
> txid,
> XLogRecGetXid(record),
> buf.origptr);
> }
> }
Some more comments
1.
ReorderBufferSequenceIsTransactional and ReorderBufferSequenceGetXid
are duplicated except the first one is just confirming whether
relfilelocator was created in the transaction or not and the other is
returning the XID as well so I think these two could be easily merged
so that we can avoid duplicate codes.
2.
/*
+ * ReorderBufferTransferSequencesToParent
+ * Copy the relfilenode entries to the parent after assignment.
+ */
+static void
+ReorderBufferTransferSequencesToParent(ReorderBuffer *rb,
+ReorderBufferTXN *txn,
+ReorderBufferTXN *subtxn)
If we agree with my comment in the previous email (i.e. the first WAL
by a subxid will always include topxid) then we do not need this
function at all and always add relfilelocator directly to the top
transaction and we never need to transfer.
That is all I have for now while first pass of 0001, later I will do a
more detailed review and will look into other patches also.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
Re: Row pattern recognition
Op 9/22/23 om 12:12 schreef Tatsuo Ishii: Op 9/22/23 om 07:16 schreef Tatsuo Ishii: Attached is the fix against v6 patch. I will include this in upcoming v7 patch. Attached is the v7 patch. It includes the fix mentioned above. Also (Champion's address bounced; removed) On my side his adress bounced too:-< Hi, In my hands, make check fails on the rpr test; see attached .diff file. In these two statements: -- using NEXT -- using AFTER MATCH SKIP TO NEXT ROW result of first_value(price) and next_value(price) are empty. Strange. I have checked out fresh master branch and applied the v7 patches, then ran make check. All tests including the rpr test passed. This is Ubuntu 20.04. The curious thing is that the server otherwise builds ok, and if I explicitly run on that server 'CREATE TEMP TABLE stock' + the 20 INSERTS (just to make sure to have known data), those two statements now both return the correct result. So maybe the testing/timing is wonky (not necessarily the server). Erik Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: Refactor ssl tests to avoid using internal PostgreSQL::Test::Cluster methods
> On 3 Jul 2023, at 13:37, Heikki Linnakangas wrote: > If "pg_ctl restart" fails because of a timeout, for example, the PID file > could be present. Previously, the _update_pid(0) would error out on that, but > now it's accepted. I think that's fine, but just wanted to point it out. Revisiting an old patch. Agreed, I think that's fine, so I went ahead and pushed this. Thanks for review! It's unfortunately not that common for buildfarm animals to run the SSL tests, so I guess I'll keep an extra eye on the CFBot for this one. -- Daniel Gustafsson
RE: pg_ctl start may return 0 even if the postmaster has been already started on Windows
Dear Horiguchi-san, Thank you for making a patch! They can pass ci. I'm still not sure what should be, but I can respond a part. > Another issue is.. that I haven't been able to cause the false > positive of pg_ctl start.. Do you have a concise reproducer of the > issue? I found a short sleep in pg_ctl/t/001_start_stop.pl. This was introduced in 6bcce2580 to ensure waiting more than 2 seconds. I've tested on my CI and found that removing the sleep can trigger the failure. Also, I confirmed your patch fixes the problem. PSA the small patch for cfbot. 0001 and 0002 were not changed. Best Regards, Hayato Kuroda FUJITSU LIMITED v2-0001-Disable-autoruns-for-cmd.exe-on-Windows.patch Description: v2-0001-Disable-autoruns-for-cmd.exe-on-Windows.patch v2-0002-Improve-pg_ctl-postmaster-process-check-on-Window.patch Description: v2-0002-Improve-pg_ctl-postmaster-process-check-on-Window.patch v2-0003-Remove-short-sleep-from-001_start_stop.pl.patch Description: v2-0003-Remove-short-sleep-from-001_start_stop.pl.patch
Re: Synchronizing slots from primary to standby
Hi, Thanks for all the work that has been done on this feature, and sorry to have been quiet on it for so long. On 9/18/23 12:22 PM, shveta malik wrote: On Wed, Sep 13, 2023 at 4:48 PM Hayato Kuroda (Fujitsu) wrote: Right, but I wanted to know why it is needed. One motivation seemed to know the WAL location of physical standby, but I thought that struct WalSnd.apply could be also used. Is it bad to assume that the physical walsender always exists? We do not plan to target this case where physical slot is not created between primary and physical-standby in the first draft. In such a case, slot-synchronization will be skipped for the time being. We can extend this functionality (if needed) later. I do think it's needed to extend this functionality. Having physical slot created sounds like a (too?) strong requirement as: - It has not been a requirement for Logical decoding on standby so that could sounds weird to require it for sync slot (while it's not allowed to logical decode from sync slots) - One could want to limit the WAL space used on the primary It seems that the "skipping sync as primary_slot_name not set." warning message is emitted every 10ms, that seems too verbose to me. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Re: Questions about the new subscription parameter: password_required
On Fri, Sep 22, 2023 at 4:25 AM Benoit Lobréau wrote: > Can we consider adding something like this to clarify? > > """ > This parameter is enforced when the CREATE SUBSCRIPTION or ALTER > SUBSCRIPTION .. CONNECTION commands are executed. Therefore, it's > possible to alter the ownership of a subscription with > password_required=true to a non-superuser. > """ I'm not sure of the exact wording, but there was another recent thread complaining about this being unclear, so it seems like some clarification is needed. [ adding Jeff Davis in case he wants to weigh in here ] > Is the DROP SUBSCRIPTION failure in password_required.log expected for > both superuser and non-superuser? > > Is the DROP SUBSCRIPTION success in password_required2.log expected? > (i.e., with password_require=false, the only action a non-superuser can > perform is dropping the subscription. Since they own it, it is > understandable). I haven't checked this, but I think what's happening here is that DROP SUBSCRIPTION tries to drop the remote slot, which requires making a connection, which can trigger the error. You might get different results if you did ALTER SUBSCRIPTION ... SET (slot_name = none) first. -- Robert Haas EDB: http://www.enterprisedb.com
Re: bug fix and documentation improvement about vacuumdb
> On 22 Sep 2023, at 11:08, Kuwamura Masaki > wrote: > > No worries at all. If you look at the page now you will see all green > checkmarks indicating that the patch was tested in CI. So now we know that > your tests fail without the fix and work with the fix applied, so all is well. > > Thank you for your kind words! > > And it seems to me that all concerns are resolved. > I'll change the patch status to Ready for Committer. > If you have or find any flaw, let me know that. I had a look at this and tweaked the testcase a bit to make the diff smaller, as well as removed the (in some cases) superfluous space in the generated SQL query mentioned upthread. The attached two patches is what I propose we commit to fix this, with a backpatch to v16 where it was introduced. -- Daniel Gustafsson v3-0002-vacuumdb-Reword-help-message-for-clarity.patch Description: Binary data v3-0001-vacuumdb-Fix-excluding-multiple-schemas-with-N.patch Description: Binary data
Re: Use virtual tuple slot for Unique node
I did a little more perf testing with this. I'm seeing the same benefit with the query you posted. But can we find a case where it's not beneficial? If I understand correctly, when the input slot is a virtual slot, it's cheaper to copy it to another virtual slot than to form a minimal tuple. Like in your test case. What if the input is a minimial tuple? On master: postgres=# set enable_hashagg=off; SET postgres=# explain analyze select distinct g::text, 'a', 'b', 'c','d', 'e','f','g','h' from generate_series(1, 500) g; QUERY PLAN --- Unique (cost=2630852.42..2655852.42 rows=200 width=288) (actual time=4525.212..6576.992 rows=500 loops=1) -> Sort (cost=2630852.42..2643352.42 rows=500 width=288) (actual time=4525.211..5960.967 rows=500 loops=1) Sort Key: ((g)::text) Sort Method: external merge Disk: 165296kB -> Function Scan on generate_series g (cost=0.00..75000.00 rows=500 width=288) (actual time=518.914..1194.702 rows=500 loops=1) Planning Time: 0.036 ms JIT: Functions: 5 Options: Inlining true, Optimization true, Expressions true, Deforming true Timing: Generation 0.242 ms (Deform 0.035 ms), Inlining 63.457 ms, Optimization 29.764 ms, Emission 20.592 ms, Total 114.056 ms Execution Time: 6766.399 ms (11 rows) With the patch: postgres=# set enable_hashagg=off; SET postgres=# explain analyze select distinct g::text, 'a', 'b', 'c','d', 'e','f','g','h' from generate_series(1, 500) g; QUERY PLAN --- Unique (cost=2630852.42..2655852.42 rows=200 width=288) (actual time=4563.639..7362.467 rows=500 loops=1) -> Sort (cost=2630852.42..2643352.42 rows=500 width=288) (actual time=4563.637..6069.000 rows=500 loops=1) Sort Key: ((g)::text) Sort Method: external merge Disk: 165296kB -> Function Scan on generate_series g (cost=0.00..75000.00 rows=500 width=288) (actual time=528.060..1191.483 rows=500 loops=1) Planning Time: 0.720 ms JIT: Functions: 5 Options: Inlining true, Optimization true, Expressions true, Deforming true Timing: Generation 0.406 ms (Deform 0.065 ms), Inlining 68.385 ms, Optimization 21.656 ms, Emission 21.033 ms, Total 111.480 ms Execution Time: 7585.306 ms (11 rows) So not a win in this case. Could you peek at the outer slot type, and use the same kind of slot for the Unique's result? Or some more complicated logic, like use a virtual slot if all the values are pass-by-val? I'd also like to keep this simple, though... Would this kind of optimization make sense elsewhere? -- Heikki Linnakangas Neon (https://neon.tech)
Re: Index range search optimization
On Fri, 22 Sept 2023 at 00:48, Peter Geoghegan wrote: > > On Thu, Sep 21, 2023 at 5:11 AM Pavel Borisov wrote: > > I looked at the patch code and I agree with this optimization. > > Implementation also looks good to me except change : > > + if (key->sk_flags & (SK_BT_REQFWD | SK_BT_REQBKWD) && > > + !(key->sk_flags & SK_ROW_HEADER)) > > + requiredDir = true; > > ... > > - if ((key->sk_flags & SK_BT_REQFWD) && > > - ScanDirectionIsForward(dir)) > > - *continuescan = false; > > - else if ((key->sk_flags & SK_BT_REQBKWD) && > > - ScanDirectionIsBackward(dir)) > > + if (requiredDir) > > *continuescan = false; > > > > looks like changing behavior in the case when key->sk_flags & > > SK_BT_REQFWD && (! ScanDirectionIsForward(dir)) && > > (!requiredDirMatched) > > Originally it doesn't set *continuescan = false; and with the patch it will > > set. > > I agree that this is a problem. Inequality strategy scan keys are used > when the initial positioning strategy used by _bt_first (for its > _bt_search call) is based on an operator other than the "=" operator > for the opclass. These scan keys are required in one direction only > (Konstantin's original patch just focussed on these cases, actually). > Obviously, that difference matters. I don't think that this patch > should do anything that even looks like it might be revising the > formal definition of "required in the current scan direction". I think it's the simplification that changed code behavior - just an overlook and this could be fixed easily. > Also, requiredDirMatched isn't initialized by _bt_readpage() when > "so->firstPage". Shouldn't it be initialized to false? True. > > Also naming of requiredDirMatched and requiredDir seems semantically > > hard to understand the meaning without looking at the patch commit > > message. But I don't have better proposals yet, so maybe it's > > acceptable. > > I agree. How about "requiredMatchedByPrecheck" instead of > "requiredDirMatched", and "required" instead of "requiredDir"? For me, the main semantic meaning is omitted and even more unclear, i.e. what exactly required and matched. I'd suppose scanDirRequired, scanDirMatched, but feel it's not ideal either. Or maybe trySkipRange, canSkipRange etc. Regards, Pavel Borisov, Supabase.
Re: Index range search optimization
Hi Peter,
Hi Pavel,
The v4 of the patch is attached.
On Thu, Sep 21, 2023 at 11:48 PM Peter Geoghegan wrote:
>
> On Thu, Sep 21, 2023 at 5:11 AM Pavel Borisov wrote:
> > I looked at the patch code and I agree with this optimization.
> > Implementation also looks good to me except change :
> > + if (key->sk_flags & (SK_BT_REQFWD | SK_BT_REQBKWD) &&
> > + !(key->sk_flags & SK_ROW_HEADER))
> > + requiredDir = true;
> > ...
> > - if ((key->sk_flags & SK_BT_REQFWD) &&
> > - ScanDirectionIsForward(dir))
> > - *continuescan = false;
> > - else if ((key->sk_flags & SK_BT_REQBKWD) &&
> > - ScanDirectionIsBackward(dir))
> > + if (requiredDir)
> > *continuescan = false;
> >
> > looks like changing behavior in the case when key->sk_flags &
> > SK_BT_REQFWD && (! ScanDirectionIsForward(dir)) &&
> > (!requiredDirMatched)
> > Originally it doesn't set *continuescan = false; and with the patch it will
> > set.
>
> I agree that this is a problem. Inequality strategy scan keys are used
> when the initial positioning strategy used by _bt_first (for its
> _bt_search call) is based on an operator other than the "=" operator
> for the opclass. These scan keys are required in one direction only
> (Konstantin's original patch just focussed on these cases, actually).
> Obviously, that difference matters. I don't think that this patch
> should do anything that even looks like it might be revising the
> formal definition of "required in the current scan direction".
Sorry, that was messed up from various attempts to write the patch.
Actually, I end up with two boolean variables indicating whether the
current key is required for the same direction or opposite direction
scan. I believe that the key required for the opposite direction scan
should be already satisfied by _bt_first() except for NULLs case.
I've implemented a skip of calling the key function for this case
(with assert that result is the same).
> Why is SK_ROW_HEADER treated as a special case by the patch? Could it
> be related to the issues with required-ness and scan direction? Note
> that we never use BTEqualStrategyNumber for SK_ROW_HEADER scan key row
> comparisons, so they're only ever required for one scan direction.
> (Equality-type row constructor syntax can of course be used without
> preventing the system from using an index scan, but the nbtree code
> will not see that case as a row comparison in the first place. This is
> due to preprocessing by the planner -- nbtree just sees conventional
> scan keys with multiple simple equality scan keys with = row
> comparisons.)
The thing is that NULLs could appear in the middle of matching values.
# WITH t (a, b) AS (VALUES ('a', 'b'), ('a', NULL), ('b', 'a'))
SELECT a, b, (a, b) > ('a', 'a') FROM t ORDER BY (a, b);
a | b | ?column?
---+--+--
a | b| t
a | NULL | NULL
b | a| t
(3 rows)
So we can't just skip the row comparison operator, because we can meet
NULL at any place.
> > This may be relevant for the first page when requiredDirMatched is
> > intentionally skipped to be set and for call
> > _bt_checkkeys(scan, itup, truncatt, dir, &continuescan, false);
>
> Also, requiredDirMatched isn't initialized by _bt_readpage() when
> "so->firstPage". Shouldn't it be initialized to false?
>
> Also, don't we need to take more care with a fully empty page? The "if
> (!so->firstPage) ... " block should be gated using a condition such as
> "if (!so->firstPage && minoff < maxoff)". (Adding a "minoff <= maxoff"
> test would also work, but then the optimization will get applied on
> pages with only one non-pivot tuple. That would be harmless, but a
> waste of cycles.)
This makes sense. I've added (minoff < maxoff) to the condition.
> > Also naming of requiredDirMatched and requiredDir seems semantically
> > hard to understand the meaning without looking at the patch commit
> > message. But I don't have better proposals yet, so maybe it's
> > acceptable.
>
> I agree. How about "requiredMatchedByPrecheck" instead of
> "requiredDirMatched", and "required" instead of "requiredDir"?
>
> It would be nice if this patch worked in a way that could be verified
> by an assertion. Under this scheme, the optimization would only really
> be used in release builds (builds without assertions enabled, really).
> We'd only verify that the optimized case agreed with the slow path in
> assert-enabled builds. It might also make sense to always "apply the
> optimization" on assert-enabled builds, even for the first page seen
> by _bt_readpage by any _bt_first-wise scan. Maybe this sort of
> approach is impractical here for some reason, but I don't see why it
> should be.
Yes, this makes sense. I've added an assert check that results are
the same as with requiredMatchedByPrecheck == false.
> Obviously, the optimization should lower the amount of work in some
> calls to _bt_checkkeys, without ever changing the answer _bt_checkkeys
> gives. Ideally, it should be done in a way that makes that very
> obvious. There
Re: Row pattern recognition
On Fri, Sep 22, 2023, 3:13 AM Tatsuo Ishii wrote: > > Op 9/22/23 om 07:16 schreef Tatsuo Ishii: > >>> Attached is the fix against v6 patch. I will include this in upcoming > >>> v7 patch. > >> Attached is the v7 patch. It includes the fix mentioned above. Also > > (Champion's address bounced; removed) > > On my side his adress bounced too:-< > Yep. I'm still here, just lurking for now. It'll take a little time for me to get back to this thread, as my schedule has changed significantly. :D Thanks, --Jacob >
Re: Questions about the new subscription parameter: password_required
On 9/22/23 14:36, Robert Haas wrote:
I haven't checked this, but I think what's happening here is that DROP
SUBSCRIPTION tries to drop the remote slot, which requires making a
connection, which can trigger the error. You might get different
results if you did ALTER SUBSCRIPTION ... SET (slot_name = none)
first.
You're right, it comes from the connection to drop the slot.
But the code in for DropSubscription in
src/backend/commands/subscriptioncmds.c tries to connect before testing
if the slot is NONE / NULL. So it doesn't work to DISABLE the
subscription and set the slot to NONE.
1522 >~~~must_use_password = !superuser_arg(subowner) &&
form->subpasswordrequired;
...
1685 >~~~wrconn = walrcv_connect(conninfo, true, must_use_password,
1 >~~~>~~~>~~~>~~~>~~~>~~~>~~~subname, &err);
2 >~~~if (wrconn == NULL)
3 >~~~{
4 >~~~>~~~if (!slotname)
5 >~~~>~~~{
6 >~~~>~~~>~~~/* be tidy */
7 >~~~>~~~>~~~list_free(rstates);
8 >~~~>~~~>~~~table_close(rel, NoLock);
9 >~~~>~~~>~~~return;
10 >~~~>~~~}
11 >~~~>~~~else
12 >~~~>~~~{
13 >~~~>~~~>~~~ReportSlotConnectionError(rstates, subid, slotname,
err);
14 >~~~>~~~}
15 >~~~}
Reading the code, I think I understand why the postgres user cannot drop
the slot:
* the owner is sub_owner (not a superuser)
* and form->subpasswordrequired is true
Should there be a test to check if the user executing the query is
superuser? maybe it's handled differently? (I am not very familiar with
the code).
I dont understand (yet?) why I can do ALTER SUBSCRIPTIONs after changing
the ownership to an unpriviledged user (must_use_password should be true
also in that case).
--
Benoit Lobréau
Consultant
http://dalibo.com
Re: GenBKI emits useless open;close for catalogs without rows
On Tue, 19 Sept 2023 at 20:05, Heikki Linnakangas wrote: > > On 18/09/2023 17:50, Matthias van de Meent wrote: > > (initdb takes about 73ms locally with syncing disabled) > > That's impressive. It takes about 600 ms on my laptop. Of which about > 140 ms goes into processing the BKI file. And that's with "initdb > -no-sync" option. Hmm, yes, I misinterpreted my own benchmark setup, the actual value would be somewhere around 365ms: I thought I was doing 50*50 runs in one timed run, but really I was doing only 50 runs. TO add insult to injury, I divided the total time taken by 250 instead of either 50 or 2500... Thanks for correcting me on that. > > Various methods of reducing the size of postgres.bki were applied, as > > detailed in the patch's commit message. I believe the current output > > is still quite human readable. > > Overall this does not seem very worthwhile to me. Reducing the size of redistributables sounds worthwhile to me, but if none of these changes are worth the effort, then alright, nothing gained, only time lost. > Looking at "perf" profile of initdb, I also noticed that a small but > measurable amount of time is spent in the "isatty(0)" call in do_end(). > Does anyone care about doing bootstrap mode interactively? We could > probably remove that. Yeah, that sounds like a good idea. Kind regards, Matthias van de Meent
Re: GUC for temporarily disabling event triggers
> On 7 Sep 2023, at 21:02, Robert Haas wrote: > > On Thu, Sep 7, 2023 at 1:57 AM Michael Paquier wrote: >> + GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE >> >> I am a bit surprised by these two additions. Setting this GUC at >> file-level can be useful, as is documenting it in the control file if >> it provides some control of how a statement behaves, no? > > Yeah, I don't think these options should be used. Removed. >> + Allow temporarily disabling execution of event triggers in order to >> + troubleshoot and repair faulty event triggers. All event triggers >> will >> + be disabled by setting it to true. Setting the >> value >> + to false will not disable any event triggers, this >> + is the default value. Only superusers and users with the appropriate >> + SET privilege can change this setting. >> >> Event triggers are disabled if setting this GUC to false, while true, >> the default, allows event triggers. The values are reversed in this >> description. > > Woops. Fixed. Since the main driver for this is to reduce the usage/need for single-user mode I also reworded the patch slightly. Instead of phrasing this as an alternative to single-user mode, I reversed it such that single-user mode is an alternative to this GUC. -- Daniel Gustafsson v9-0001-Add-GUC-for-temporarily-disabling-event-triggers.patch Description: Binary data
Re: GenBKI emits useless open;close for catalogs without rows
On Fri, 22 Sept 2023 at 00:25, Andres Freund wrote: > > Hi, > > On 2023-09-19 21:05:41 +0300, Heikki Linnakangas wrote: > > On 18/09/2023 17:50, Matthias van de Meent wrote: > > > (initdb takes about 73ms locally with syncing disabled) > > > > That's impressive. It takes about 600 ms on my laptop. Of which about 140 ms > > goes into processing the BKI file. And that's with "initdb -no-sync" option. > > I think there must be a digit missing in Matthias' numbers. Yes, kind of. The run was on 50 iterations, not the assumed 250. Also note that the improved measurements were recorded inside the boostrap-mode PostgreSQL instance, not inside the initdb that was processing the postgres.bki file. So it might well be that I didn't improve the total timing by much. > > > Various methods of reducing the size of postgres.bki were applied, as > > > detailed in the patch's commit message. I believe the current output > > > is still quite human readable. > > > > Overall this does not seem very worthwhile to me. > > Because the wins are too small? > > FWIW, Making postgres.bki smaller and improving bootstrapping time does seem > worthwhile to me. But it doesn't seem quite right to handle the batching in > the file format, it should be on the server side, no? The main reason I did batching in the file format is to reduce the storage overhead of the current one "INSERT" per row. Batching improved that by replacing the token with a different construct, but it's not neccessarily the only solution. The actual parser still inserts the tuples one by one in the relation, as I didn't spend time on making a simple_heap_insert analog for bulk insertions. > We really should stop emitting WAL during initdb... I think it's quite elegant that we're able to bootstrap the relation data of a new PostgreSQL cluster from the WAL generated in another cluster, even if it is indeed a bit wasteful. I do see your point though - the WAL shouldn't be needed if we're already fsyncing the files to disk. Kind regards, Matthias van de Meent Neon (https://neon.tech)
Invalidate the subscription worker in cases where a user loses their superuser status
Hi,
The subscription worker was not getting invalidated when the
subscription owner changed from superuser to non-superuser. Here is a
test case for the same:
Publisher:
CREATE USER repl REPLICATION PASSWORD 'secret';
CREATE TABLE t(i INT);
INSERT INTO t VALUES(1);
GRANT SELECT ON t TO repl;
CREATE PUBLICATION p1 FOR TABLE t;
Subscriber (has a PGPASSFILE for user "repl"):
CREATE USER u1 SUPERUSER;
CREATE TABLE t(i INT);
ALTER TABLE t OWNER TO u1;
-- no password specified
CREATE SUBSCRIPTION s1
CONNECTION 'dbname=postgres host=127.0.0.1 port=5432 user=repl'
PUBLICATION p1;
ALTER USER u1 NOSUPERUSER: -- Change u1 user to non-superuser
Publisher:
INSERT INTO t VALUES(1);
Subscriber:
SELECT COUNT(*) FROM t; -- should have been 1 but is 2, the apply
worker has not exited after changing from superuser to non-superuser.
Fixed this issue by checking if the subscription owner has changed
from superuser to non-superuser in case the pg_authid rows changes.
The attached patch has the changes for the same.
Thanks to Jeff Davis for identifying this issue and reporting it at [1].
[1] -
https://www.postgresql.org/message-id/5dff4caf26f45ce224a33a5e18e110b93a351b2f.camel%40j-davis.com
Regards,
Vignesh
From 9347e863cc78d328f7556db0e357787d14feae75 Mon Sep 17 00:00:00 2001
From: Vignesh C
Date: Fri, 22 Sep 2023 15:12:23 +0530
Subject: [PATCH v1] Restart the apply worker if the subscription owner has
changed from superuser to non-superuser.
Restart the apply worker if the subscription owner has changed from
superuser to non-superuser. This is required so that the subscription
connection string gets revalidated to identify cases where the
password option is not specified as part of the connection string for
non-superuser.
---
src/backend/replication/logical/worker.c | 23 +++---
src/include/catalog/pg_subscription.h | 1 +
src/test/subscription/t/027_nosuperuser.pl | 21
3 files changed, 42 insertions(+), 3 deletions(-)
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index 597947410f..29197d86cb 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -3942,7 +3942,8 @@ maybe_reread_subscription(void)
* The launcher will start a new worker but note that the parallel apply
* worker won't restart if the streaming option's value is changed from
* 'parallel' to any other value or the server decides not to stream the
- * in-progress transaction.
+ * in-progress transaction. Exit if the owner of the subscription has
+ * changed from superuser to a non-superuser.
*/
if (strcmp(newsub->conninfo, MySubscription->conninfo) != 0 ||
strcmp(newsub->name, MySubscription->name) != 0 ||
@@ -3952,7 +3953,9 @@ maybe_reread_subscription(void)
newsub->passwordrequired != MySubscription->passwordrequired ||
strcmp(newsub->origin, MySubscription->origin) != 0 ||
newsub->owner != MySubscription->owner ||
- !equal(newsub->publications, MySubscription->publications))
+ !equal(newsub->publications, MySubscription->publications) ||
+ (!superuser_arg(MySubscription->owner) &&
+ MySubscription->isownersuperuser))
{
if (am_parallel_apply_worker())
ereport(LOG,
@@ -4605,6 +4608,13 @@ InitializeLogRepWorker(void)
proc_exit(0);
}
+ /*
+ * Fetch subscription owner is a superuser. This value will be later
+ * checked to see when there is any change with this role and the worker
+ * will be restarted if required.
+ */
+ MySubscription->isownersuperuser = superuser_arg(MySubscription->owner);
+
MySubscriptionValid = true;
MemoryContextSwitchTo(oldctx);
@@ -4621,11 +4631,18 @@ InitializeLogRepWorker(void)
SetConfigOption("synchronous_commit", MySubscription->synccommit,
PGC_BACKEND, PGC_S_OVERRIDE);
- /* Keep us informed about subscription changes. */
+ /*
+ * Keep us informed about subscription changes or pg_authid rows.
+ * (superuser can become non-superuser.)
+ */
CacheRegisterSyscacheCallback(SUBSCRIPTIONOID,
subscription_change_cb,
(Datum) 0);
+ CacheRegisterSyscacheCallback(AUTHOID,
+ subscription_change_cb,
+ (Datum) 0);
+
if (am_tablesync_worker())
ereport(LOG,
(errmsg("logical replication table synchronization worker for subscription \"%s\", table \"%s\" has started",
diff --git a/src/include/catalog/pg_subscription.h b/src/include/catalog/pg_subscription.h
index be36c4a820..87f6f644a9 100644
--- a/src/include/catalog/pg_subscription.h
+++ b/src/include/catalog/pg_subscription.h
@@ -144,6 +144,7 @@ typedef struct Subscription
List *publications; /* List of publication names to subscribe to */
char *origin; /* Only publish data originating from the
* specified origin */
+ bool isownersuperuser; /* Is subscription owner superuser? */
} Subscription;
/* Disallow streaming in-progress transactions. */
diff --git
Re: Questions about the new subscription parameter: password_required
On Fri, Sep 22, 2023 at 10:59 AM Benoit Lobréau
wrote:
> You're right, it comes from the connection to drop the slot.
>
> But the code in for DropSubscription in
> src/backend/commands/subscriptioncmds.c tries to connect before testing
> if the slot is NONE / NULL. So it doesn't work to DISABLE the
> subscription and set the slot to NONE.
So I'm seeing this:
if (!slotname && rstates == NIL)
{
table_close(rel, NoLock);
return;
}
load_file("libpqwalreceiver", false);
wrconn = walrcv_connect(conninfo, true, must_use_password,
subname, &err);
That looks like it's intended to return if there's nothing to do, and
the comments say as much. But that (!slotname && rstates == NIL) test
looks sketchy. It seems like we should bail out early if *either*
!slotname *or* rstates == NIL, or for that matter if all of the
rstates have rstate->relid == 0 or rstate->state ==
SUBREL_STATE_SYNCDONE. Maybe we need to push setting up the connection
inside the foreach(lc, rstates) loop and do it only once we're sure we
want to do something. Or at least, I don't understand why we don't
bail out immediately in all cases where slotname is NULL, regardless
of rstates. Am I missing something here?
> Reading the code, I think I understand why the postgres user cannot drop
> the slot:
>
> * the owner is sub_owner (not a superuser)
> * and form->subpasswordrequired is true
>
> Should there be a test to check if the user executing the query is
> superuser? maybe it's handled differently? (I am not very familiar with
> the code).
I think that there normally shouldn't be any problem here, because if
form->subpasswordrequired is true, we expect that the connection
string should contain a password which the remote side actually uses,
or we expect the subscription to be owned by the superuser. If neither
of those things is the case, then either the superuser made a
subscription that doesn't use a password owned by a non-superuser
without fixing subpasswordrequired, or else the configuration on the
remote side has changed so that it now doesn't use the password when
formerly it did. In the first case, perhaps it would be fine to go
ahead and drop the slot, but in the second case I don't think it's OK
from a security point view, because the command is going to behave the
same way no matter who executes the drop command, and a non-superuser
who drops the slot shouldn't be permitted to rely on the postgres
processes's identity to do anything on a remote node -- including
dropping a relation slot. So I tentatively think that this behavior is
correct.
> I dont understand (yet?) why I can do ALTER SUBSCRIPTIONs after changing
> the ownership to an unpriviledged user (must_use_password should be true
> also in that case).
Maybe you're altering it in a way that doesn't involve any connections
or any changes to the connection string? There's no security issue if,
say, you rename it.
--
Robert Haas
EDB: http://www.enterprisedb.com
Re: CREATE FUNCTION ... SEARCH { DEFAULT | SYSTEM | SESSION }
On Thu, 2023-09-21 at 14:06 -0400, Robert Haas wrote: > Also, in a case like this, I don't think it's unreasonable to ask > whether, perhaps, Bob just needs to be a little more careful about > setting search_path. That's what this whole thread is about: I wish it was reasonable, but I don't think the tools we provide today make it reasonable. You expect Bob to do something like: CREATE FUNCTION ... SET search_path = pg_catalog, pg_temp ... for all functions, not just SECURITY DEFINER functions, is that right? Up until now, we've mostly treated search_path as a problem for SECURITY DEFINER, and specifying something like that might be reasonable for a small number of SECURITY DEFINER functions. But as my example showed, search_path is actually a problem for SECURITY INVOKER too: an index expression relies on the function producing the correct results, and it's hard to control that without controlling the search_path. > I think that there is a big difference between > (a) defining a SQL-language function that is accessible to multiple > users and (b) inserting a row into a table you don't own. When you > define a function, you know people are potentially going to call it. It's a bit problematic that (a) is the default: CREATE FUNCTION f(INT) RETURNS INT IMMUTABLE LANGUAGE plpgsql AS $$ BEGIN RETURN 42+$1; END; $$; CREATE TABLE x(i INT); CREATE INDEX x_idx ON x(f(i)); GRANT INSERT ON TABLE x TO u2; It's not obvious that f() is directly callable by u2 (though it is documented). I'm not disagreeing with the principle behind what you say above. My point is that "accessible to multiple users" is the ordinary default case, so there's no cue for the user that they need to do something special to secure function f(). > Asking you, as the function author, to take some care to secure your > function against a malicious search_path doesn't seem like an > unsupportable burden. What you are suggesting has been possible for quite some time. Do you think users are taking care to do this today? If not, how can we encourage them to do so? > You can, I think, be expected to > check that functions you define have SET search_path attached. We've already established that even postgres hackers are having difficulty keeping up with these nuances. Even though the SET clause has been there for a long time, our documentation on the subject is insufficient and misleading. And on top of that, it's extra typing and noise for every schema file. Until we make some changes I don't think we can expect users to do as you suggest. Regards, Jeff Davis
Is this a problem in GenericXLogFinish()?
src/backend/transam/README says:
...
4. Mark the shared buffer(s) as dirty with MarkBufferDirty(). (This
must happen before the WAL record is inserted; see notes in
SyncOneBuffer().)
...
But GenericXLogFinish() does this:
...
/* Insert xlog record */
lsn = XLogInsert(RM_GENERIC_ID, 0);
/* Set LSN and mark buffers dirty */
for (i = 0; i < MAX_GENERIC_XLOG_PAGES; i++)
{
PageData *pageData = &state->pages[i];
if (BufferIsInvalid(pageData->buffer))
continue;
PageSetLSN(BufferGetPage(pageData->buffer), lsn);
MarkBufferDirty(pageData->buffer);
}
END_CRIT_SECTION();
Am I missing something or is that a problem?
Regards,
Jeff Davis
Re: Better help output for pgbench -I init_steps
The following review has been posted through the commitfest application: make installcheck-world: not tested Implements feature: tested, passed Spec compliant: not tested Documentation:not tested Hello, I've reviewed all 4 of your patches, each one applies and builds correctly. > I think I prefer variant 2. Currently, we only have 8 steps, so it might > be overkill to separate them out into a different option. +1 to this from Peter. Variant 2 is nicely formatted with lots of information which I feel better solves the problem this patch is trying to address. Both versions 3 and 4 are a bit too jumbled for my liking without adding anything significant, even the shortened version 4. If we were to go with variant 1 however, I think it would add more to have a link to the pgbench documentation that refers to the different init steps. Perhaps on a new line just under where it says "see pgbench documentation for a description of these steps". Overall good patch, I'm a firm believer that more information is always better than less. Tristen --- Software Engineer HighGo Software Inc. (Canada) tristen.r...@highgo.ca www.highgo.ca
Re: Add support for AT LOCAL
The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: tested, passed Documentation:tested, passed Hello I think this feature can be a useful addition in dealing with time zones. I have applied and tried out the patch, The feature works as described and seems promising. The problem with compilation failure was probably reported on CirrusCI when compiled on different platforms. I have run the latest patch on my own Cirrus CI environment and everything checked out fine. Thank you Cary Huang -- Highgo Software Canada www.highgo.ca
Re: Add support for AT LOCAL
On 9/22/23 23:46, cary huang wrote: The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: tested, passed Documentation:tested, passed Hello I think this feature can be a useful addition in dealing with time zones. I have applied and tried out the patch, The feature works as described and seems promising. The problem with compilation failure was probably reported on CirrusCI when compiled on different platforms. I have run the latest patch on my own Cirrus CI environment and everything checked out fine. Thank you Thank you for reviewing! -- Vik Fearing
Re: Synchronizing slots from primary to standby
On Fri, Sep 22, 2023 at 6:01 PM Drouvot, Bertrand wrote: > > Thanks for all the work that has been done on this feature, and sorry > to have been quiet on it for so long. > > On 9/18/23 12:22 PM, shveta malik wrote: > > On Wed, Sep 13, 2023 at 4:48 PM Hayato Kuroda (Fujitsu) > > wrote: > >> Right, but I wanted to know why it is needed. One motivation seemed to > >> know the > >> WAL location of physical standby, but I thought that struct WalSnd.apply > >> could > >> be also used. Is it bad to assume that the physical walsender always > >> exists? > >> > > > > We do not plan to target this case where physical slot is not created > > between primary and physical-standby in the first draft. In such a > > case, slot-synchronization will be skipped for the time being. We can > > extend this functionality (if needed) later. > > > > I do think it's needed to extend this functionality. Having physical slot > created sounds like a (too?) strong requirement as: > > - It has not been a requirement for Logical decoding on standby so that could > sounds weird > to require it for sync slot (while it's not allowed to logical decode from > sync slots) > There is a difference here that we also need to prevent removal of rows required by sync_slots. That could be achieved by physical slot (and hot_standby_feedback). So, having a requirement to have physical slot doesn't sound too unreasonable to me. Otherwise, we need to invent some new mechanism of having some sort of placeholder slot to avoid removal of required rows. I guess we can always extend the functionality in later version as Shveta mentioned. Now, if we have somewhat simpler way to achieve prevention of removal of rows then it is fine otherwise let's focus on getting other parts correct considering this is already a reasonably big and complex patch. Thanks for looking into this work and your feedback will definetely help in moving this work forward. -- With Regards, Amit Kapila.
Failures on gombessa -- EIO?
I am not sure why REL_16_STABLE fails consistently as of ~4 days ago. It seems like bad storage or something? Just now it happened also on HEAD. I wonder why it would be sensitive to the branch.
Re: Questions about the new subscription parameter: password_required
On Fri, 2023-09-22 at 08:36 -0400, Robert Haas wrote: > On Fri, Sep 22, 2023 at 4:25 AM Benoit Lobréau > wrote: > > Can we consider adding something like this to clarify? > > > > """ > > This parameter is enforced when the CREATE SUBSCRIPTION or ALTER > > SUBSCRIPTION .. CONNECTION commands are executed. Therefore, it's > > possible to alter the ownership of a subscription with > > password_required=true to a non-superuser. > > """ > > I'm not sure of the exact wording, but there was another recent > thread > complaining about this being unclear, so it seems like some > clarification is needed. IIUC there is really one use case here, which is for superuser to define a subscription including the connection, and then change the owner to a non-superuser to actually run it (without being able to touch the connection string itself). I'd just document that in its own section, and mention a few caveats / mistakes to avoid. For instance, when the superuser is defining the connection, don't forget to set password_required=false, so that when you reassign to a non-superuser then the connection doesn't break. Regards, Jeff Davis
nbtree's ScalarArrayOp array mark/restore code appears to be buggy
Attached test case demonstrates an issue with nbtree's mark/restore code. All supported versions are affected. My suspicion is that bugfix commit 70bc5833 missed some subtlety around what we need to do to make sure that the array keys stay "in sync" with the scan. I'll have time to debug the problem some more tomorrow. My ScalarArrayOp project [1] seems unaffected by the bug, so I don't expect it'll take long to get to the bottom of this. This is probably due to its general design, and not any specific detail. The patch makes the relationship between the current scan position and the current array keys a great deal looser. [1] https://commitfest.postgresql.org/44/4455/ -- Peter Geoghegan nbtree_array_mark_restore_bug.sql Description: Binary data
RE: [PoC] pg_upgrade: allow to upgrade publisher node
Dear Bharath,
Again, thank you for reviewing! Here is a new version patch.
> 1.
> +{ oid => '8046', descr => 'for use by pg_upgrade',
> + proname => 'binary_upgrade_validate_wal_records', proisstrict => 'f',
> + provolatile => 'v', proparallel => 'u', prorettype => 'bool',
>
> +if (PG_ARGISNULL(0))
> +elog(ERROR, "null argument to
> binary_upgrade_validate_wal_records is not allowed");
>
> Can proisstrict => 'f' be removed so that there's no need for explicit
> PG_ARGISNULL check? Any specific reason to keep it?
Theoretically it could be, but I was not sure. I think you wanted us to follow
specs of pg_walinspect functions, but it is just a upgrade function. Normally
users cannot call it. Also, as Amit said [1], the caller must consider the
special case. Currently the function returns false at that time, we can change
more appropriate style later.
> And, the before the ISNULL check the arg is read, which isn't good.
Right, fixed.
> 2.
> +Datum
> +binary_upgrade_validate_wal_records(PG_FUNCTION_ARGS)
>
> The function name looks too generic in the sense that it validates WAL
> records for correctness/corruption, but it is not. Can it be something
> like binary_upgrade_{check_for_wal_logical_end,
> check_for_logical_end_of_wal} or such?
Per discussion [2], changed to binary_upgrade_validate_wal_logical_end.
> 3.
> +/* Quick exit if the given lsn is larger than current one */
> +if (start_lsn >= GetFlushRecPtr(NULL))
> +PG_RETURN_BOOL(false);
> +
>
> An LSN that doesn't exists yet is an error IMO, may be an error better here?
We think that the invalid slots should be listed at the end, so basically we do
not want to error out. This would be also changed if there are better opinions.
> 4.
> + * This function is used to verify that there are no WAL records (except some
> + * types) after confirmed_flush_lsn of logical slots, which means all the
> + * changes were replicated to the subscriber. There is a possibility that
> some
> + * WALs are inserted during upgrade, so such types would be ignored.
> + *
>
> This comment before the function better be at the callsite of the
> function, because as far as this function is concerned, it checks if
> there are any WAL records that are not "certain" types after the given
> LSN, it doesn't know logical slots or confirmed_flush_lsn or such.
Hmm, I think it is better to do the reverse, because otherwise we need to
mention
the same explanation at other caller of the function if any. So, I have
adjusted the comments atop and at caller. Thought?
> 8.
> +if (nslots_on_new)
> +pg_fatal("expected 0 logical replication slots but found %d",
> + nslots_on_new);
>
> How about "New cluster database is containing logical replication
> slots", note that the some of the fatal messages are starting with an
> upper-case letter.
I did not use your suggestion, but changed to upper-case.
Actually, the uppper-case rule is broken even in the file. Here I regarded
this sentence as hint message.
> 9.
> +res = executeQueryOrDie(conn, "SHOW wal_level;");
> +res = executeQueryOrDie(conn, "SHOW max_replication_slots;");
>
> Instead of 2 queries to determine required parameters, isn't it better
> with a single query like the following?
>
> select setting from pg_settings where name in ('wal_level',
> 'max_replication_slots') order by name;
Modified, but use ORDER BY ... DESC. This come from a previous comment [3].
>
> 12.
> +extern XLogReaderState *InitXLogReaderState(XLogRecPtr lsn);
> +extern XLogRecord *ReadNextXLogRecord(XLogReaderState *xlogreader);
> +
>
> Why not these functions be defined in xlogreader.h with elog/ereport
> in #ifndef FRONTEND #endif blocks? IMO, xlogreader.h seems right
> location for these functions.
I checked comments atop both files, and xlogreader.h seems better. Fixed.
> 13.
> +LogicalReplicationSlotInfo
>
> Where is this structure defined?
Opps, removed.
[1]:
https://www.postgresql.org/message-id/CAA4eK1LxPDeSkTttEAG2MPEWO%3D83vQe_Bja9F4QcCjVn%3DWt9rA%40mail.gmail.com
[2]:
https://www.postgresql.org/message-id/CAA4eK1L9oJmdxprFR3oob5KLpHUnkJAt5Le4woxO3wHz-SZ%2BTA%40mail.gmail.com
[3]:
https://www.postgresql.org/message-id/CAA4eK1LHH_%3DwbxsEn20%3DW%2Bqz1193OqFj-vvJ-u0uHLMmwLHbRw%40mail.gmail.com
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
v44-0001-pg_upgrade-Allow-to-replicate-logical-replicatio.patch
Description: v44-0001-pg_upgrade-Allow-to-replicate-logical-replicatio.patch
RE: [PoC] pg_upgrade: allow to upgrade publisher node
Dear Bharath, > > You mentioned at line 118, but at that time logical replication system is > > not > created. > > The subscriber is created at line 163. > > Therefore WALs would not be consumed automatically. > > So, not calling pg_logical_slot_get_changes() on test_slot1 won't > consume the WAL? Yes. This slot was created manually and no one activated it automatically. pg_logical_slot_get_changes() can consume WALs but never called. > > 2. > +++ b/src/bin/pg_upgrade/t/003_logical_replication_slots.pl > > How about a more descriptive and pointed name for the TAP test file, > something like 003_upgrade_logical_replication_slots.pl? Good suggestion. Renamed. > 3. Does this patch support upgrading of logical replication slots on a > streaming standby? If yes, isn't it a good idea to add one test for > upgrading standby with logical replication slots? IIUC pg_upgrade would not be used for physical standby. The standby would be upgrade by: * Recreating the database cluster, or * Executing rsync command. For more detail, please see the documentation. Best Regards, Hayato Kuroda FUJITSU LIMITED
Re: Invalidate the subscription worker in cases where a user loses their superuser status
On Sat, Sep 23, 2023 at 1:27 AM vignesh C wrote:
>
>
> Fixed this issue by checking if the subscription owner has changed
> from superuser to non-superuser in case the pg_authid rows changes.
> The attached patch has the changes for the same.
>
@@ -3952,7 +3953,9 @@ maybe_reread_subscription(void)
newsub->passwordrequired != MySubscription->passwordrequired ||
strcmp(newsub->origin, MySubscription->origin) != 0 ||
newsub->owner != MySubscription->owner ||
- !equal(newsub->publications, MySubscription->publications))
+ !equal(newsub->publications, MySubscription->publications) ||
+ (!superuser_arg(MySubscription->owner) &&
+ MySubscription->isownersuperuser))
{
if (am_parallel_apply_worker())
ereport(LOG,
@@ -4605,6 +4608,13 @@ InitializeLogRepWorker(void)
proc_exit(0);
}
+ /*
+ * Fetch subscription owner is a superuser. This value will be later
+ * checked to see when there is any change with this role and the worker
+ * will be restarted if required.
+ */
+ MySubscription->isownersuperuser = superuser_arg(MySubscription->owner);
Why didn't you filled this parameter in GetSubscription() like other
parameters? If we do that then the comparison of first change in your
patch will look similar to all other comparisons.
--
With Regards,
Amit Kapila.
