Re: bogus: logical replication rows/cols combinations

[Amit Kapila]

On Mon, May 2, 2022 at 11:01 AM Amit Kapila  wrote:
>
> On Mon, May 2, 2022 at 3:27 AM Tomas Vondra
>  wrote:
> >
> > On 4/30/22 12:11, Amit Kapila wrote:
> > > On Sat, Apr 30, 2022 at 3:01 PM Alvaro Herrera  
> > > wrote:
> > >>
> > >> My proposal is that if users want to define multiple publications, and
> > >> their definitions conflict in a way that would behave ridiculously (==
> > >> bound to cause data inconsistencies eventually), an error should be
> > >> thrown.  Maybe we will not be able to catch all bogus cases, but we can
> > >> be prepared for the most obvious ones, and patch later when we find
> > >> others.
> > >>
> > >
> > > I agree with throwing errors for obvious/known bogus cases but do we
> > > want to throw errors or restrict the combining of column lists when
> > > row filters are present in all cases? See some examples [1 ] where it
> > > may be valid to combine them.
> > >
> >
> > I think there are three challenges:
> >
> > (a) Deciding what's an obvious bug or an unsupported case (e.g. because
> > it's not clear what's the correct behavior / way to merge column lists).
> >
> > (b) When / where to detect the issue.
> >
> > (c) Making sure this does not break/prevent existing use cases.
> >
> >
> > As I said before [1], I think the issue stems from essentially allowing
> > DML to have different row filters / column lists. So we could forbid
> > publications to specify WITH (publish=...) and one of the two features,
> >
>
> I don't think this is feasible for row filters because that would mean
> publishing all actions because we have a restriction that all columns
>

Read the above sentence as: "publishing all actions and we have a
restriction that all columns ..."

> referenced in the row filter expression are part of the REPLICA
> IDENTITY index. This restriction is only valid for updates/deletes, so
> if we allow all pubactions then this will be imposed on inserts as
> well. A similar restriction is there for column lists as well, so I
> don't think we can do it there as well. Do you have some idea to
> address it?
>
> > or make sure subscription does not combine multiple such publications.
> >
>
> Yeah, or don't allow to define such publications in the first place so
> that different subscriptions can't combine them but I guess that might
> forbid some useful cases as well where publication may not get
> combined with other publications.
>
> > The second option has the annoying consequence that it makes this
> > useless for the "data redaction" use case I described in [2], because
> > that relies on combining multiple publications.
> >
>
> True, but as a workaround users can create different subscriptions for
> different publications.
>
> > Furthermore, what if the publications change after the subscriptions get
> > created? Will we be able to detect the error etc.?
> >
>
> I think from that apart from 'Create Subscription', the same check
> needs to be added for Alter Subscription ... Refresh, Alter
> Subscription ... Enable.
>
> In the publication side, we need an additional check in Alter
> Publication ... SET table variant. One idea is that we get all other
> publications for which the corresponding relation is defined. And then
> if we find anything which we don't want to allow then we can throw an
> error. This will forbid some useful cases as well as mentioned above.
> So, the other possibility is to expose all publications for a
> walsender, and then we can find the exact set of publications where
> the current publication is used with other publications and we can
> check only those publications. So, if we have three walsenders
> (walsnd1: pub1, pub2; walsnd2 pub2; walsnd3: pub2, pub3) in the system
> and we are currently altering publication pub1 then we need to check
> only pub3 for any conflicting conditions.
>

Typo, it should be pub2 instead of pub3 in the above sentence.

> Yet another simple way could
> be that we don't allow to change column list via Alter Publication ...
> Set variant because the other variants anyway need REFRESH publication
> which we have covered.
>
> I think it is tricky to decide what exactly we want to forbid, so, we
> may want to follow something simple like if the column list and row
> filters for a table are different in the required set of publications
> then we treat it as an unsupported case. I think this will prohibit
> some useful cases but should probably forbid the cases we are worried
> about here.
>

-- 
With Regards,
Amit Kapila.

Re: bogus: logical replication rows/cols combinations

[Amit Kapila]

On Mon, May 2, 2022 at 3:27 AM Tomas Vondra
 wrote:
>
> On 4/30/22 12:11, Amit Kapila wrote:
> > On Sat, Apr 30, 2022 at 3:01 PM Alvaro Herrera  
> > wrote:
> >>
> >> My proposal is that if users want to define multiple publications, and
> >> their definitions conflict in a way that would behave ridiculously (==
> >> bound to cause data inconsistencies eventually), an error should be
> >> thrown.  Maybe we will not be able to catch all bogus cases, but we can
> >> be prepared for the most obvious ones, and patch later when we find
> >> others.
> >>
> >
> > I agree with throwing errors for obvious/known bogus cases but do we
> > want to throw errors or restrict the combining of column lists when
> > row filters are present in all cases? See some examples [1 ] where it
> > may be valid to combine them.
> >
>
> I think there are three challenges:
>
> (a) Deciding what's an obvious bug or an unsupported case (e.g. because
> it's not clear what's the correct behavior / way to merge column lists).
>
> (b) When / where to detect the issue.
>
> (c) Making sure this does not break/prevent existing use cases.
>
>
> As I said before [1], I think the issue stems from essentially allowing
> DML to have different row filters / column lists. So we could forbid
> publications to specify WITH (publish=...) and one of the two features,
>

I don't think this is feasible for row filters because that would mean
publishing all actions because we have a restriction that all columns
referenced in the row filter expression are part of the REPLICA
IDENTITY index. This restriction is only valid for updates/deletes, so
if we allow all pubactions then this will be imposed on inserts as
well. A similar restriction is there for column lists as well, so I
don't think we can do it there as well. Do you have some idea to
address it?

> or make sure subscription does not combine multiple such publications.
>

Yeah, or don't allow to define such publications in the first place so
that different subscriptions can't combine them but I guess that might
forbid some useful cases as well where publication may not get
combined with other publications.

> The second option has the annoying consequence that it makes this
> useless for the "data redaction" use case I described in [2], because
> that relies on combining multiple publications.
>

True, but as a workaround users can create different subscriptions for
different publications.

> Furthermore, what if the publications change after the subscriptions get
> created? Will we be able to detect the error etc.?
>

I think from that apart from 'Create Subscription', the same check
needs to be added for Alter Subscription ... Refresh, Alter
Subscription ... Enable.

In the publication side, we need an additional check in Alter
Publication ... SET table variant. One idea is that we get all other
publications for which the corresponding relation is defined. And then
if we find anything which we don't want to allow then we can throw an
error. This will forbid some useful cases as well as mentioned above.
So, the other possibility is to expose all publications for a
walsender, and then we can find the exact set of publications where
the current publication is used with other publications and we can
check only those publications. So, if we have three walsenders
(walsnd1: pub1, pub2; walsnd2 pub2; walsnd3: pub2, pub3) in the system
and we are currently altering publication pub1 then we need to check
only pub3 for any conflicting conditions. Yet another simple way could
be that we don't allow to change column list via Alter Publication ...
Set variant because the other variants anyway need REFRESH publication
which we have covered.

I think it is tricky to decide what exactly we want to forbid, so, we
may want to follow something simple like if the column list and row
filters for a table are different in the required set of publications
then we treat it as an unsupported case. I think this will prohibit
some useful cases but should probably forbid the cases we are worried
about here.

-- 
With Regards,
Amit Kapila.

Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set

[Noah Misch]

On Fri, Apr 01, 2022 at 10:16:48AM +0900, Michael Paquier wrote:
> On Thu, Mar 31, 2022 at 09:49:50AM -0400, Tom Lane wrote:
> > Well, let's go ahead with it and see what happens.  If it's too
> > much of a mess we can always revert.
> 
> Okay, done after an extra round of self-review.

commit 322becb wrote:
> --- /dev/null
> +++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl

> +# Generate a database with a name made of a range of ASCII characters.
> +sub generate_db
> +{
> + my ($node, $from_char, $to_char) = @_;
> +
> + my $dbname = '';
> + for my $i ($from_char .. $to_char)
> + {
> + next if $i == 7 || $i == 10 || $i == 13;# skip BEL, LF, and 
> CR
> + $dbname = $dbname . sprintf('%c', $i);
> + }
> + $node->run_log(
> + [ 'createdb', '--host', $node->host, '--port', $node->port, 
> $dbname ]
> + );

Nothing checks the command result, so the test file passes even if each of
these createdb calls fails.  Other run_log() calls in this file have the same
problem.  This particular one should be command_ok() or similar.

--host and --port are redundant in a PostgreSQL::Test::Cluster::run_log call,
because that call puts equivalent configuration in the environment.  Other
calls in the file have the same redundant operands.  (No other test file has
redundant --host or --port.)

> + # Grab any regression options that may be passed down by caller.
> + my $extra_opts_val = $ENV{EXTRA_REGRESS_OPT} || "";

Typo: s/_OPT/_OPTS/

> + my @extra_opts = split(/\s+/, $extra_opts_val);

src/test/recovery/t/027_stream_regress.pl and the makefiles treat
EXTRA_REGRESS_OPTS as a shell fragment.  To be compatible, use the
src/test/recovery/t/027_stream_regress.pl approach.  Affected usage patetrns
are not very important, but since the tree has code for it, you may as well
borrow that code.  These examples witness the difference:

EXTRA_REGRESS_OPTS='--nosuc" h"' MAKEFLAGS= make -C src/bin/pg_upgrade check 
PROVE_TESTS=t/002_pg_upgrade.pl
# log has: 
/home/nm/src/pg/postgresql/src/bin/pg_upgrade/../../../src/test/regress/pg_regress:
 unrecognized option '--nosuc"'
EXTRA_REGRESS_OPTS='--nosuc" h"' MAKEFLAGS= make -C src/test/recovery check 
PROVE_TESTS=t/027_stream_regress.pl
# log has: 
/home/nm/src/pg/postgresql/src/test/recovery/../../../src/test/regress/pg_regress:
 unrecognized option '--nosuc h'

> --- a/src/bin/pg_upgrade/test.sh
> +++ /dev/null

> -# Create databases with names covering the ASCII bytes other than NUL, BEL,
> -# LF, or CR.  BEL would ring the terminal bell in the course of this test, 
> and
> -# it is not otherwise a special case.  PostgreSQL doesn't support the rest.
> -dbname1=`awk 'BEGIN { for (i= 1; i < 46; i++)
> - if (i != 7 && i != 10 && i != 13) printf "%c", i }'  -# Exercise backslashes adjacent to double quotes, a Windows special case.
> -dbname1='\"\'$dbname1'\\"\\\'

This rewrite dropped the exercise of backslashes adjacent to double quotes.

Re: Add index scan progress to pg_stat_progress_vacuum

[Masahiko Sawada]

On Thu, Apr 14, 2022 at 10:32 AM Imseih (AWS), Sami  wrote:
>
> Taking the above feedback, I modified the patches
> and I believe this approach should be acceptable.
>
> For now, I also reduced the scope to only exposing
> Indexes_total and indexes_completed in
> pg_stat_progress_vacuum. I will create a new CF entry
> for the new view pg_stat_progress_vacuum_index.
>
> V10-0001: This patch adds a callback to ParallelContext
> that could be called by the leader in vacuumparallel.c
> and more importantly while the leader is waiting
> for the parallel workers to complete inside.

Thank you for updating the patch! The new design looks much better to me.

 typedef struct ParallelWorkerInfo
@@ -46,6 +49,8 @@ typedef struct ParallelContext
ParallelWorkerInfo *worker;
int nknown_attached_workers;
bool   *known_attached_workers;
+   ParallelProgressCallback parallel_progress_callback;
+   void*parallel_progress_callback_arg;
 } ParallelContext;

I think we can pass the progress update function to
WaitForParallelWorkersToFinish(), which seems simpler. And we can call
the function after updating the index status to
PARALLEL_INDVAC_STATUS_COMPLETED.

BTW, currently we don't need a lock for touching index status since
each worker touches different indexes. But after this patch, the
leader will touch all index status, do we need a lock for that?

Regards,

--
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

Re: Logical replication timeout problem

[Masahiko Sawada]

On Mon, May 2, 2022 at 11:32 AM Amit Kapila  wrote:
>
> On Mon, May 2, 2022 at 7:33 AM Masahiko Sawada  wrote:
> > On Thu, Apr 28, 2022 at 7:01 PM houzj.f...@fujitsu.com
> >  wrote:
> > >
> > > Hi Sawada-san, Wang
> > >
> > > I was looking at the patch and noticed that we moved some logic from
> > > update_replication_progress() to OutputPluginUpdateProgress() like
> > > your suggestion.
> > >
> > > I have a question about this change. In the patch we added some
> > > restriction in this function OutputPluginUpdateProgress() like below:
> > >
> > > + /*
> > > + * If we are at the end of transaction LSN, update progress tracking.
> > > + * Otherwise, after continuously processing CHANGES_THRESHOLD changes, we
> > > + * try to send a keepalive message if required.
> > > + */
> > > + if (ctx->end_xact || ++changes_count >= CHANGES_THRESHOLD)
> > > + {
> > > + ctx->update_progress(ctx, ctx->write_location, ctx->write_xid,
> > > + skipped_xact);
> > > + changes_count = 0;
> > > + }
> > >
> > > After the patch, we won't be able to always invoke the update_progress() 
> > > if the
> > > caller are at the middle of transaction(e.g. end_xact = false). The 
> > > bebavior of the
> > > public function OutputPluginUpdateProgress() is changed. I am thinking is 
> > > it ok to
> > > change this at back-branches ?
> > >
> > > Because OutputPluginUpdateProgress() is a public function for the 
> > > extension
> > > developer, just a little concerned about the behavior change here.
> >
> > Good point.
> >
> > As you pointed out, it would not be good if we change the behavior of
> > OutputPluginUpdateProgress() in back branches. Also, after more
> > thought, it is not a good idea even for HEAD since there might be
> > background workers that use logical decoding and the timeout checking
> > might not be relevant at all with them.
> >
>
> So, shall we go back to the previous approach of using a separate
> function update_replication_progress?

Ok, agreed.

>
> > BTW, I think you're concerned about the plugins that call
> > OutputPluginUpdateProgress() at the middle of the transaction (i.e.,
> > end_xact = false). We have the following change that makes the
> > walsender not update the progress at the middle of the transaction. Do
> > you think it is okay since it's not common usage to call
> > OutputPluginUpdateProgress() at the middle of the transaction by the
> > plugin that is used by the walsender?
> >
>
> We have done that purposefully as otherwise, the lag tracker shows
> incorrect information. See email [1]. The reason is that we always get
> ack from subscribers for transaction end. Also, prior to this patch we
> never call the lag tracker recording apart from the transaction end,
> so as a bug fix we shouldn't try to change it.

Make sense.

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

Re: Logical replication timeout problem

[Amit Kapila]

On Mon, May 2, 2022 at 7:33 AM Masahiko Sawada  wrote:
> On Thu, Apr 28, 2022 at 7:01 PM houzj.f...@fujitsu.com
>  wrote:
> >
> > Hi Sawada-san, Wang
> >
> > I was looking at the patch and noticed that we moved some logic from
> > update_replication_progress() to OutputPluginUpdateProgress() like
> > your suggestion.
> >
> > I have a question about this change. In the patch we added some
> > restriction in this function OutputPluginUpdateProgress() like below:
> >
> > + /*
> > + * If we are at the end of transaction LSN, update progress tracking.
> > + * Otherwise, after continuously processing CHANGES_THRESHOLD changes, we
> > + * try to send a keepalive message if required.
> > + */
> > + if (ctx->end_xact || ++changes_count >= CHANGES_THRESHOLD)
> > + {
> > + ctx->update_progress(ctx, ctx->write_location, ctx->write_xid,
> > + skipped_xact);
> > + changes_count = 0;
> > + }
> >
> > After the patch, we won't be able to always invoke the update_progress() if 
> > the
> > caller are at the middle of transaction(e.g. end_xact = false). The 
> > bebavior of the
> > public function OutputPluginUpdateProgress() is changed. I am thinking is 
> > it ok to
> > change this at back-branches ?
> >
> > Because OutputPluginUpdateProgress() is a public function for the extension
> > developer, just a little concerned about the behavior change here.
>
> Good point.
>
> As you pointed out, it would not be good if we change the behavior of
> OutputPluginUpdateProgress() in back branches. Also, after more
> thought, it is not a good idea even for HEAD since there might be
> background workers that use logical decoding and the timeout checking
> might not be relevant at all with them.
>

So, shall we go back to the previous approach of using a separate
function update_replication_progress?

> BTW, I think you're concerned about the plugins that call
> OutputPluginUpdateProgress() at the middle of the transaction (i.e.,
> end_xact = false). We have the following change that makes the
> walsender not update the progress at the middle of the transaction. Do
> you think it is okay since it's not common usage to call
> OutputPluginUpdateProgress() at the middle of the transaction by the
> plugin that is used by the walsender?
>

We have done that purposefully as otherwise, the lag tracker shows
incorrect information. See email [1]. The reason is that we always get
ack from subscribers for transaction end. Also, prior to this patch we
never call the lag tracker recording apart from the transaction end,
so as a bug fix we shouldn't try to change it.

[1] - 
https://www.postgresql.org/message-id/OS3PR01MB62755D216245199554DDC8DB9EEA9%40OS3PR01MB6275.jpnprd01.prod.outlook.com

-- 
With Regards,
Amit Kapila.

Re: ERROR: type of parameter 1 (fruit2) does not match that when preparing the plan (fruit1)

[David G. Johnston]

Moving discussion to -hackers

On Sun, May 1, 2022 at 12:46 PM Tom Lane  wrote:

> "David G. Johnston"  writes:
> > On Sun, May 1, 2022 at 10:08 AM Tom Lane  wrote:
> >> Maybe we could improve this situation by treating a "record" parameter
> >> as polymorphic, though that might cause some odd inconsistencies with
> >> plpgsql's historical treatment of "record" local variables.
>
> > The extent of needing to treat "record" as polymorphic-like seems like it
> > would be limited to resolve_polymorphic_argtype in funcapi.c.  Namely, in
> > computing the hash key for the compiled hash entry for the function.
> > Similar to how we append the trigger oid in compute_function_hashkey in
> > pl.compile (which ultimately calls the former) so trigger invocations
> > become per-table.
>
> I'm hesitant to touch funcapi.c for this; the scope of potential
> side-effects becomes enormous as soon as you do.
>
>
Agreed, though the only caller of this particular function seems to be in
plpgsql/pl_comp.c anyway...one more-or-less directly from do_compile and
the other via compute_function_hashkey, so its presence in funcapi.c seems
unusual.

But, to get rid of the cache error it seems sufficient to simply ensure we
compute a hash key that considers the called types.

That do_compile doesn't see these substitutions is a positive for minimal
potential impact with no apparent downside.

I added the test case from the -bug email (I'd probably change it to match
the "scenario" for the file for a final version).

 I have some questions in the code that I could probably get answers for
via tests - would including those tests be acceptable (defaults,
named-argument-calling, output argmode)?

I did check-world without issue - but I suspect this to be under-tested and
not naturally used in the course of unrelated testing.

David J.

+   /*
+* Saved compiled functions with record-typed input args to a
hashkey
+* that substitutes all known actual composite type OIDs in the
+* call signature for the corresponding generic record OID from
+* the definition signature.  This avoids a probable error:
+* "type of parameter ... does not match that when preparing the
plan"
+* when such a record variable is used in a query within the
function.
+*/
+   for (int i = 0; i < procStruct->pronargs; i++)
+   {
+   if (hashkey->argtypes[i] != RECORDOID)
+   continue;
+
+   // ??? I presume other parts of the system synchronize these
two arrays
+   // In particular for default arguments not specified in the
function call
+   // or named-argument function call syntax.
+
+   /* Don't bother trying to substitute once we run out of input
arguments */
+   if (i > fcinfo->nargs - 1)
+   break;
+
+   hashkey->argtypes[i] =
+   get_call_expr_argtype(fcinfo->flinfo->fn_expr, i);
+   }

+select record_to_form_data(fruit1) from fruit1;
+ record_to_form_data
+-
+ (1,apple,red)
+(1 row)
+
+select record_to_form_data(fruit2) from fruit2;
+ record_to_form_data
+-
+ (1,apple)
+(1 row)
+


v0001-cache-record-input-function-based-on-call-arguments.patch
Description: Binary data

Re: Logical replication timeout problem

[Masahiko Sawada]

On Thu, Apr 28, 2022 at 7:01 PM houzj.f...@fujitsu.com
 wrote:
>
> On Wednesday, April 20, 2022 3:21 PM Masahiko Sawada  
> wrote:
> >
> > BTW the changes in
> > REL_14_v1-0001-Fix-the-logical-replication-timeout-during-large-.patch,
> > adding end_xact to LogicalDecodingContext, seems good to me and it
> > might be better than the approach of v17 patch from plugin developers’
> > perspective? This is because they won’t need to pass true/false to
> > end_xact of  OutputPluginUpdateProgress(). Furthermore, if we do what
> > we do in update_replication_progress() in
> > OutputPluginUpdateProgress(), what plugins need to do will be just to
> > call OutputPluginUpdate() in every callback and they don't need to
> > have the CHANGES_THRESHOLD logic. What do you think?
>
> Hi Sawada-san, Wang
>
> I was looking at the patch and noticed that we moved some logic from
> update_replication_progress() to OutputPluginUpdateProgress() like
> your suggestion.
>
> I have a question about this change. In the patch we added some
> restriction in this function OutputPluginUpdateProgress() like below:
>
> + /*
> + * If we are at the end of transaction LSN, update progress tracking.
> + * Otherwise, after continuously processing CHANGES_THRESHOLD changes, we
> + * try to send a keepalive message if required.
> + */
> + if (ctx->end_xact || ++changes_count >= CHANGES_THRESHOLD)
> + {
> + ctx->update_progress(ctx, ctx->write_location, ctx->write_xid,
> + skipped_xact);
> + changes_count = 0;
> + }
>
> After the patch, we won't be able to always invoke the update_progress() if 
> the
> caller are at the middle of transaction(e.g. end_xact = false). The bebavior 
> of the
> public function OutputPluginUpdateProgress() is changed. I am thinking is it 
> ok to
> change this at back-branches ?
>
> Because OutputPluginUpdateProgress() is a public function for the extension
> developer, just a little concerned about the behavior change here.

Good point.

As you pointed out, it would not be good if we change the behavior of
OutputPluginUpdateProgress() in back branches. Also, after more
thought, it is not a good idea even for HEAD since there might be
background workers that use logical decoding and the timeout checking
might not be relevant at all with them.

BTW, I think you're concerned about the plugins that call
OutputPluginUpdateProgress() at the middle of the transaction (i.e.,
end_xact = false). We have the following change that makes the
walsender not update the progress at the middle of the transaction. Do
you think it is okay since it's not common usage to call
OutputPluginUpdateProgress() at the middle of the transaction by the
plugin that is used by the walsender?

 #define WALSND_LOGICAL_LAG_TRACK_INTERVAL_MS 1000
- if (!TimestampDifferenceExceeds(sendTime, now,
+ if (end_xact && TimestampDifferenceExceeds(sendTime, now,
  WALSND_LOGICAL_LAG_TRACK_INTERVAL_MS))
- return;
+ {
+ LagTrackerWrite(lsn, now);
+ sendTime = now;
+ }

Regards,

--
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

Re: Handle infinite recursion in logical replication setup

[Peter Smith]

Thanks for updating the patches for all my prior feedback.

For v12* I have only minor feedback for the docs.

==
V12-0001

no comments

==
V12-0002

1. Commit message

In another thread using the terminology "multi-master" seems to be
causing some problems. Maybe you should choose different wording in
this commit message to avoid having the same problems?

~~~

2. doc/src/sgml/logical-replication.sgml

These are all similar minor wording suggestions to split the sentences.
Wording: ", here copy_data is specified as XXX ..." -> ". Use
copy_data specified as XXX ..."

Also:
Wording: "... to subscribe the changes from nodeXXX..."
-> "... to subscribe to the changes from nodeXXX... " (add "to")
-> "... to subscribe to nodeXXX."  (or I preferred just remove the
whole "changes" part)

2a.
Create a subscription in node3 to subscribe the changes from node1,
here copy_data is specified as force so that the existing table data
is copied during initial sync:

SUGGESTION
Create a subscription in node3 to subscribe to node1. Use copy_data
specified as force so that the existing table data is copied during
initial sync:

2b.
Create a subscription in node1 to subscribe the changes fromnode3,
here copy_data is specified as force so that the existing table data
is copied during initial sync:

SUGGESTION
Create a subscription in node1 to subscribe to node3. Use copy_data
specified as force so that the existing table data is copied during
initial sync:

2c.
Create a subscription in node2 to subscribe the changes from node3,
here copy_data is specified as force so that the existing table data
is copied during initial sync:

SUGGESTION
Create a subscription in node2 to subscribe to node3. Use copy_data
specified as force so that the existing table data is copied during
initial sync:

2d.
Create a subscription in node3 to subscribe the changes from node1,
here copy_data is specified as force when creating a subscription to
node1 so that the existing table data is copied during initial sync:

SUGGESTION
Create a subscription in node3 to subscribe to node1. Use copy_data
specified as force when creating a subscription to node1 so that the
existing table data is copied during initial sync:

2e.
Create a subscription in node3 to subscribe the changes from node2,
here copy_data is specified as off as the initial table data would
have been copied in the earlier step:

SUGGESTION (this one has a bit more re-wording than the others)
Create a subscription in node3 to subscribe to node2. Use copy_data
specified as off because the initial table data would have been
already copied in the previous step:

~~~

3. doc/src/sgml/logical-replication.sgml

31.11.2. Adding new node when there is no data in any of the nodes
31.11.3. Adding new node when data is present in the existing nodes
31.11.4. Adding new node when data is present in the new node

Minor change to the above heading?

Wording: "Adding new node ..." -> "Adding a new node ..."

--
[1] 
https://www.postgresql.org/message-id/flat/CAHut%2BPuwRAoWY9pz%3DEubps3ooQCOBFiYPU9Yi%3DVB-U%2ByORU7OA%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia

Re: Tree-walker callbacks vs -Wdeprecated-non-prototype

[Tom Lane]

Thomas Munro  writes:
> As visible on seawasp (and noticed here in passing, while hacking on
> the opaque pointer changes for bleeding edge LLVM), Clang 15 now warns
> by default about our use of tree walkers functions with no function
> prototype, because the next revision of C (C23?) will apparently be
> harmonising with C++ in interpreting f() to mean f(void), not
> f(anything goes).

Ugh.  I wonder if we can get away with declaring the walker arguments
as something like "bool (*walker) (Node *, void *)" without having
to change all the actual walkers to be exactly that signature.
Having to insert casts in the walkers would be a major pain-in-the-butt.

regards, tom lane

Re: Libpq single-row mode slowness

[Daniele Varrazzo]

On Sun, 1 May 2022 at 23:12, Tom Lane  wrote:

> The usual expectation is that you call PQconsumeInput to get rid of
> a read-ready condition on the socket.  If you don't have a poll() or
> select() or the like in the loop, you might be wasting a lot of
> pointless recvfrom calls.  You definitely don't need to call
> PQconsumeInput if PQisBusy is already saying that a result is available,
> and in single-row mode it's likely that several results can be consumed
> per recvfrom call.

This makes sense and, with some refactoring of our fetch loop, the
overhead of using single-row mode is now down to about 3x, likely
caused by the greater overhead in Python calls.

Please note that the insight you gave in your answer seems to
contradict the documentation. Some excerpts of
https://www.postgresql.org/docs/current/libpq-async.html:

"""
PQconsumeInput:  "After calling PQconsumeInput , the application can
check PQisBusy and/or PQnotifies to see if their state has changed"

PQisBusy: "will not itself attempt to read data from the server;
therefore PQconsumeInput must be invoked first, or the busy state will
never end."

...
A typical application [will use select()]. When the main loop detects
input ready, it should call PQconsumeInput to read the input. It can
then call PQisBusy, followed by PQgetResult if PQisBusy returns false
(0).
"""

All these indications give the impression that there is a sort of
mandatory order, requiring to call first PQconsumeInput, then
PQisBusy. As a consequence, the core of our function to fetch a single
result was implemented as:

```
def fetch(pgconn):
while True:
pgconn.consume_input()
if not pgconn.is_busy():
break
yield Wait.R

return pgconn.get_result()
```

(Where the `yield Wait.R` suspends this execution to call into
select() or whatever waiting policy the program is using.)

Your remarks suggest that PQisBusy() can be called before
PQconsumeInput(), and that the latter doesn't need to be called if not
busy. As such I have modified the loop to be:

```
def fetch(pgconn):
if pgconn.is_busy():
yield Wait.R
while True:
pgconn.consume_input()
if not pgconn.is_busy():
break
yield Wait.R

return pgconn.get_result()
```

which seems to work well: tests don't show regressions and single-row
mode doesn't waste recvfrom() anymore.

Is this new fetching pattern the expected way to interact with the libpq?

If so, should we improve the documentation to suggest that there are
reasons to call PQisBusy before PQconsumeInput? Especially in the
single-row mode docs page, which doesn't make relevant mentions to the
use of these functions.

Thank you very much for your help, really appreciated.

-- Daniele

Tree-walker callbacks vs -Wdeprecated-non-prototype

[Thomas Munro]

Hi,

As visible on seawasp (and noticed here in passing, while hacking on
the opaque pointer changes for bleeding edge LLVM), Clang 15 now warns
by default about our use of tree walkers functions with no function
prototype, because the next revision of C (C23?) will apparently be
harmonising with C++ in interpreting f() to mean f(void), not
f(anything goes).

nodeFuncs.c:2051:17: warning: passing arguments to a function without
a prototype is deprecated in all versions of C and is not supported in
C2x [-Wdeprecated-non-prototype]
return walker(((WithCheckOption *)
node)->qual, context);

Discussion trail:

https://reviews.llvm.org/D123456
https://discourse.llvm.org/t/rfc-enabling-wstrict-prototypes-by-default-in-c/60521
http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2841.htm

Not sure where to see the official status of N2841 (other than waiting
for the next draft to pop out), but on random/unofficial social media
I saw that it was accepted in February, and the Clang people
apparently think it's in and I also saw a rumour that bleeding edge
GCC takes this view if you run with -std=c2x (not tested by me).

Re: Accessing git.postgresql.org fails

[Tatsuo Ishii]

> Tatsuo Ishii  writes:
>> This is ok:
>> git clone ssh://g...@gitmaster.postgresql.org/postgresql.git
> 
> That's the thing to use if you're a committer.
> 
>> But this fails:
>> $ git clone ssh://g...@git.postgresql.org/postgresql.git
> 
> Per [1], the recommended git URL for non-committers is
> 
> https://git.postgresql.org/git/postgresql.git
> 
> not ssh:.  I'm not sure that ssh: has ever worked --- wouldn't it
> require an account on the target machine?

I know. My point is, if ssh://g...@git.postgresql.org/postgresql.git
does not work for even committers, shouldn't descriptions at:
https://git.postgresql.org/gitweb/?p=postgresql.git;a=summary be
changed?

-
description This is the main PostgreSQL git repository.
owner   Magnus Hagander
last change Sat, 30 Apr 2022 16:05:32 + (09:05 -0700)
URL git://git.postgresql.org/git/postgresql.git
https://git.postgresql.org/git/postgresql.git
ssh://g...@git.postgresql.org/postgresql.git
-

I think:
ssh://g...@git.postgresql.org/postgresql.git
needs to be changed to:
ssh://g...@gitmaster.postgresql.org/postgresql.git

Best reagards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp

Re: bogus: logical replication rows/cols combinations

[Tomas Vondra]

On 4/30/22 12:11, Amit Kapila wrote:
> On Sat, Apr 30, 2022 at 3:01 PM Alvaro Herrera  
> wrote:
>>
>> On 2022-Apr-30, Amit Kapila wrote:
>>
>>> On Sat, Apr 30, 2022 at 2:02 AM Tomas Vondra
>>>  wrote:
>>
 That seems to deal with a circular replication, i.e. two logical
 replication links - a bit like a multi-master. Not sure how is that
 related to the issue we're discussing here?
>>>
>>> It is not directly related to what we are discussing here but I was
>>> trying to emphasize the point that users need to define the logical
>>> replication via pub/sub sanely otherwise they might see some weird
>>> behaviors like that.
>>
>> I agree with that.
>>
>> My proposal is that if users want to define multiple publications, and
>> their definitions conflict in a way that would behave ridiculously (==
>> bound to cause data inconsistencies eventually), an error should be
>> thrown.  Maybe we will not be able to catch all bogus cases, but we can
>> be prepared for the most obvious ones, and patch later when we find
>> others.
>>
> 
> I agree with throwing errors for obvious/known bogus cases but do we
> want to throw errors or restrict the combining of column lists when
> row filters are present in all cases? See some examples [1 ] where it
> may be valid to combine them.
> 

I think there are three challenges:

(a) Deciding what's an obvious bug or an unsupported case (e.g. because
it's not clear what's the correct behavior / way to merge column lists).

(b) When / where to detect the issue.

(c) Making sure this does not break/prevent existing use cases.


As I said before [1], I think the issue stems from essentially allowing
DML to have different row filters / column lists. So we could forbid
publications to specify WITH (publish=...) and one of the two features,
or make sure subscription does not combine multiple such publications.

The second option has the annoying consequence that it makes this
useless for the "data redaction" use case I described in [2], because
that relies on combining multiple publications.

Furthermore, what if the publications change after the subscriptions get
created? Will we be able to detect the error etc.?

So I'd prefer the first option, but maybe that prevents some useful use
cases too ...


regards


[1]
https://www.postgresql.org/message-id/45d27a8a-7c7a-88e8-a3db-c7c1d144df5e%40enterprisedb.com

[2]
https://www.postgresql.org/message-id/338e719c-4bc8-f40a-f701-e29543a264e4%40enterprisedb.com

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: bogus: logical replication rows/cols combinations

[Tomas Vondra]

On 4/29/22 07:05, Amit Kapila wrote:
> On Thu, Apr 28, 2022 at 5:56 PM Peter Eisentraut
>  wrote:
>>
>> On 27.04.22 12:33, Amit Kapila wrote:
>>> Currently, when the subscription has multiple publications, we combine
>>> the objects, and actions of those publications. It happens for
>>> 'publish_via_partition_root', publication actions, tables, column
>>> lists, or row filters. I think the whole design works on this idea
>>> even the initial table sync. I think it might need a major change
>>> (which I am not sure about at this stage) if we want to make the
>>> initial sync also behave similar to what you are proposing.
>>
>> If one publication says "publish if insert" and another publication says
>> "publish if update", then the combination of that is clearly "publish if
>> insert or update".  Similarly, if one publication says "WHERE (foo)" and
>> one says "WHERE (bar)", then the combination is "WHERE (foo OR bar)".
>>
>> But if one publication says "publish columns a and b if condition-X" and
>> another publication says "publish columns a and c if not-condition-X",
>> then the combination is clearly *not* "publish columns a, b, c if true".
>>   That is not logical, in the literal sense of that word.
>>
> 
> So, what should be the behavior in the below cases:
> 
> Case-1:
> pub1: "publish columns a and b if condition-X"
> pub2: "publish column c if condition-X"
> 
> Isn't it okay to combine these?
> 

Yes, I think it's reasonable to combine those. So the whole publication
will have

  WHERE (condition-X)

and the column list will be (a,b,c).

> Case-2:
> pub1: "publish columns a and b if condition-X"
> pub2: "publish columns c if condition-Y"
> 

In this case the publication will have

  WHERE (condition-X or condition-Y)

and there will be different column filters for different row sets:

  if (condition-X and condition-Y)
 => (a,b,c)
  else if (condition-X and NOT condition-Y)
 => (a,b)
  else if (condition-Y and NOT condition-X)
 => (c)

I think this behavior is reasonable, and it's what the patch does.

> Here Y is subset of condition X (say something like condition-X:
> "col > 5" and condition-Y: "col1 > 10").>
> What should we do in such a case?
> 
> I think if there are some cases where combining them is okay but in
> other cases, it is not okay then it is better to prohibit 'not-okay'
> cases if that is feasible.
> 

Not sure I understand what's the (supposed) issue with this example.
We'll simply do this:

  if (col1 > 5 and col1 > 10)
 => (a,b,c)
  else if (col1 > 5 and col1 <= 10)
 => (a,b)
  else if (col1 > 10 and col1 <= 5)
 => (c)

Obviously, the third branch is unreachable, because the if condition can
never be satisfied, so we can never see only column list (c). But that's
fine IMO. When phrased using the CASE expressions (as in tablesync) it's
probably somewhat less cumbersome.

I think it's easier to think about this using "data redaction" example
where you specify which columns can be replicated under what condition.
Obviously, that's "orthogonal" in the sense that we specify column list
for a row filer condition, not row filter for a column. But in principle
it's the same thing, just different grammar.

And in that case it makes perfect sense that you don't blindly combine
the column lists from all publications, because that'd defeat the whole
point of filtering columns based on row filters.

Imagine have a table with customers from different regions, and you want
to replicate the data somewhere else, but for some reason you can only
replicate details for one particular region, and subset of columns for
everyone else. So you'd do something like this:

CREATE PUBLICATION p1 FOR TABLE customers (... all columns ...)
 WHERE region = 'USA';

CREATE PUBLICATION p1 FOR TABLE customers (... subset of columns ...)
 WHERE region != 'USA';

I think ignoring the row filters and just merging the column lists makes
no sense for this use case.


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Libpq single-row mode slowness

[Tom Lane]

Daniele Varrazzo  writes:
> The operations we perform, for every row, are PQconsumeInput,
> PQisBusy, PQgetResult. Every PQconsumeInput results in a recvfrom()
> syscall, of which the first one returns the whole recordset, the
> following ones return EAGAIN. There are two extra cycles: one to get
> the TUPLES_OK result, one to get the end-of-stream NULL. It seems the
> documented usage pattern, but I'm not sure if I'm not misreading it,
> especially in the light of this libpq grumble [1].

The usual expectation is that you call PQconsumeInput to get rid of
a read-ready condition on the socket.  If you don't have a poll() or
select() or the like in the loop, you might be wasting a lot of
pointless recvfrom calls.  You definitely don't need to call
PQconsumeInput if PQisBusy is already saying that a result is available,
and in single-row mode it's likely that several results can be consumed
per recvfrom call.

regards, tom lane

Re: bogus: logical replication rows/cols combinations

[Tomas Vondra]

On 4/30/22 11:28, Alvaro Herrera wrote:
> On 2022-Apr-28, Tomas Vondra wrote:
> 
>> Attached is a patch doing the same thing in tablesync. The overall idea
>> is to generate copy statement with CASE expressions, applying filters to
>> individual columns. For Alvaro's example, this generates something like
>>
>>   SELECT
>> (CASE WHEN (a < 0) OR (a > 0) THEN a ELSE NULL END) AS a,
>> (CASE WHEN (a > 0) THEN b ELSE NULL END) AS b,
>> (CASE WHEN (a < 0) THEN c ELSE NULL END) AS c
>>   FROM uno WHERE (a < 0) OR (a > 0)
> 
> I've been reading the tablesync.c code you propose and the idea seems
> correct.  (I was distracted by wondering if a different data structure
> would be more appropriate, because what's there looks slightly
> uncomfortable to work with.  But after playing around I can't find
> anything that feels better in an obvious way.)
> 
> (I confess I'm a bit bothered by the fact that there are now three
> different data structures in our code called PublicationInfo).
> 

True. I haven't really thought about naming of the data structures, so
maybe we should name them differently.

> I propose some comment changes in the attached patch, and my
> interpretation (untested) of the idea of optimizing for a single
> publication.  (In there I also rename logicalrep_relmap_free_entry
> because it's confusing.  That should be a separate patch but I didn't
> split it before posting, apologies.)
> 
>> There's a couple options how we might optimize this for common cases.
>> For example if there's just a single publication, there's no need to
>> generate the CASE expressions - the WHERE filter will do the trick.
> 
> Right.
> 

OK, now that we agree on the approach in general, I'll look into these
optimizations (and the comments from your patch).

regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Libpq single-row mode slowness

[Daniele Varrazzo]

Hello,

a Psycopg 3 user has tested what boils down pretty much to a
"generate_series(100K)" and has reported a 100x difference between
fetching it normally and in single-row mode. I have repeated the test
myself and I have found a 50x difference (the OP doesn't specify which
platform is using, mine is Linux).

Of course calling PQconsumeInput 100K times has some overhead compared
to calling it only once. However, I would like to know if this level
of overhead is expected, or if instead anyone smells some wasted
cycles.

According to some profiling, the largest part of the time is spent
inside a libc function I don't know the symbol of, called by
pqReadData(). Details and pretty graphs are available at
https://github.com/psycopg/psycopg/issues/286

The operations we perform, for every row, are PQconsumeInput,
PQisBusy, PQgetResult. Every PQconsumeInput results in a recvfrom()
syscall, of which the first one returns the whole recordset, the
following ones return EAGAIN. There are two extra cycles: one to get
the TUPLES_OK result, one to get the end-of-stream NULL. It seems the
documented usage pattern, but I'm not sure if I'm not misreading it,
especially in the light of this libpq grumble [1].

[1] 
https://github.com/postgres/postgres/blob/master/src/interfaces/libpq/fe-misc.c#L681

Our connection is in non-blocking mode and we see the need for waiting
(using epoll here) only on the first call. The resulting strace of the
entire query process (of two rows) is:

21:36:53.659529 sendto(3, "P\0\0\0>\0select 'test' a, t b from "...,
108, MSG_NOSIGNAL, NULL, 0) = 108
21:36:53.660236 recvfrom(3, 0x1f6a870, 16384, 0, NULL, NULL) = -1
EAGAIN (Resource temporarily unavailable)
21:36:53.660589 epoll_create1(EPOLL_CLOEXEC) = 4
21:36:53.660848 epoll_ctl(4, EPOLL_CTL_ADD, 3, {EPOLLIN|EPOLLONESHOT,
{u32=3, u64=3}}) = 0
21:36:53.661099 epoll_wait(4, [{EPOLLIN, {u32=3, u64=3}}], 1023, 10) = 1
21:36:53.661941 recvfrom(3,
"1\0\0\0\0042\0\0\0\4T\0\0\0.\0\2a\0\0\0\0\0\0\0\0\0\0\31\377\377\377"...,
16384, 0, NULL, NULL) = 117
21:36:53.662506 close(4)= 0
21:36:53.662830 recvfrom(3, 0x1f6a898, 16344, 0, NULL, NULL) = -1
EAGAIN (Resource temporarily unavailable)
21:36:53.663162 recvfrom(3, 0x1f6a884, 16364, 0, NULL, NULL) = -1
EAGAIN (Resource temporarily unavailable)
21:36:53.663359 recvfrom(3, 0x1f6a876, 16378, 0, NULL, NULL) = -1
EAGAIN (Resource temporarily unavailable)

The test is on a localhost connection with sslmode disabled using libpq 14.2.

Is this the correct usage? Any insight is welcome.

Thank you very much!

-- Daniele

Re: Missing can't-assign-to-constant checks in plpgsql

[Pavel Stehule]

Hi


>> Regardless of which way we handle that point, I'm inclined to
>> change this only in HEAD.  Probably people wouldn't thank us
>> for making the back branches more strict.
>>
>
> +1
>
> I can implement these checks in plpgsql_check. So possible issues can be
> detected and fixed on older versions by using plpgsql_check.
>

new related checks are implemented on plpgsql_check 2.1.4

Regards

Pavel


>
> Regards
>
> Pavel
>
>
>> regards, tom lane
>>
>> PS: I didn't do it here, but I'm kind of tempted to pull out
>> all the cursor-related tests in plpgsql.sql and move them to
>> a new test file under src/pl/plpgsql/src/sql/.  They look
>> pretty self-contained, and I doubt they're worth keeping in
>> the core tests.
>>
>>

Re: testclient.exe installed under MSVC

[Justin Pryzby]

On Sun, May 01, 2022 at 10:23:18PM +0900, Michael Paquier wrote:
> On Sun, May 01, 2022 at 01:07:06AM -0700, Noah Misch wrote:
> > My annual audit for executables missing Windows icons turned up these:
> > 
> > pginstall/bin/testclient.exe
> > pginstall/bin/uri-regress.exe
> > 
> > I was going to add the icons, but I felt the testclient.exe name is too
> > generic-sounding to be installed.  testclient originated in commit ebc8b7d. 
> >  I
> > recommend ceasing to install both programs under MSVC.  (The GNU make build
> > system does not install them.)

See also:
a17fd67d2f2861ae0ce00d1aeefdf2facc47cd5e Build libpq test programs under MSVC.
https://www.postgresql.org/message-id/74952229-b3b0-fe47-f958-4088529a3...@dunslane.net
 MSVC build system installs extra executables
https://www.postgresql.org/message-id/e4233934-98a6-6f76-46a0-992c0f4f1...@dunslane.net
 Re: set TESTDIR from perl rather than Makefile

I'm not really sure what the plan is for the TESTDIR patches.  Is "vcregress
alltaptests" still an interesting patch to pursue, or is that going to be
obsoleted by meson build ?  

> But MSVC works differently.  vcregress.pl does a TempInstall(), which
> is a simple Install(), so isn't it going to be an issue for the tests
> if these two tools are not installed anymore?

Andrew didn't propose any mechanism for avoiding installation of the
executables, so it would break the tests.  However, at least cfbot currently
doesn't run them anyway.

One idea is if "vcregress install" accepted an option like
"vcregress install check", which would mean "install extra binaries needed for
running tests".  Something maybe not much more elegant than this.

next
  if ($insttype eq "client" && !grep { $_ eq $pf }
@client_program_files);
 
+   next if ($pf =~ /testclient|uri-regress/);

Re: Accessing git.postgresql.org fails

[Magnus Hagander]

On Sun, May 1, 2022 at 4:52 PM Tom Lane  wrote:

> Tatsuo Ishii  writes:
> > This is ok:
> > git clone ssh://g...@gitmaster.postgresql.org/postgresql.git
>
> That's the thing to use if you're a committer.
>
> > But this fails:
> > $ git clone ssh://g...@git.postgresql.org/postgresql.git
>
> Per [1], the recommended git URL for non-committers is
>
> https://git.postgresql.org/git/postgresql.git
>
> not ssh:.  I'm not sure that ssh: has ever worked --- wouldn't it
> require an account on the target machine?
>

That's correct.

ssh works if you have committer access on the repo at git.postgresql.org.
Since the main postgresql.git repo there is a mirror only, nobody has
commit access there, so it doesn't work (but there are other repos hosted
on the same server that does have committers). But for the postgresql.git
repo, it has never worked on that server.

-- 
 Magnus Hagander
 Me: https://www.hagander.net/ 
 Work: https://www.redpill-linpro.com/ 

Re: Accessing git.postgresql.org fails

[Tom Lane]

Tatsuo Ishii  writes:
> This is ok:
> git clone ssh://g...@gitmaster.postgresql.org/postgresql.git

That's the thing to use if you're a committer.

> But this fails:
> $ git clone ssh://g...@git.postgresql.org/postgresql.git

Per [1], the recommended git URL for non-committers is

https://git.postgresql.org/git/postgresql.git

not ssh:.  I'm not sure that ssh: has ever worked --- wouldn't it
require an account on the target machine?

regards, tom lane

[1] https://wiki.postgresql.org/wiki/Working_with_Git

Re: testclient.exe installed under MSVC

[Michael Paquier]

On Sun, May 01, 2022 at 01:07:06AM -0700, Noah Misch wrote:
> My annual audit for executables missing Windows icons turned up these:
> 
> pginstall/bin/testclient.exe
> pginstall/bin/uri-regress.exe
> 
> I was going to add the icons, but I felt the testclient.exe name is too
> generic-sounding to be installed.  testclient originated in commit ebc8b7d.  I
> recommend ceasing to install both programs under MSVC.  (The GNU make build
> system does not install them.)

But MSVC works differently.  vcregress.pl does a TempInstall(), which
is a simple Install(), so isn't it going to be an issue for the tests
if these two tools are not installed anymore?

> If that's unwanted for some reason, could you
> rename testclient to something like libpq_test?

Yes, the renaming makes sense.  I'd say to do more, and also rename
uri-regress, removing the hyphen from the binary name and prefix both
binaries with a "pg_".
--
Michael


signature.asc
Description: PGP signature

Re: avoid multiple hard links to same WAL file after a crash

[Michael Paquier]

On Tue, Apr 12, 2022 at 09:27:42AM -0700, Nathan Bossart wrote:
> On Tue, Apr 12, 2022 at 03:46:31PM +0900, Kyotaro Horiguchi wrote:
>> At Mon, 11 Apr 2022 09:52:57 -0700, Nathan Bossart 
>>  wrote in 
>>> I traced this back a while ago.  I believe the link() was first added in
>>> November 2000 as part of f0e37a8.  This even predates WAL recycling, which
>>> was added in July 2001 as part of 7d4d5c0.
>> 
>> f0e37a8 lacks discussion.. It introduced the CHECKPOINT command from
>> somwhere out of the ML.. This patch changed XLogFileInit to
>> supportusing existent files so that XLogWrite can use the new segment
>> provided by checkpoint and still allow XLogWrite to create a new
>> segment by itself.

Yes, I think that you are right here.  I also suspect that the
checkpoint command was facing a concurrency issue while working on
the feature and that Vadim saw that this part of the implementation
would be safer in the long run if we use link() followed by unlink().

> Yeah, I've been unable to find any discussion besides a brief reference to
> adding checkpointing [0].
> 
> [0] 
> https://postgr.es/m/8F4C99C66D04D4118F580090272A7A23018D85%40sectorbase1.sectorbase.com

While looking at the history of this area, I have also noticed this
argument, telling also that this is a safety measure if this code were
to run in parallel, but that's without counting on the control file
lock hold while doing this operation anyway:
https://www.postgresql.org/message-id/24974.982597...@sss.pgh.pa.us

As mentioned already upthread, f0e37a8 is the origin of the
link()/unlink() business in the WAL segment initialization logic, and
also note 1f159e5 that has added a rename() as extra code path for
systems where link() was not working.

At the end, switching directly from durable_rename_excl() to
durable_rename() should be fine for the WAL segment initialization,
but we could do things a bit more carefully by adding a check on the
file existence before calling durable_rename() and issue a elog(LOG)
if a file is found, giving a mean for the WAL recycling to give up
peacefully as it does now.  Per my analysis, the TLI history file
created at the end of recovery ought to issue an elog(ERROR).

Now, I am surprised by the third code path of durable_rename_excl(),
as of the WAL receiver doing writeTimeLineHistoryFile(), to not cause
any issues, as link() should exit with EEXIST when the startup process
grabs the same history file concurrently.  It seems to me that in this
last case using durable_rename() could be an improvement and prevent
extra WAL receiver restarts as a TLI history fetched from the primary
via streaming or from some archives should be the same, but we could
be more careful, like the WAL init logic, by skipping the
durable_rename() and issuing an elog(LOG).  That would not be perfect,
still a bit better than the current state of HEAD.

As we are getting closer to the beta release, it looks safer to let
this change aside a bit longer and wait for v16 to be opened for
business on HEAD.
--
Michael


signature.asc
Description: PGP signature

testclient.exe installed under MSVC

[Noah Misch]

My annual audit for executables missing Windows icons turned up these:

pginstall/bin/testclient.exe
pginstall/bin/uri-regress.exe

I was going to add the icons, but I felt the testclient.exe name is too
generic-sounding to be installed.  testclient originated in commit ebc8b7d.  I
recommend ceasing to install both programs under MSVC.  (The GNU make build
system does not install them.)  If that's unwanted for some reason, could you
rename testclient to something like libpq_test?

Thanks,
nm

Accessing git.postgresql.org fails

[Tatsuo Ishii]

This is ok:

git clone ssh://g...@gitmaster.postgresql.org/postgresql.git

But this fails:

$ git clone ssh://g...@git.postgresql.org/postgresql.git
Cloning into 'postgresql'...
Permission denied on repository for user ishii
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.


Is accessing git.postgresql.org wrong and should we access
gitmaster.postgresql.org instead?

Best reagards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp