Re: pg_upgrade and logical replication

[Michael Paquier]

On Thu, Nov 02, 2023 at 04:35:26PM +1100, Peter Smith wrote:
> The chance of being able to do so should be small as pg_upgrade uses its
> own port and unix domain directory (customizable as well with
> --socketdir), but just preventing the launcher to start is safer at the
> end, because we are then sure that no changes would ever be applied.
> ~
> "safer at the end" (??)

Well, just safer.

> 2a.
> The comment is one big long sentence. IMO it will be better to break it up.
> 2b.
> Add a blank line between this comment note and the previous one.

Yes, I found that equally confusing when looking at this patch, so
I've edited the patch this way when I was looking at it today.  This
is enough to do the job, so I have applied it for now, before moving
on with the second one of this thread.

> 2c.
> In a recent similar thread [1], they chose to implement a guc_hook to
> prevent a user from overriding this via the command line option during
> the upgrade. Shouldn't this patch do the same thing, for consistency?
> 2d.
> If you do implement such a guc_hook (per #2c above), then should the
> patch also include a test case for getting an ERROR if the user tries
> to override that GUC?

Yeah, that may be something to do, but I am not sure that it is worth
complicating the backend code for the remote case where one enforces
an option while we are already setting a GUC in the upgrade path:
https://www.postgresql.org/message-id/CAA4eK1Lh9J5VLypSQugkdD+H=_5-6p3roocjo7jbtogcxa2...@mail.gmail.com

That feels like a lot of extra facility for cases that should never
happen.
--
Michael


signature.asc
Description: PGP signature

Re: Tab completion regression test failed on illumos

[Japin Li]

On Thu, 02 Nov 2023 at 13:01, Noah Misch  wrote:
> On Wed, Nov 01, 2023 at 03:19:39PM +0800, Japin Li wrote:
>> I try to run regression test on illumos, the 010_tab_completion will
>> failed because of timeout.
>
>> Any suggestions?  Thanks in advance!
>
> This test failed for me, in a different way, when I briefly installed IO::Pty
> on a Solaris buildfarm member:
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=wrasse=2023-01-03%2022%3A39%3A26
>
Thanks for confirm this!

I try to install IO::Pty using cpan, however, I cannot get the same error.

> The IPC::Run pty tests also fail on Solaris.  If I were debugging this, I'd
> start by fixing IO::Pty and IPC::Run to pass their own test suites on Solaris
> or illumos.  Then I'd see if problems continue for this postgresql test.

So, it might be a bug comes from Perl.

-- 
Regrads,
Japin Li
ChengDu WenWu Information Technology Co., Ltd.

Re: Why is DEFAULT_FDW_TUPLE_COST so insanely low?

[Michael Paquier]

On Thu, Nov 02, 2023 at 02:32:44PM +1300, David Rowley wrote:
> No takers on the additional testing so I've pushed the patch that
> increases it to 0.2.

The CI has been telling me that the plans of the tests introduced by
this patch are not that stable when building with 32b.  See:
diff -U3 /tmp/cirrus-ci-build/contrib/postgres_fdw/expected/postgres_fdw.out 
/tmp/cirrus-ci-build/build-32/testrun/postgres_fdw/regress/results/postgres_fdw.out
--- /tmp/cirrus-ci-build/contrib/postgres_fdw/expected/postgres_fdw.out 
2023-11-02 05:25:47.290268511 +
+++ 
/tmp/cirrus-ci-build/build-32/testrun/postgres_fdw/regress/results/postgres_fdw.out
 2023-11-02 05:30:45.242316423 +
@@ -4026,13 +4026,13 @@
  Sort
Output: t1.c1, t1.c2, t1.c3, t1.c4, t1.c5, t1.c6, t1.c7, t1.c8
Sort Key: t1.c1
-   ->  Nested Loop Semi Join
+   ->  Hash Semi Join
  Output: t1.c1, t1.c2, t1.c3, t1.c4, t1.c5, t1.c6, t1.c7, t1.c8
- Join Filter: (t2.c3 = t1.c3)
+ Hash Cond: (t1.c3 = t2.c3)
  ->  Foreign Scan on public.ft1 t1
Output: t1.c1, t1.c2, t1.c3, t1.c4, t1.c5, t1.c6, t1.c7, t1.c8
Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 
1"."T 1" WHERE (("C 1" < 20))
- ->  Materialize
+ ->  Hash
Output: t2.c3
->  Foreign Scan on public.ft2 t2
  Output: t2.c3
--
Michael


signature.asc
Description: PGP signature

Re: pg_upgrade and logical replication

[Peter Smith]

Here are some review comments for patch v10-0001

==
Commit message

1.
The chance of being able to do so should be small as pg_upgrade uses its
own port and unix domain directory (customizable as well with
--socketdir), but just preventing the launcher to start is safer at the
end, because we are then sure that no changes would ever be applied.

~

"safer at the end" (??)

==
src/bin/pg_upgrade/server.c

2.
+ * We don't want the launcher to run while upgrading because it may start
+ * apply workers which could start receiving changes from the publisher
+ * before the physical files are put in place, causing corruption on the
+ * new cluster upgrading to, so setting max_logical_replication_workers=0
+ * to disable launcher.
  */
  if (GET_MAJOR_VERSION(cluster->major_version) >= 1700)
- appendPQExpBufferStr(, " -c max_slot_wal_keep_size=-1");
+ appendPQExpBufferStr(, " -c max_slot_wal_keep_size=-1 -c
max_logical_replication_workers=0");

2a.
The comment is one big long sentence. IMO it will be better to break it up.

~

2b.
Add a blank line between this comment note and the previous one.

~~~

2c.
In a recent similar thread [1], they chose to implement a guc_hook to
prevent a user from overriding this via the command line option during
the upgrade. Shouldn't this patch do the same thing, for consistency?

~~~

2d.
If you do implement such a guc_hook (per #2c above), then should the
patch also include a test case for getting an ERROR if the user tries
to override that GUC?

==
[1] 
https://www.postgresql.org/message-id/20231027.115759.2206827438943188717.horikyota.ntt%40gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia

Re: Statistics Import and Export

[Corey Huinker]

>
>
>
> Maybe I just don't understand, but I'm pretty sure ANALYZE does not
> derive index stats from column stats. It actually builds them from the
> row sample.
>

That is correct, my error.


>
> > * now support extended statistics except for MCV, which is currently
> > serialized as an difficult-to-decompose bytea field.
>
> Doesn't pg_mcv_list_items() already do all the heavy work?
>

Thanks! I'll look into that.

The comment below in mcv.c made me think there was no easy way to get
output.

/*
 * pg_mcv_list_out  - output routine for type pg_mcv_list.
 *
 * MCV lists are serialized into a bytea value, so we simply call byteaout()
 * to serialize the value into text. But it'd be nice to serialize that into
 * a meaningful representation (e.g. for inspection by people).
 *
 * XXX This should probably return something meaningful, similar to what
 * pg_dependencies_out does. Not sure how to deal with the deduplicated
 * values, though - do we want to expand that or not?
 */

Re: Tab completion regression test failed on illumos

[Noah Misch]

On Wed, Nov 01, 2023 at 03:19:39PM +0800, Japin Li wrote:
> I try to run regression test on illumos, the 010_tab_completion will
> failed because of timeout.

> Any suggestions?  Thanks in advance!

This test failed for me, in a different way, when I briefly installed IO::Pty
on a Solaris buildfarm member:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=wrasse=2023-01-03%2022%3A39%3A26

The IPC::Run pty tests also fail on Solaris.  If I were debugging this, I'd
start by fixing IO::Pty and IPC::Run to pass their own test suites on Solaris
or illumos.  Then I'd see if problems continue for this postgresql test.

Re: [PATCH] Clarify the behavior of the system when approaching XID wraparound

[Alexander Lakhin]

Hello Robert,

17.10.2023 17:39, Robert Haas wrote:

On Tue, Oct 17, 2023 at 4:57 AM Aleksander Alekseev
 wrote:

v11-0001 and v11-0002 LGTM too.

Cool. Seems we are all in agreement, so committed these. Thanks!


Please look at the following sentence added by the commit:
   ...
   to issue manual VACUUM commands on the tables where
   relminxid is oldest.

Isn't relminxid a typo there?

Best regards,
Alexander

Re: 003_check_guc.pl crashes if some extensions were loaded.

[Anton A. Melnikov]

On 02.11.2023 01:53, Michael Paquier wrote:> On Thu, Nov 02, 2023 at 12:28:05AM 
+0300, Anton A. Melnikov wrote:

Found that src/test/modules/test_misc/t/003_check_guc.pl will crash if an 
extension
that adds own GUCs was loaded into memory.
So it is now impossible to run a check-world with loaded extension libraries.


Right.  That's annoying, so let's fix it.


Thanks!

On 02.11.2023 02:29, Tom Lane wrote:

Michael Paquier  writes:

Wouldn't it be better to add a qual as of "category <> 'Customized
Options'"?


+1, seems like a cleaner answer.


Also agreed. That is a better variant!


--
Anton A. Melnikov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: Don't pass NULL pointer to strcmp().

[Nathan Bossart]

On Wed, Nov 01, 2023 at 10:39:04PM -0400, Tom Lane wrote:
> Nathan Bossart  writes:
>> What if we disallowed NULL string GUCs in v17?
> 
> Well, we'd need to devise some other solution for hacks like the
> one used by timezone_abbreviations (see comment in
> check_timezone_abbreviations).  I think it's not worth the trouble,
> especially seeing that 95% of guc.c is already set up for this.
> The bugs are mostly in newer code like get_explain_guc_options,
> and I think that's directly traceable to the lack of any comments
> or docs about this behavior.

Eh, yeah, it's probably not worth it if we find ourselves trading one set
of hacks for another.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: Atomic ops for unlogged LSN

[Nathan Bossart]

On Wed, Nov 01, 2023 at 09:15:20PM +, John Morris wrote:
> This is a rebased version . Even though I labelled it “v3”, there should be 
> no changes from “v2”.

Thanks.  I think this is almost ready, but I have to harp on the
pg_atomic_read_u64() business once more.  The relevant comment in atomics.h
has this note:

 * The read is guaranteed to return a value as it has been written by this or
 * another process at some point in the past. There's however no cache
 * coherency interaction guaranteeing the value hasn't since been written to
 * again.

However unlikely, this seems to suggest that CreateCheckPoint() could see
an old value with your patch.  Out of an abundance of caution, I'd
recommend changing this to pg_atomic_compare_exchange_u64() like
pg_atomic_read_u64_impl() does in generic.h.

@@ -4635,7 +4629,6 @@ XLOGShmemInit(void)
 
SpinLockInit(>Insert.insertpos_lck);
SpinLockInit(>info_lck);
-   SpinLockInit(>ulsn_lck);
 }

Shouldn't we do the pg_atomic_init_u64() here?  We can still set the
initial value in StartupXLOG(), but it might be safer to initialize the
variable where we are initializing the other shared memory stuff.

Since this isn't a tremendously performance-sensitive area, IMHO we should
code defensively to eliminate any doubts about correctness and to make it
easier to reason about.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: A recent message added to pg_upgade

[Peter Smith]

On Thu, Nov 2, 2023 at 2:25 PM Peter Smith  wrote:
>
> On Thu, Nov 2, 2023 at 1:58 PM Kyotaro Horiguchi
>  wrote:
> >
> > Thanks you for the comments!
> >
> > At Wed, 1 Nov 2023 18:08:19 +1100, Peter Smith  
> > wrote in
> > > Hi, here are some minor review comments for the v3 patch.
> > >
> > > ==
> > > src/backend/access/transam/xlog.c
> >
> ...
> > > 2.
> >
> > > + GUC_check_errdetail("\"max_slot_wal_keep_size\" must be set to -1
> > > during binary upgrade mode.");
> >
> > > Some of the other GUC_check_errdetail()'s do not include the GUC name
> > > in the translatable message text. Isn't that a preferred style?
> >
> > > SUGGESTION
> > > GUC_check_errdetail("\"%s\" must be set to -1 during binary upgrade 
> > > mode.",
> > >   "max_slot_wal_keep_size");
> >
> > I believe that that style was adopted to minimize translatable
> > messages by consolidting identical ones that only differ in variable
> > names.  I see both versions in the tree. I didn't find necessity to
> > adopt this approach for this specific message, especially since I'm
> > skeptical about adding new messages that end with "must be set to -1
> > during binary upgrade mode".  (pg_upgrade sets synchronous_commit,
> > fsync and full_page_writes to "off".)
> >
> > However, some unique messages are in this style, so I'm fine with
> > using that style.  Revised accordingly.
> >
>
> Checking this patch yesterday prompted me to create a new thread
> questioning the inconsistencies of the "GUC names in messages". In
> that thread, Tom Lane replied and gave some background information [1]
> about the GUC name embedding versus substitution. In hindsight, I
> think your original message was fine as-is, but there seem to be
> examples of every kind of style, so whatever you do would have some
> precedent.
>
> The patch v4 LGTM.
>

To clarify, all the current code LGTM, but the patch is still missing
a guc_hook test case, right?

==
Kind Regards,
Peter Smith.
Fujitsu Australia

Re: A recent message added to pg_upgade

[Peter Smith]

On Thu, Nov 2, 2023 at 1:58 PM Kyotaro Horiguchi
 wrote:
>
> Thanks you for the comments!
>
> At Wed, 1 Nov 2023 18:08:19 +1100, Peter Smith  wrote 
> in
> > Hi, here are some minor review comments for the v3 patch.
> >
> > ==
> > src/backend/access/transam/xlog.c
>
...
> > 2.
>
> > + GUC_check_errdetail("\"max_slot_wal_keep_size\" must be set to -1
> > during binary upgrade mode.");
>
> > Some of the other GUC_check_errdetail()'s do not include the GUC name
> > in the translatable message text. Isn't that a preferred style?
>
> > SUGGESTION
> > GUC_check_errdetail("\"%s\" must be set to -1 during binary upgrade mode.",
> >   "max_slot_wal_keep_size");
>
> I believe that that style was adopted to minimize translatable
> messages by consolidting identical ones that only differ in variable
> names.  I see both versions in the tree. I didn't find necessity to
> adopt this approach for this specific message, especially since I'm
> skeptical about adding new messages that end with "must be set to -1
> during binary upgrade mode".  (pg_upgrade sets synchronous_commit,
> fsync and full_page_writes to "off".)
>
> However, some unique messages are in this style, so I'm fine with
> using that style.  Revised accordingly.
>

Checking this patch yesterday prompted me to create a new thread
questioning the inconsistencies of the "GUC names in messages". In
that thread, Tom Lane replied and gave some background information [1]
about the GUC name embedding versus substitution. In hindsight, I
think your original message was fine as-is, but there seem to be
examples of every kind of style, so whatever you do would have some
precedent.

The patch v4 LGTM.

==
[1] https://www.postgresql.org/message-id/2758485.1698848717%40sss.pgh.pa.us

Kind Regards,
Peter Smith.
Fujitsu Australia

Re: pg_rewind WAL segments deletion pitfall

[torikoshia]

On 2023-10-31 00:26, Alexander Kukushkin wrote:

Hi,

On Wed, 18 Oct 2023 at 08:50, torikoshia 
wrote:


I have very minor questions on the regression tests mainly regarding
the
consistency with other tests for pg_rewind:


Please find attached a new version of the patch. It addresses all your
comments.


Thanks for updating the patch!


+extern void preserve_file(char *filepath);


Is this necessary?
This function was defined in older version patch, but no longer seems to 
exist.


+# We use "perl -e 'exit(1)'" as a alternative to "false", because the 
last one

'a' should be 'an'?


--
Regards,

--
Atsushi Torikoshi
NTT DATA Group Corporation

Re: race condition in pg_class

[Noah Misch]

I prototyped two ways, one with a special t_ctid and one with LockTuple().

On Fri, Oct 27, 2023 at 04:26:12PM -0700, Noah Misch wrote:
> On Fri, Oct 27, 2023 at 06:40:55PM -0400, Tom Lane wrote:
> > Noah Misch  writes:
> > > On Fri, Oct 27, 2023 at 03:32:26PM -0400, Tom Lane wrote:

> > > I anticipate a new locktag per catalog that can receive inplace updates,
> > > i.e. LOCKTAG_RELATION_DEFINITION and LOCKTAG_DATABASE_DEFINITION.
> > 
> > We could perhaps make this work by using the existing tuple-lock
> > infrastructure, rather than inventing new locktags (a choice that
> > spills to a lot of places including clients that examine pg_locks).
> 
> That could be okay.  It would be weird to reuse a short-term lock like that
> one as something held till end of transaction.  But the alternative of new
> locktags ain't perfect, as you say.

That worked.

> > I would prefer though to find a solution that only depends on making
> > heap_inplace_update protect itself, without high-level cooperation
> > from the possibly-interfering updater.  This is basically because
> > I'm still afraid that we're defining the problem too narrowly.
> > For one thing, I have nearly zero confidence that GRANT et al are
> > the only problematic source of conflicting transactional updates.
> 
> Likewise here, but I have fair confidence that an assertion would flush out
> the rest.  heap_inplace_update() would assert that the backend holds one of
> the acceptable locks.  It could even be an elog; heap_inplace_update() can
> tolerate that cost.

That check would fall in both heap_inplace_update() and heap_update().  After
all, a heap_inplace_update() check won't detect an omission in GRANT.

> > For another, I'm worried that some extension may be using
> > heap_inplace_update against a catalog we're not considering here.
> 
> A pgxn search finds "citus" using heap_inplace_update().
> 
> > I'd also like to find a solution that fixes the case of a conflicting
> > manual UPDATE (although certainly that's a stretch goal we may not be
> > able to reach).
> 
> It would be nice.

I expect most approaches could get there by having ExecModifyTable() arrange
for the expected locking or other actions.  That's analogous to how
heap_update() takes care of sinval even for a manual UPDATE.

> > I wonder if there's a way for heap_inplace_update to mark the tuple
> > header as just-updated in a way that regular heap_update could
> > recognize.  (For standard catalog updates, we'd then end up erroring
> > in simple_heap_update, which I think is fine.)  We can't update xmin,
> > because the VACUUM callers don't have an XID; but maybe there's some
> > other way?  I'm speculating about putting a funny value into xmax,
> > or something like that, and having heap_update check that what it
> > sees in xmax matches what was in the tuple the update started with.
> 
> Hmmm.  Achieving it without an XID would be the real trick.  (With an XID, we
> could use xl_heap_lock like heap_update() does.)  Thinking out loud, what if
> heap_inplace_update() sets HEAP_XMAX_INVALID and xmax =
> TransactionIdAdvance(xmax)?  Or change t_ctid in a similar way.  Then regular
> heap_update() could complain if the field changed vs. last seen value.  This
> feels like something to regret later in terms of limiting our ability to
> harness those fields for more-valuable ends or compact them away in a future
> page format.  I can't pinpoint a specific loss, so the idea might have legs.
> Nontransactional data in separate tables or in new metapages smells like the
> right long-term state.  A project wanting to reuse the tuple header bits could
> introduce such storage to unblock its own bit reuse.

heap_update() does not have the pre-modification xmax today, so I used t_ctid.
heap_modify_tuple() preserves t_ctid, so heap_update() already has the
pre-modification t_ctid in key cases.  For details of how the prototype uses
t_ctid, see comment at "#define InplaceCanaryOffsetNumber".  The prototype
doesn't prevent corruption in the following scenario, because the aborted
ALTER TABLE RENAME overwrites the special t_ctid:

  == session 1
  drop table t;
  create table t (c int);
  begin;
  -- in gdb, set breakpoint on heap_modify_tuple
  grant select on t to public;

  == session 2
  alter table t add primary key (c);
  begin; alter table t rename to t2; rollback;

  == back in session 1
  -- release breakpoint
  -- want error (would get it w/o the begin;alter;rollback)
  commit;

I'm missing how to mark the tuple in a fashion accessible to a second
heap_update() after a rolled-back heap_update().  The mark needs enough bits
"N" so it's implausible for 2^N inplace updates to happen between GRANT
fetching the old tuple and GRANT completing heap_update().  Looking for bits
that could persist across a rolled-back heap_update(), we have 3 in t_ctid, 2
in t_infomask2, and 0 in xmax.  I definitely don't want to paint us into a
corner by spending the t_infomask2 bits on this.  Even if I did, 

Re: A recent message added to pg_upgade

[Kyotaro Horiguchi]

Thanks you for the comments!

At Wed, 1 Nov 2023 18:08:19 +1100, Peter Smith  wrote in 
> Hi, here are some minor review comments for the v3 patch.
> 
> ==
> src/backend/access/transam/xlog.c

> I asked ChatGPT to suggest alternative wording for that comment, and
> it came up with something that I felt was a slight improvement.
> 
> SUGGESTION
> ...
> If WALs needed by logical replication slots are deleted, these slots
> become inoperable. During a binary upgrade, pg_upgrade sets this
> variable to -1 via the command line in an attempt to prevent such
> deletions, but users have ways to override it. To ensure the
> successful completion of the upgrade, it's essential to keep this
> variable unaltered.
> ...
> 
> ~~~

ChatGPT seems to tend to generate sentences in a slightly different
from our usual writing. While I tried to retain the original phrasing
in the patch, I don't mind using the suggested version.  Used as is.

> 2.

> + GUC_check_errdetail("\"max_slot_wal_keep_size\" must be set to -1
> during binary upgrade mode.");

> Some of the other GUC_check_errdetail()'s do not include the GUC name
> in the translatable message text. Isn't that a preferred style?

> SUGGESTION
> GUC_check_errdetail("\"%s\" must be set to -1 during binary upgrade mode.",
>   "max_slot_wal_keep_size");

I believe that that style was adopted to minimize translatable
messages by consolidting identical ones that only differ in variable
names.  I see both versions in the tree. I didn't find necessity to
adopt this approach for this specific message, especially since I'm
skeptical about adding new messages that end with "must be set to -1
during binary upgrade mode".  (pg_upgrade sets synchronous_commit,
fsync and full_page_writes to "off".)

However, some unique messages are in this style, so I'm fine with
using that style.  Revised accordingly.

> ==
> src/backend/replication/slot.c
> 
> 3. InvalidatePossiblyObsoleteSlot

> + Assert (!*invalidated || !SlotIsLogical(s) || !IsBinaryUpgrade);
> 
> IMO new Assert became trickier to understand than the original condition. 
> YMMV.
> 
> SUGGESTION
> Assert(!(*invalidated && SlotIsLogical(s) && IsBinaryUpgrade));

Yeah, I also liked that style and considered using it, but I didn't
feel it was too hard to read in this particular case, so I ended up
using the current way.  Just like with the point of other comments,
I'm not particularly attached to this style. Thus if someone find it
difficult to read, I have no issue with changing it. Revised as
suggested.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/access/transam/xlog.c 
b/src/backend/access/transam/xlog.c
index b541be8eec..46833f6ecd 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -2063,6 +2063,29 @@ check_wal_segment_size(int *newval, void **extra, 
GucSource source)
return true;
 }
 
+/*
+ * GUC check_hook for max_slot_wal_keep_size
+ *
+ * If WALs needed by logical replication slots are deleted, these slots become
+ * inoperable. During a binary upgrade, pg_upgrade sets this variable to -1 via
+ * the command line in an attempt to prevent such deletions, but users have
+ * ways to override it. To ensure the successful completion of the upgrade,
+ * it's essential to keep this variable unaltered.  See
+ * InvalidatePossiblyObsoleteSlot() and start_postmaster() in pg_upgrade for
+ * more details.
+ */
+bool
+check_max_slot_wal_keep_size(int *newval, void **extra, GucSource source)
+{
+   if (IsBinaryUpgrade && *newval != -1)
+   {
+   GUC_check_errdetail("\"%s\" must be set to -1 during binary 
upgrade mode.",
+   "max_slot_wal_keep_size");
+   return false;
+   }
+   return true;
+}
+
 /*
  * At a checkpoint, how many WAL segments to recycle as preallocated future
  * XLOG segments? Returns the highest segment that should be preallocated.
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index 99823df3c7..5c3d2b1082 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -1424,18 +1424,12 @@ 
InvalidatePossiblyObsoleteSlot(ReplicationSlotInvalidationCause cause,
SpinLockRelease(>mutex);
 
/*
-* The logical replication slots shouldn't be invalidated as
-* max_slot_wal_keep_size GUC is set to -1 during the upgrade.
-*
-* The following is just a sanity check.
+* check_max_slot_wal_keep_size() ensures 
max_slot_wal_keep_size is set
+* to -1, so, slot invalidation for logical slots shouldn't 
happen
+* during an upgrade. At present, only logical slots really 
require
+* this.
 */
-   if (*invalidated && SlotIsLogical(s) && IsBinaryUpgrade)
-   {
-   ereport(ERROR,
-   

Re: Don't pass NULL pointer to strcmp().

[Tom Lane]

Nathan Bossart  writes:
> On Wed, Nov 01, 2023 at 09:57:18PM -0400, Tom Lane wrote:
>> After digging around for a bit, I think part of the problem is a lack
>> of a clearly defined spec for what should happen with NULL string GUCs.

> What if we disallowed NULL string GUCs in v17?

Well, we'd need to devise some other solution for hacks like the
one used by timezone_abbreviations (see comment in
check_timezone_abbreviations).  I think it's not worth the trouble,
especially seeing that 95% of guc.c is already set up for this.
The bugs are mostly in newer code like get_explain_guc_options,
and I think that's directly traceable to the lack of any comments
or docs about this behavior.

regards, tom lane

Re: Don't pass NULL pointer to strcmp().

[Nathan Bossart]

On Wed, Nov 01, 2023 at 09:57:18PM -0400, Tom Lane wrote:
> I wrote:
>> Hmm ... if we're doing it ourselves, I suppose we've got to consider
>> it supported :-(.  But I'm still wondering how many seldom-used
>> code paths didn't get the message.  An example here is that this
>> could lead to GetConfigOptionResetString returning NULL, which
>> I think is outside its admittedly-vague API spec.
> 
> After digging around for a bit, I think part of the problem is a lack
> of a clearly defined spec for what should happen with NULL string GUCs.
> In the attached v3, I attempted to remedy that by adding a comment in
> guc_tables.h (which is maybe not the best place but I didn't see a
> better one).  That led me to a couple more changes beyond what you had.

What if we disallowed NULL string GUCs in v17?  That'd simplify the spec
and future-proof against similar bugs, but it might also break a fair
number of extensions.  If there aren't any other reasons to continue
supporting it, maybe it's the right long-term approach, though.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: Why is DEFAULT_FDW_TUPLE_COST so insanely low?

[Bruce Momjian]

On Thu, Nov  2, 2023 at 02:32:44PM +1300, David Rowley wrote:
> On Tue, 31 Oct 2023 at 11:16, David Rowley  wrote:
> > I'd be happy if anyone else would like to try the same experiment to
> > see if there's some other value of DEFAULT_FDW_TUPLE_COST that might
> > suit better.
> 
> No takers on the additional testing so I've pushed the patch that
> increases it to 0.2.

Great!  Thanks.

-- 
  Bruce Momjian  https://momjian.us
  EDB  https://enterprisedb.com

  Only you can decide what is important to you.

Re: Don't pass NULL pointer to strcmp().

[Xing Guo]

Thank you Tom!

Your comment
"NULL doesn't have semantics that are visibly different from an empty
string" is exactly what I want to confirm :-)

On 11/2/23, Tom Lane  wrote:
> I wrote:
>> Hmm ... if we're doing it ourselves, I suppose we've got to consider
>> it supported :-(.  But I'm still wondering how many seldom-used
>> code paths didn't get the message.  An example here is that this
>> could lead to GetConfigOptionResetString returning NULL, which
>> I think is outside its admittedly-vague API spec.
>
> After digging around for a bit, I think part of the problem is a lack
> of a clearly defined spec for what should happen with NULL string GUCs.
> In the attached v3, I attempted to remedy that by adding a comment in
> guc_tables.h (which is maybe not the best place but I didn't see a
> better one).  That led me to a couple more changes beyond what you had.
>
> It's possible that some of these are unreachable --- for example,
> given that a NULL could only be the default value, I'm not sure that
> the fix in write_one_nondefault_variable is a live bug.  But we ought
> to code all this stuff defensively, and most of it already was
> NULL-safe.
>
>   regards, tom lane
>
>


-- 
Best Regards,
Xing

Re: Requiring recovery.signal or standby.signal when recovering with a backup_label

[Kyotaro Horiguchi]

At Wed, 1 Nov 2023 08:39:17 +0900, Michael Paquier  wrote 
in 
> See in StartupXLOG(), around the comment "complain if we did not roll
> forward far enough to reach".  This complains if archive recovery has
> been requested *or* if we retrieved a backup end LSN from the
> backup_label.

Please note that backupStartPoint is not reset even when reaching the
backup end point during crash recovery. If backup_label enforces
archive recovery, I think this point won't be an issue as you
mentioned. For the record, my earlier proposal aimed to detect
reaching the end point even during crash recovery.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: Don't pass NULL pointer to strcmp().

[Tom Lane]

I wrote:
> Hmm ... if we're doing it ourselves, I suppose we've got to consider
> it supported :-(.  But I'm still wondering how many seldom-used
> code paths didn't get the message.  An example here is that this
> could lead to GetConfigOptionResetString returning NULL, which
> I think is outside its admittedly-vague API spec.

After digging around for a bit, I think part of the problem is a lack
of a clearly defined spec for what should happen with NULL string GUCs.
In the attached v3, I attempted to remedy that by adding a comment in
guc_tables.h (which is maybe not the best place but I didn't see a
better one).  That led me to a couple more changes beyond what you had.

It's possible that some of these are unreachable --- for example,
given that a NULL could only be the default value, I'm not sure that
the fix in write_one_nondefault_variable is a live bug.  But we ought
to code all this stuff defensively, and most of it already was
NULL-safe.

regards, tom lane

diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 39d3775e80..ccffaaad02 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -1473,7 +1473,9 @@ check_GUC_init(struct config_generic *gconf)
 			{
 struct config_string *conf = (struct config_string *) gconf;
 
-if (*conf->variable != NULL && strcmp(*conf->variable, conf->boot_val) != 0)
+if (*conf->variable != NULL &&
+	(conf->boot_val == NULL ||
+	 strcmp(*conf->variable, conf->boot_val) != 0))
 {
 	elog(LOG, "GUC (PGC_STRING) %s, boot_val=%s, C-var=%s",
 		 conf->gen.name, conf->boot_val ? conf->boot_val : "", *conf->variable);
@@ -4213,8 +4215,7 @@ SetConfigOption(const char *name, const char *value,
 /*
  * Fetch the current value of the option `name', as a string.
  *
- * If the option doesn't exist, return NULL if missing_ok is true (NOTE that
- * this cannot be distinguished from a string variable with a NULL value!),
+ * If the option doesn't exist, return NULL if missing_ok is true,
  * otherwise throw an ereport and don't return.
  *
  * If restrict_privileged is true, we also enforce that only superusers and
@@ -4257,7 +4258,8 @@ GetConfigOption(const char *name, bool missing_ok, bool restrict_privileged)
 			return buffer;
 
 		case PGC_STRING:
-			return *((struct config_string *) record)->variable;
+			return *((struct config_string *) record)->variable ?
+*((struct config_string *) record)->variable : "";
 
 		case PGC_ENUM:
 			return config_enum_lookup_by_value((struct config_enum *) record,
@@ -4304,7 +4306,8 @@ GetConfigOptionResetString(const char *name)
 			return buffer;
 
 		case PGC_STRING:
-			return ((struct config_string *) record)->reset_val;
+			return ((struct config_string *) record)->reset_val ?
+((struct config_string *) record)->reset_val : "";
 
 		case PGC_ENUM:
 			return config_enum_lookup_by_value((struct config_enum *) record,
@@ -5255,7 +5258,14 @@ get_explain_guc_options(int *num)
 {
 	struct config_string *lconf = (struct config_string *) conf;
 
-	modified = (strcmp(lconf->boot_val, *(lconf->variable)) != 0);
+	if (lconf->boot_val == NULL &&
+		*lconf->variable == NULL)
+		modified = false;
+	else if (lconf->boot_val == NULL ||
+			 *lconf->variable == NULL)
+		modified = true;
+	else
+		modified = (strcmp(lconf->boot_val, *(lconf->variable)) != 0);
 }
 break;
 
@@ -5482,7 +5492,8 @@ write_one_nondefault_variable(FILE *fp, struct config_generic *gconf)
 			{
 struct config_string *conf = (struct config_string *) gconf;
 
-fprintf(fp, "%s", *conf->variable);
+if (*conf->variable)
+	fprintf(fp, "%s", *conf->variable);
 			}
 			break;
 
@@ -6142,7 +6153,8 @@ RestoreGUCState(void *gucstate)
 {
 	struct config_string *conf = (struct config_string *) gconf;
 
-	guc_free(*conf->variable);
+	if (*conf->variable)
+		guc_free(*conf->variable);
 	if (conf->reset_val && conf->reset_val != *conf->variable)
 		guc_free(conf->reset_val);
 	if (conf->reset_extra && conf->reset_extra != gconf->extra)
diff --git a/src/include/utils/guc_tables.h b/src/include/utils/guc_tables.h
index 1ec9575570..0c38255961 100644
--- a/src/include/utils/guc_tables.h
+++ b/src/include/utils/guc_tables.h
@@ -240,6 +240,16 @@ struct config_real
 	void	   *reset_extra;
 };
 
+/*
+ * A note about string GUCs: the boot_val is allowed to be NULL, which leads
+ * to the reset_val and the actual variable value (*variable) also being NULL.
+ * However, there is no way to set a NULL value subsequently using
+ * set_config_option or any other GUC API.  Also, GUC APIs such as SHOW will
+ * display a NULL value as an empty string.  Callers that choose to use a NULL
+ * boot_val should overwrite the setting later in startup, or else be careful
+ * that NULL doesn't have semantics that are visibly different from an empty
+ * string.
+ */
 struct config_string
 {
 	struct 

Re: GUC names in messages

[Nathan Bossart]

On Wed, Nov 01, 2023 at 09:46:52PM +0100, Laurenz Albe wrote:
> I agree for names with underscores in them.  But I think that quoting
> is necessary for names like "timezone" or "datestyle" that might be
> mistaken for normal words.  My personal preference is to always quote
> GUC names, but I think it is OK not to quote GOCs whose name are
> clearly not natural language words.

+1, IMHO quoting GUC names makes it abundantly clear that they are special
identifiers.  In de4d456, we quoted the role names in a bunch of messages.
We didn't quote the attribute/option names, but those are in all-caps, so
they already stand out nicely.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: add log messages when replication slots become active and inactive (was Re: Is it worth adding ReplicationSlot active_pid to ReplicationSlotPersistentData?)

[Peter Smith]

On Wed, Nov 1, 2023 at 2:03 PM Amit Kapila  wrote:
>
> On Mon, Mar 27, 2023 at 11:08 AM Bharath Rupireddy
>  wrote:
> >
> > The v9 patch was failing because I was using MyReplicationSlot after
> > it got reset by slot release, attached v10 patch fixes it.
> >
>
> + *
> + * Note: use LogReplicationSlotAcquire() if needed, to log a message after
> + * acquiring the replication slot.
>   */
>  void
>  ReplicationSlotAcquire(const char *name, bool nowait)
> @@ -542,6 +554,9 @@ retry:
>
> When does it need to be logged? For example, recently, we added one
> more slot acquisition/release call in
> binary_upgrade_logical_slot_has_caught_up(); it is not clear from the
> comments whether we need to LOG it or not. I guess at some place like
> atop LogReplicationSlotAcquire() we should document in a bit more
> specific way as to when is this expected to be called.
>

I agree. Just saying "if needed" in those function comments doesn't
help with knowing how to judge when logging is needed or not.

~

Looking back at the thread history it seems the function comment was
added after Euler [1] suggested ("... you should add a comment at the
top of ReplicationSlotAcquire() and ReplicationSlotRelease() functions
saying that LogReplicationSlotAquired() and
LogReplicationSlotReleased() functions should be called respectively
after it.")

But that's not quite compatible with what Alvaro [2] had written long
back ("... the only acquisitions that would log messages are those in
StartReplication and StartLogicalReplication.").

In other words, ReplicationSlotAcquire/ReplicationSlotRelease is
called by more places than you care to log from.

~

Adding a better explanatory comment than "if needed" will be good, and
maybe that is all that is necessary. I'm not sure.

OTOH, if you have to explain that logging is only wanted for a couple
of scenarios, then it raises some doubts about the usefulness of
having a common function in the first place. I had the same doubts
back in March [3] ("I am not sure for the *current* code if the
encapsulation is worth the trouble or not.").

==
[1] Euler - 
https://www.postgresql.org/message-id/c42d5634-ca9b-49a7-85cd-9eff9feb33b4%40app.fastmail.com
[2] Alvaro - 
https://www.postgresql.org/message-id/202204291032.qfvyuqxkjnjw%40alvherre.pgsql
[3] Peter - 
https://www.postgresql.org/message-id/CAHut%2BPu6Knwooc_NckMxszGrAJnytgpMadtoJ-OA-SFWT%2BGFYw%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Austalia

Re: Why is DEFAULT_FDW_TUPLE_COST so insanely low?

[David Rowley]

On Tue, 31 Oct 2023 at 11:16, David Rowley  wrote:
> I'd be happy if anyone else would like to try the same experiment to
> see if there's some other value of DEFAULT_FDW_TUPLE_COST that might
> suit better.

No takers on the additional testing so I've pushed the patch that
increases it to 0.2.

David

Re: Don't pass NULL pointer to strcmp().

[Tom Lane]

Xing Guo  writes:
> There're extensions that set their boot_val to NULL. E.g., postgres_fdw

Hmm ... if we're doing it ourselves, I suppose we've got to consider
it supported :-(.  But I'm still wondering how many seldom-used
code paths didn't get the message.  An example here is that this
could lead to GetConfigOptionResetString returning NULL, which
I think is outside its admittedly-vague API spec.

regards, tom lane

Re: document deviation from standard on REVOKE ROLE

[Bruce Momjian]

On Wed, Nov  1, 2023 at 07:49:25PM -0400, Tom Lane wrote:
> Bruce Momjian  writes:
> > On Fri, Oct 30, 2020 at 02:03:48PM -0400, John Naylor wrote:
> >> +In the SQL standard, REVOKE only revokes the 
> >> privilege
> >> +as granted by the invoking role. In 
> >> PostgreSQL,
> >> +this will also revoke privileges granted by other roles.
> 
> > John, should this 2020 patch still be applied?
> 
> [ raised eyebrow... ]  I do not think that was ever true as written,
> and it's demonstrably not true now.
..
> Maybe there's some related point that needs to be made,
> but not that one.

Cool, thanks, closed!

-- 
  Bruce Momjian  https://momjian.us
  EDB  https://enterprisedb.com

  Only you can decide what is important to you.

Re: document deviation from standard on REVOKE ROLE

[Tom Lane]

Bruce Momjian  writes:
> On Fri, Oct 30, 2020 at 02:03:48PM -0400, John Naylor wrote:
>> +In the SQL standard, REVOKE only revokes the 
>> privilege
>> +as granted by the invoking role. In 
>> PostgreSQL,
>> +this will also revoke privileges granted by other roles.

> John, should this 2020 patch still be applied?

[ raised eyebrow... ]  I do not think that was ever true as written,
and it's demonstrably not true now.

regression=# create user alice;
CREATE ROLE
regression=# create user bob;
CREATE ROLE
regression=# create table subject (id int);
CREATE TABLE
regression=# grant select on table subject to alice with grant option;
GRANT
regression=# grant select on table subject to bob with grant option;
GRANT
regression=# \c - alice
You are now connected to database "regression" as user "alice".
regression=> grant select on table subject to public;
GRANT
regression=> \c - bob
You are now connected to database "regression" as user "bob".
regression=> grant select on table subject to public;
GRANT
regression=> \dp subject
  Access privileges
 Schema |  Name   | Type  | Access privileges | Column privileges | 
Policies 
+-+---+---+---+--
 public | subject | table | postgres=arwdDxt/postgres+|   | 
| |   | alice=r*/postgres+|   | 
| |   | bob=r*/postgres  +|   | 
| |   | =r/alice +|   | 
| |   | =r/bob|   | 
(1 row)

regression=> revoke select on table subject from public;
REVOKE
regression=> \dp subject
  Access privileges
 Schema |  Name   | Type  | Access privileges | Column privileges | 
Policies 
+-+---+---+---+--
 public | subject | table | postgres=arwdDxt/postgres+|   | 
| |   | alice=r*/postgres+|   | 
| |   | bob=r*/postgres  +|   | 
| |   | =r/alice  |   | 
(1 row)

Maybe there's some related point that needs to be made,
but not that one.

regards, tom lane

Re: Don't pass NULL pointer to strcmp().

[Xing Guo]

Hi Tom,

There're extensions that set their boot_val to NULL. E.g., postgres_fdw (
https://github.com/postgres/postgres/blob/4210b55f598534db9d52c4535b7dcc777dda75a6/contrib/postgres_fdw/option.c#L582),
plperl (
https://github.com/postgres/postgres/blob/4210b55f598534db9d52c4535b7dcc777dda75a6/src/pl/plperl/plperl.c#L422C13-L422C13,
https://github.com/postgres/postgres/blob/4210b55f598534db9d52c4535b7dcc777dda75a6/src/pl/plperl/plperl.c#L444C12-L444C12,
https://github.com/postgres/postgres/blob/4210b55f598534db9d52c4535b7dcc777dda75a6/src/pl/plperl/plperl.c#L452C6-L452C6)
(Can we treat plperl as an extension?), pltcl (
https://github.com/postgres/postgres/blob/4210b55f598534db9d52c4535b7dcc777dda75a6/src/pl/tcl/pltcl.c#L465C14-L465C14,
https://github.com/postgres/postgres/blob/4210b55f598534db9d52c4535b7dcc777dda75a6/src/pl/tcl/pltcl.c#L472C12-L472C12
).

TBH, I don't know if NULL is a valid boot_val for string variables, I just
came across some extensions that use NULL as their boot_val. If the
boot_val can't be NULL in extensions, we should probably add some
assertions or comments about it?

Best Regards,
Xing








On Wed, Nov 1, 2023 at 11:30 PM Tom Lane  wrote:

> Xing Guo  writes:
> > Thanks for your comments. I have updated the patch accordingly.
>
> I'm leery of accepting this patch, as I see no reason that we
> should consider it valid for an extension to have a string GUC
> with a boot_val of NULL.
>
> I realize that we have a few core GUCs that are like that, but
> I'm pretty sure that every one of them has special-case code
> that initializes the GUC to something non-null a bit later on
> in startup.  I don't think there are any cases where a string
> GUC's persistent value will be null, and I don't like the
> idea of considering that to be an allowed case.  It would
> open the door to more crash situations, and it brings up the
> old question of how could a user tell NULL from empty string
> (via SHOW or current_setting() or whatever).  Besides, what's
> the benefit really?
>
> regards, tom lane
>

Properly pathify the union planner

[David Rowley]

The upper planner was pathified many years ago now.  That was a large
chunk of work and because of that, the union planner was not properly
pathified in that effort.  A small note was left in
recurse_set_operations() to mention about future work.

You can see this lack of pathification in make_union_unique() and
choose_hashed_setop(). There are heuristics in there to decide the
method to use instead of creating paths and letting add_path() decide
what's faster.

I've been working on improving this for the past few weeks and I'm not
quite as far along as I'd hoped, but what I have is perhaps worthy of
sharing.  For now, I've only improved UNIONs.

A UNION plan can now look like:

# explain (costs off) select * from a union select * from a;
QUERY PLAN
---
 Unique
   ->  Merge Append
 Sort Key: a.a
 ->  Index Only Scan using a_pkey on a
 ->  Index Only Scan using a_pkey on a a_1

Previously we'd have only considered Append -> Hash Aggregate or via
Append -> Sort -> Unique

To make this work, the query_planner() needs to know about setops, so
I've passed those down via the standard_qp_extra struct so that we can
choose pathkeys for the setops.

One part that still needs work is the EquivalanceClass building.
Because we only build the final targetlist for the Append after
planning all the append child queries, I ended up having to populate
the EquivalanceClasses backwards, i.e children first. add_eq_member()
determines if you're passing a child member by checking if parent !=
NULL.  Since I don't have a parent EquivalenceMember to pass,
em_is_child gets set wrongly, and that causes problems because
ec_has_const can get set to true when it shouldn't. This is a problem
as it can make a PathKey redundant when it isn't.  I wonder if I'll
need to change the signature of add_eq_member() and add an "is_child"
bool to force the EM to be a child em... Needs more thought...

I've not worked on the creation of Incremental Sort paths yet, or done
any path plumbing work to have UNION consider Gather Merge -> Unique
on already sorted paths.  I think to make similar improvements to
EXCEPT and INTERSECT we'd need a node executor node. Perhaps
nodeMergeAppendSetops.c which can be configured to do EXCEPT or
INTERSECT.  It could also perhaps handle UNION too then we can use
that instead of a Merge Append -> Unique.  That might save doing some
slot copying and improve performance further. I'm not planning on
doing that for the first stage. I only intend to improve UNION for
that and we have all the executor nodes to make that work already.

Anyway, I've attached my WIP patch for this.

David
diff --git a/src/backend/optimizer/path/equivclass.c 
b/src/backend/optimizer/path/equivclass.c
index 7fa502d6e2..54ddcf8285 100644
--- a/src/backend/optimizer/path/equivclass.c
+++ b/src/backend/optimizer/path/equivclass.c
@@ -2599,6 +2599,44 @@ find_derived_clause_for_ec_member(EquivalenceClass *ec,
return NULL;
 }
 
+/*
+ * add_union_rel_equivalences
+ */
+void
+add_union_rel_equivalences(PlannerInfo *root, Relids relids,
+  List *tlist, List 
*union_pathkeys)
+{
+   ListCell   *lc;
+   ListCell   *lc2 = list_head(union_pathkeys);
+
+   foreach(lc, tlist)
+   {
+   TargetEntry *tle = lfirst_node(TargetEntry, lc);
+   PathKey*pk;
+
+   if (tle->resjunk)
+   continue;
+
+   if (lc2 == NULL)
+   elog(ERROR, "wrong number of union pathkeys");
+
+   pk = lfirst_node(PathKey, lc2);
+
+   /*
+* XXX this needs fixed.  The children are added before the 
parents,
+* so we cannot identify the parent.
+*/
+   add_eq_member(pk->pk_eclass,
+ tle->expr,
+ relids,
+ NULL,
+ NULL,
+ exprType((Node *) tle->expr));
+   pk->pk_eclass->ec_has_const = false;/* XXX hack hack */
+
+   lc2 = lnext(union_pathkeys, lc2);
+   }
+}
 
 /*
  * add_child_rel_equivalences
diff --git a/src/backend/optimizer/path/pathkeys.c 
b/src/backend/optimizer/path/pathkeys.c
index fdb60aaa8d..00f2071cf0 100644
--- a/src/backend/optimizer/path/pathkeys.c
+++ b/src/backend/optimizer/path/pathkeys.c
@@ -786,6 +786,59 @@ build_partition_pathkeys(PlannerInfo *root, RelOptInfo 
*partrel,
return retval;
 }
 
+/*
+ * build_union_child_pathkeys
+ */
+List *
+build_union_child_pathkeys(PlannerInfo *root, SetOperationStmt *op,
+  RelOptInfo *rel, List *tlist)
+{
+   ListCell   *lc;
+   ListCell   *sgccell = list_head(op->groupClauses);
+   List   *retval = NIL;
+

Re: document deviation from standard on REVOKE ROLE

[Bruce Momjian]

On Fri, Oct 30, 2020 at 02:03:48PM -0400, John Naylor wrote:
> This is the other doc fix as suggested in 
> https://www.postgresql.org/message-id/20201027220555.GS4951%40momjian.us
> 
> There is already a compatibility section, so put there.
> -- 
> John Naylor
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company 

> diff --git a/doc/src/sgml/ref/revoke.sgml b/doc/src/sgml/ref/revoke.sgml
> index 35ff87a4f5..c8d5be92db 100644
> --- a/doc/src/sgml/ref/revoke.sgml
> +++ b/doc/src/sgml/ref/revoke.sgml
> @@ -298,6 +298,12 @@ REVOKE admins FROM joe;
>  is required according to the standard, but 
> PostgreSQL
>  assumes RESTRICT by default.
> 
> +
> +   
> +In the SQL standard, REVOKE only revokes the privilege
> +as granted by the invoking role. In 
> PostgreSQL,
> +this will also revoke privileges granted by other roles.
> +   
>   
>  
>   

John, should this 2020 patch still be applied?


-- 
  Bruce Momjian  https://momjian.us
  EDB  https://enterprisedb.com

  Only you can decide what is important to you.

Re: 003_check_guc.pl crashes if some extensions were loaded.

[Michael Paquier]

On Wed, Nov 01, 2023 at 07:29:51PM -0400, Tom Lane wrote:
> Actually we do force that, see valid_custom_variable_name().
> But I think your idea is better.

Ah, indeed, thanks.  I didn't recall this was the case.
--
Michael


signature.asc
Description: PGP signature

Re: 003_check_guc.pl crashes if some extensions were loaded.

[Tom Lane]

Michael Paquier  writes:
> On Thu, Nov 02, 2023 at 12:28:05AM +0300, Anton A. Melnikov wrote:
>> "SELECT name
>> FROM pg_settings
>> WHERE NOT 'NOT_IN_SAMPLE' = ANY (pg_settings_get_flags(name)) AND
>> -   name <> 'config_file'
>> +   name <> 'config_file' AND name NOT LIKE '%.%'
>> ORDER BY 1");

> Wouldn't it be better to add a qual as of "category <> 'Customized
> Options'"?

+1, seems like a cleaner answer.

> That's something arbitrarily assigned for all custom GUCs
> and we are sure that none of them will exist in
> postgresql.conf.sample.  There's also no guarantee that out-of-core
> custom GUCs will include a dot in their name (even if I know that
> maintainers close to the community adopt this convention and are
> rather careful about that).

Actually we do force that, see valid_custom_variable_name().
But I think your idea is better.

regards, tom lane

Re: Commitfest manager November 2023

[Michael Paquier]

On Wed, Nov 01, 2023 at 01:33:26PM +0700, John Naylor wrote:
> I didn't see any recent mentions in the archives, so I'll volunteer to
> be CF manager for 2023-11.

Thanks, John!
--
Michael


signature.asc
Description: PGP signature

Re: 003_check_guc.pl crashes if some extensions were loaded.

[Michael Paquier]

On Thu, Nov 02, 2023 at 12:28:05AM +0300, Anton A. Melnikov wrote:
> Found that src/test/modules/test_misc/t/003_check_guc.pl will crash if an 
> extension
> that adds own GUCs was loaded into memory.
> So it is now impossible to run a check-world with loaded extension libraries.

Right.  That's annoying, so let's fix it.

> --- a/src/test/modules/test_misc/t/003_check_guc.pl
> +++ b/src/test/modules/test_misc/t/003_check_guc.pl
> @@ -19,7 +19,7 @@ my $all_params = $node->safe_psql(
> "SELECT name
>   FROM pg_settings
> WHERE NOT 'NOT_IN_SAMPLE' = ANY (pg_settings_get_flags(name)) AND
> -   name <> 'config_file'
> +   name <> 'config_file' AND name NOT LIKE '%.%'
>   ORDER BY 1");

Wouldn't it be better to add a qual as of "category <> 'Customized
Options'"?  That's something arbitrarily assigned for all custom GUCs
and we are sure that none of them will exist in
postgresql.conf.sample.  There's also no guarantee that out-of-core
custom GUCs will include a dot in their name (even if I know that
maintainers close to the community adopt this convention and are
rather careful about that).
--
Michael


signature.asc
Description: PGP signature

Re: Guiding principle for dropping LLVM versions?

[Thomas Munro]

So it sounds like we're in agreement that it is time to require LLVM
10+ in master.  Could the owners (CC'd) of the following animals
please turn off --with-llvm on master (and future 17+ branches), or
consider upgrading to a modern OS release?  Otherwise they'll turn
red.

   animal|arch| llvm_version |   os   | os_release
| end_of_support
-++--+++
 mantid  | x86_64 | 5.0.1| CentOS | 7
| 2019-08-06
 cotinga | s390x  | 6.0.0| Ubuntu | 18.04
| 2023-06-01
 vimba   | aarch64| 6.0.0| Ubuntu | 18.04
| 2023-06-01
 topminnow   | mips64el; -mabi=32 | 6.0.1| Debian | 8
| 2018-06-17
 alimoche| aarch64| 7.0.1| Debian | 10
| 2022-09-10
 blackneck   | aarch64| 7.0.1| Debian | 10
| 2022-09-10

And of course Andres would need to do the same for his coverage
animals in that range:

   animal|arch| llvm_version |   os   | os_release
| end_of_support
-++--+++
 dragonet| x86_64 | 3.9.1| Debian | sid|
 phycodurus  | x86_64 | 3.9.1| Debian | sid|
 desmoxytes  | x86_64 | 4.0.1| Debian | sid|
 petalura| x86_64 | 4.0.1| Debian | sid|
 idiacanthus | x86_64 | 5.0.2| Debian | sid|
 pogona  | x86_64 | 5.0.2| Debian | sid|
 komodoensis | x86_64 | 6.0.1| Debian | sid|
 xenodermus  | x86_64 | 6.0.1| Debian | sid|

Re: GUC names in messages

[Peter Smith]

On Thu, Nov 2, 2023 at 1:25 AM Tom Lane  wrote:
>
> Daniel Gustafsson  writes:
> > On 1 Nov 2023, at 10:22, Peter Smith  wrote:
> >> One idea to achieve consistency might be to always substitute GUC
> >> names using a macro.
> >>
> >> #define GUC_NAME(s) ("\"" s "\"")
> >>
> >> ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> >> errmsg("%s must be at least twice %s",
> >> GUC_NAME("max_wal_size"),
> >> GUC_NAME("wal_segment_size";
>
> > Something like this might make translations harder since the remaining 
> > string
> > leaves little context about the message.  We already have that today to some
> > extent (so it might not be an issue), and it might be doable to 
> > automatically
> > add translator comments, but it's something to consider.
>
> Our error message style guidelines say not to assemble messages out
> of separate parts, because it makes translation difficult.  Originally
> we applied that rule to GUC names mentioned in messages as well.
> Awhile ago the translation team decided that that made for too many
> duplicative translations, so they'd be willing to compromise on
> substituting GUC names.  That's only been changed in a haphazard
> fashion though, mostly in cases where there actually were duplicative
> messages that could be merged this way.  And there's never been any
> real clarity about whether to quote GUC names, though certainly we're
> more likely to quote anything injected with %s.  So that's why we have
> a mishmash right now.
>
> I'm not enamored of the GUC_NAME idea suggested above.  I don't
> think it buys anything, and what it does do is make *every single
> one* of our GUC-mentioning messages wrong.  I think if we want to
> standardize here, we should standardize on something that's
> already pretty common in the code base.
>

Thanks to everybody for the feedback received so far.

Perhaps as a first step, I can try to quantify the GUC name styles
already in the source code. The numbers might help decide how to
proceed

==
Kind Regards,
Peter Smith.
Fujitsu Australia

003_check_guc.pl crashes if some extensions were loaded.

[Anton A. Melnikov]

Hello!

Found that src/test/modules/test_misc/t/003_check_guc.pl will crash if an 
extension
that adds own GUCs was loaded into memory.
So it is now impossible to run a check-world with loaded extension libraries.

Reproduction:
cd src/test/modules/test_misc
export EXTRA_INSTALL="contrib/pg_stat_statements"
export TEMP_CONFIG=$(pwd)/pg_stat_statements_temp.conf
echo -e "shared_preload_libraries = 'pg_stat_statements'" > $TEMP_CONFIG
echo "compute_query_id = 'regress'" >> $TEMP_CONFIG
make check PROVE_TESTS='t/003_check_guc.pl'

# +++ tap check in src/test/modules/test_misc +++
t/003_check_guc.pl .. 1/?
#   Failed test 'no parameters missing from postgresql.conf.sample'
#   at t/003_check_guc.pl line 81.
#  got: '5'
# expected: '0'
# Looks like you failed 1 test of 3.

Maybe exclude such GUCs from this test?
For instance, like that:

--- a/src/test/modules/test_misc/t/003_check_guc.pl
+++ b/src/test/modules/test_misc/t/003_check_guc.pl
@@ -19,7 +19,7 @@ my $all_params = $node->safe_psql(
"SELECT name
  FROM pg_settings
WHERE NOT 'NOT_IN_SAMPLE' = ANY (pg_settings_get_flags(name)) AND
-   name <> 'config_file'
+   name <> 'config_file' AND name NOT LIKE '%.%'
  ORDER BY 1");

With the best wishes,

--
Anton A. Melnikov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Companycommit 91c84d50ba4b1c75749e4c160e1d4a25ca684fda
Author: Anton A. Melnikov 
Date:   Wed Nov 1 23:58:19 2023 +0300

Exclude extensions' GUCs from the 003_check_guc.pl

diff --git a/src/test/modules/test_misc/t/003_check_guc.pl b/src/test/modules/test_misc/t/003_check_guc.pl
index 4fd6d03b9e..63fcd754de 100644
--- a/src/test/modules/test_misc/t/003_check_guc.pl
+++ b/src/test/modules/test_misc/t/003_check_guc.pl
@@ -19,7 +19,7 @@ my $all_params = $node->safe_psql(
 	"SELECT name
  FROM pg_settings
WHERE NOT 'NOT_IN_SAMPLE' = ANY (pg_settings_get_flags(name)) AND
-   name <> 'config_file'
+   name <> 'config_file' AND name NOT LIKE '%.%'
  ORDER BY 1");
 # Note the lower-case conversion, for consistency.
 my @all_params_array = split("\n", lc($all_params));

Re: Atomic ops for unlogged LSN

[John Morris]

Here is what I meant to do earlier. As it turns out, this patch has not been 
merged yet.

This is a rebased version . Even though I labelled it “v3”, there should be no 
changes from “v2”.


atomicLSN-v3-Use-atomic-ops-for-unloggedLSNs.patch
Description: atomicLSN-v3-Use-atomic-ops-for-unloggedLSNs.patch

Re: generate syscache info automatically

[Peter Eisentraut]

Here is a rebased patch set, along with a summary of the questions I 
have about these patches:



v4-0001-Generate-syscache-info-from-catalog-files.patch

What's a good syntax to declare a syscache?  Currently, I have, for example

-DECLARE_UNIQUE_INDEX_PKEY(pg_type_oid_index, 2703, TypeOidIndexId, 
pg_type, btree(oid oid_ops));
+DECLARE_UNIQUE_INDEX_PKEY(pg_type_oid_index, 2703, TypeOidIndexId, 
pg_type, btree(oid oid_ops), TYPEOID, 64);


I suppose a sensible alternative could be to leave the
DECLARE_..._INDEX... alone and make a separate statement, like

MAKE_SYSCACHE(pg_type_oid_index, TYPEOID, 64);

That's at least visually easier, because some of those
DECLARE_... lines are getting pretty long.

I would like to keep those MAKE_SYSCACHE lines in the catalog files.
That just makes it easier to reference everything together.


v4-0002-Generate-ObjectProperty-from-catalog-files.patch

Several questions here:

* What's a good way to declare the mapping between catalog and object
  type?

* How to select which catalogs have ObjectProperty entries generated?

* Where to declare class descriptions?  Or just keep the current hack in
  the patch.

* How to declare the purpose of a catalog column, like "this is the ACL
  column for this catalog".  This is currently done by name, but maybe
  it should be more explicit.

* Question about how to pick the correct value for is_nsp_name_unique?

This second patch is clearly still WIP.  I hope the first patch can be 
finished soon, however.



I was also amused to find the original commit fa351d5a0db that 
introduced ObjectProperty, which contains the following comment:


Performance isn't critical here, so there's no need to be smart
about how we do the search.

which I'm now trying to amend.


On 11.09.23 07:02, John Naylor wrote:


On Thu, Aug 31, 2023 at 6:23 PM Peter Eisentraut > wrote:


 > Attached is an updated patch set.

 > There was some discussion about whether the catalog files are the right
 > place to keep syscache information.  Personally, I would find that more
 > convenient.  (Note that we also recently moved the definitions of
 > indexes and toast tables from files with the whole list into the various
 > catalog files.)  But we can also keep it somewhere else.  The important
 > thing is that Catalog.pm can find it somewhere in a structured form.

I don't have anything further to say on how to fit it together, but I'll 
go ahead share some other comments from a quick read of v3-0003:


+ # XXX hardcoded exceptions
+ # extension doesn't belong to extnamespace
+ $attnum_namespace = undef if $catname eq 'pg_extension';
+ # pg_database owner is spelled datdba
+ $attnum_owner = "Anum_pg_database_datdba" if $catname eq 'pg_database';

These exceptions seem like true exceptions...

+ # XXX?
+ $name_syscache = "SUBSCRIPTIONNAME" if $catname eq 'pg_subscription';
+ # XXX?
+ $is_nsp_name_unique = 1 if $catname eq 'pg_collation';
+ $is_nsp_name_unique = 1 if $catname eq 'pg_opclass';
+ $is_nsp_name_unique = 1 if $catname eq 'pg_opfamily';
+ $is_nsp_name_unique = 1 if $catname eq 'pg_subscription';

... but as the XXX conveys, these indicate a failure to do the right 
thing. Trying to derive this info from the index declarations is 
currently fragile. E.g. making one small change to this regex:


-               if ($index->{index_decl} =~ /\(\w+name name_ops(, 
\w+namespace oid_ops)?\)/)
+               if ($index->{index_decl} =~ /\w+name name_ops(, 
\w+namespace oid_ops)?\)/)


...causes "is_nsp_name_unique" to flip from false to true, and/or sets 
"name_catcache_id" where it wasn't before, for several entries. It's not 
quite clear what the "rule" is intended to be, or whether it's robust 
going forward.


That being the case, perhaps some effort should also be made to make it 
easy to verify the output matches HEAD (or rather, v3-0001). This is now 
difficult because of the C99 ".foo = bar" syntax, plus the alphabetical 
ordering.


+foreach my $catname (@catnames)
+{
+ print $objectproperty_info_fh qq{#include "catalog/${catname}_d.h"\n};
+}

Assuming we have a better way to know which catalogs need 
object properties, is there a todo to only #include those?


 > To finish up the ObjectProperty generation, we also need to store some
 > more data, namely the OBJECT_* mappings.  We probably do not want to
 > store those in the catalog headers; that looks like a layering violation
 > to me.

Possibly, but it's also convenient and, one could argue, more 
straightforward than storing syscache id and num_buckets in the index info.


One last thing: I did try to make the hash function use uint16 for the 
key (to reduce loop iterations on nul bytes), and that didn't help, so 
we are left with the ideas I mentioned earlier.


(not changing CF status, because nothing specific is really required to 
change at the moment, some things up in the air)


--
John Naylor
EDB: http://www.enterprisedb.com 

Re: GUC names in messages

[Laurenz Albe]

On Wed, 2023-11-01 at 16:12 -0400, Peter Eisentraut wrote:
> On 01.11.23 10:25, Tom Lane wrote:
> > And there's never been any
> > real clarity about whether to quote GUC names, though certainly we're
> > more likely to quote anything injected with %s.  So that's why we have
> > a mishmash right now.
> 
> I'm leaning toward not quoting GUC names.  The quoting is needed in 
> places where the value can be arbitrary, to avoid potential confusion. 
> But the GUC names are well-known, and we wouldn't add confusing GUC 
> names like "table" or "not found" in the future.

I agree for names with underscores in them.  But I think that quoting
is necessary for names like "timezone" or "datestyle" that might be
mistaken for normal words.  My personal preference is to always quote
GUC names, but I think it is OK not to quote GOCs whose name are
clearly not natural language words.

Yours,
Laurenz Albe

Re: Remove distprep

[Peter Eisentraut]

On 09.10.23 17:14, Andres Freund wrote:

It kinda works, but I'm not sure how well.  Because the aliasing happens in
Makefile.global, we won't know about the "original" maintainer-clean target
once recursing into a subdir.

That's perhaps OK, because extensions likely won't utilize subdirectories? But
I'm not sure. I know that some people build postgres extensions by adding them
to contrib/, in those cases it won't work.

OTOH, it seems somewhat unlikely that maintainer-clean is utilized much in
extensions. I see it in things like postgis, but that has it's own configure
etc, even though it also invokes pgxs.


I thought about this.  I don't think this is something that any 
extension would use.  If they care about the distinction between 
distclean and maintainer-clean, are they also doing their own distprep 
and dist?  Seems unlikely.  I mean, if some extension is actually 
affected, I'm happy to accommodate, but we can deal with that when we 
learn about it.  Moreover, if we are moving forward in this direction, 
we would presumably also like the extensions to get rid of their 
distprep step.


So I think we are ready to move ahead with this patch.  There have been 
some light complaints earlier in this thread that people wanted to keep 
some way to clean only some of the files.  But there hasn't been any 
concrete follow-up on that, as far as I can see, so I don't know what to 
do about that.

Re: GUC names in messages

[Peter Eisentraut]

On 01.11.23 10:25, Tom Lane wrote:

And there's never been any
real clarity about whether to quote GUC names, though certainly we're
more likely to quote anything injected with %s.  So that's why we have
a mishmash right now.


I'm leaning toward not quoting GUC names.  The quoting is needed in 
places where the value can be arbitrary, to avoid potential confusion. 
But the GUC names are well-known, and we wouldn't add confusing GUC 
names like "table" or "not found" in the future.

Re: Not deleted mentions of the cleared path

[a.rybakina]

Hi! Thank you for the interest to this issue.

On 31.10.2023 06:25, Richard Guo wrote:


On Mon, Oct 30, 2023 at 7:31 PM Alena Rybakina  
wrote:

 I have already written about the problem of InvalidPath [0]
 appearing. I investigated this and found an error in the add_path()
 function, when we reject a path, we free up the memory of the path,
 but do not delete various mentions of it (for example, in the
 ancestor of relation, as in the example below).

I agree that what you observed is true - add_path() may free a path
while it's still referenced from some lower rels.  For instance, when
creating ordered paths, we may use the input path unchanged without
copying if it's already well ordered, and it might be freed afterwards
if it fails when competing in add_path().
But this doesn't seem to be a problem in practice.  We will not access
these references from the lower rels.
I'm not sure if this is an issue that we need to fix, or we need to live
with.  But I do think it deserves some explanation in the comment of
add_path().


I agree that the code looks like an error, but without a real request, 
it is still difficult to identify it as a bug. I'll try to reproduce it. 
And yes, at least a comment is required here, and to be honest, I have 
already faced this problem myself.


On 30.10.2023 17:36, Ashutosh Bapat wrote:

On Mon, Oct 30, 2023 at 5:01 PM Alena Rybakina
  wrote:

Hi, hackers!

I have already written about the problem of InvalidPath [0] appearing. I 
investigated this and found an error in the add_path() function, when we reject 
a path, we free up the memory of the path, but do not delete various mentions 
of it (for example, in the ancestor of relation, as in the example below).

Thus, we may face the problem of accessing the freed memory.

I demonstrated this below using gdb when I call a query after running a test in 
test/regression/sql/create_misc.sql:

Query:

:-- That ALTER TABLE should have added TOAST tables.

SELECT relname, reltoastrelid <> 0 AS has_toast_table
FROM pg_class
WHERE oid::regclass IN ('a_star', 'c_star')
ORDER BY 1;

--UPDATE b_star*
--   SET a = text 'gazpacho'
--   WHERE aa > 4;

SELECT class, aa, a FROM a_star*;


gdb:

0x7ff3f7325fda in epoll_wait (epfd=5, events=0x55bf9ee75c38, maxevents=1, 
timeout=-1) at ../sysdeps/unix/sysv/linux/epoll_wait.c:30
30  ../sysdeps/unix/sysv/linux/epoll_wait.c: No such file or directory.
(gdb) b /home/alena/postgrespro_3/src/backend/optimizer/util/pathnode.c:620
Breakpoint 1 at 0x55bf9cfe4c65: file pathnode.c, line 621.
(gdb) c
Continuing.

Breakpoint 1, add_path (parent_rel=0x55bf9ef7f5c0, new_path=0x55bf9ef7f4e0) at 
pathnode.c:621
621 if (!IsA(new_path, IndexPath))
(gdb) n
622   pfree(new_path);
(gdb) n
624 }
(gdb) p *new_path
$1 = {type = T_Invalid, pathtype = T_Invalid, parent = 0x7f7f7f7f7f7f7f7f, 
pathtarget = 0x7f7f7f7f7f7f7f7f,
   param_info = 0x7f7f7f7f7f7f7f7f, parallel_aware = 127, parallel_safe = 127, 
parallel_workers = 2139062143,
   rows = 1.3824172084878715e+306, startup_cost = 1.3824172084878715e+306, 
total_cost = 1.3824172084878715e+306,
   pathkeys = 0x7f7f7f7f7f7f7f7f}
(gdb) p new_path
$2 = (Path *) 0x55bf9ef7f4e0

At this point the new_path has not been added to the parent_rel. We do
not set the cheapest* paths while paths are being added. The stack
trace will give you an idea where this is happening.

(gdb) p ((ProjectionPath *)((SortPath*)parent_rel->pathlist->elements 
[0].ptr_value)->subpath)->path->parent->cheapest_startup_path
$20 = (struct Path *) 0x55bf9ef7f4e0

This looks familiar though. There was some nearby thread where Tom
Lane, if my memory serves well, provided a case where a path from
lower rel was added to an upper rel without copying or changing its
parent. This very much looks like that case.


Thank you, I think this might help me to find a query to reproduce it.

Re: Statistics Import and Export

[Tomas Vondra]

On 10/31/23 08:25, Corey Huinker wrote:
>
> Attached is v2 of this patch.
> 
> New features:
> * imports index statistics. This is not strictly accurate: it 
> re-computes index statistics the same as ANALYZE does, which is to
> say it derives those stats entirely from table column stats, which
> are imported, so in that sense we're getting index stats without
> touching the heap.

Maybe I just don't understand, but I'm pretty sure ANALYZE does not
derive index stats from column stats. It actually builds them from the
row sample.

> * now support extended statistics except for MCV, which is currently 
> serialized as an difficult-to-decompose bytea field.

Doesn't pg_mcv_list_items() already do all the heavy work?


regards

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

Re: pg_upgrade and logical replication

[vignesh C]

On Fri, 27 Oct 2023 at 17:05, Amit Kapila  wrote:
>
> On Fri, Oct 27, 2023 at 12:09 PM vignesh C  wrote:
> >
> > Apart from this I'm still checking that the old cluster's subscription
> > relations states are READY state still, but there is a possibility
> > that SYNCDONE or FINISHEDCOPY could work, this needs more thought
> > before concluding which is the correct state to check. Let' handle
> > this in the upcoming version.
> >
>
> I was analyzing this part and it seems it could be tricky to upgrade
> in FINISHEDCOPY state. Because the system would expect that subscriber
> would know the old slotname from oldcluster which it can drop at
> SYNCDONE state. Now, as sync_slot_name is generated based on subid,
> relid which could be different in the new cluster, the generated
> slotname would be different after the upgrade. OTOH, if the relstate
> is INIT, then I think the sync could be performed even after the
> upgrade.

I had analyzed all the subscription relation states further, here is
my analysis:
The following states are ok, as either the replication slot is not
created or the replication slot is already dropped and the required
WAL files will be present in the publisher:
a) SUBREL_STATE_SYNCDONE b) SUBREL_STATE_READY c) SUBREL_STATE_INIT
The following states are not ok as the worker has dependency on the
replication slot/origin in these case:
a) SUBREL_STATE_DATASYNC: In this case, the table sync worker will try
to drop the replication slot but as the replication slots will be
created with old subscription id in the publisher and the upgraded
subscriber will not be able to clean the slots in this case. b)
SUBREL_STATE_FINISHEDCOPY: In this case, the tablesync worker will
expect the origin to be already existing as the origin is created with
an old subscription id, tablesync worker will not be able to find the
origin in this case. c) SUBREL_STATE_SYNCWAIT, SUBREL_STATE_CATCHUP
and SUBREL_STATE_UNKNOWN: These states are not stored in the catalog,
so we need not allow these states.
I modified it to support the relation states accordingly.

> Shouldn't we at least ensure that replication origins do exist in the
> old cluster corresponding to each of the subscriptions? Otherwise,
> later the query to get remote_lsn for origin in getSubscriptions()
> would fail.
Added a check for the same.

The attached v10 version patch has the changes for the same.

Regards,
Vignesh
From 779fc68636da8c901670574c7284726e0a0c58ac Mon Sep 17 00:00:00 2001
From: Vignesh C 
Date: Fri, 27 Oct 2023 11:18:28 +0530
Subject: [PATCH v10 1/2] Prevent startup of logical replication launcher in
 binary upgrade mode

The logical replication launcher may start apply workers during an
upgrade, which could be the cause of corruptions on a new cluster if
these are able to apply changes before the physical files are copied
over.

The chance of being able to do so should be small as pg_upgrade uses its
own port and unix domain directory (customizable as well with
--socketdir), but just preventing the launcher to start is safer at the
end, because we are then sure that no changes would ever be applied.

Author: Vignesh C
Discussion: https://postgr.es/m/CALDaNm2g9ZKf=y8X6z6MsLCuh8WwU-=q6plj35nfi2m5bzn...@mail.gmail.com
---
 src/bin/pg_upgrade/server.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/bin/pg_upgrade/server.c b/src/bin/pg_upgrade/server.c
index d7f6c268ef..9dedf63a87 100644
--- a/src/bin/pg_upgrade/server.c
+++ b/src/bin/pg_upgrade/server.c
@@ -248,9 +248,14 @@ start_postmaster(ClusterInfo *cluster, bool report_and_exit_on_error)
 	 * invalidation of slots during the upgrade. We set this option when
 	 * cluster is PG17 or later because logical replication slots can only be
 	 * migrated since then. Besides, max_slot_wal_keep_size is added in PG13.
+	 * We don't want the launcher to run while upgrading because it may start
+	 * apply workers which could start receiving changes from the publisher
+	 * before the physical files are put in place, causing corruption on the
+	 * new cluster upgrading to, so setting max_logical_replication_workers=0
+	 * to disable launcher.
 	 */
 	if (GET_MAJOR_VERSION(cluster->major_version) >= 1700)
-		appendPQExpBufferStr(, " -c max_slot_wal_keep_size=-1");
+		appendPQExpBufferStr(, " -c max_slot_wal_keep_size=-1 -c max_logical_replication_workers=0");
 
 	/* Use -b to disable autovacuum. */
 	snprintf(cmd, sizeof(cmd),
-- 
2.34.1

From 7e9440e47d1d0844ffd6647d4e341a07f033de86 Mon Sep 17 00:00:00 2001
From: Vignesh C 
Date: Mon, 30 Oct 2023 12:31:59 +0530
Subject: [PATCH v10 2/2] Preserve the full subscription's state during
 pg_upgrade

Previously, only the subscription metadata information was preserved.  Without
the list of relations and their state it's impossible to re-enable the
subscriptions without missing some records as the list of relations can only be
refreshed after enabling the subscription (and therefore starting the apply
worker).  Even if we added a 

Re: Confused about stream replication protocol documentation

[Bruce Momjian]

Patch applied to master, thanks.

---

On Thu, Dec 24, 2020 at 02:28:53AM +, Li Japin wrote:
> 
> On Dec 23, 2020, at 8:11 PM, Fujii Masao 
> wrote:
> 
> 
> On 2020/12/23 11:08, Li Japin wrote:
> 
> On Dec 22, 2020, at 11:13 PM, Fujii Masao <
> masao.fu...@oss.nttdata.com >
> wrote:
> 
> ‘B’ means a backend and ‘F’ means a frontend. Maybe as [1] does, 
> we
> should
> add the note like "Each is marked to indicate that it can be sent
> by
> a frontend (F) and a backend (B)" into the description about each
> message
> format for START_REPLICATION.
> 
> [1]
> 
> https://www.postgresql.org/docs/devel/protocol-message-formats.html
>   protocol-message-formats.html>
> 
> Thanks for your clarify.  Maybe we should move the "protocol message
> formats”
> before “stream replication protocol” or referenced it in "stream
> replication protocol”.
> 
> 
> I like the latter. And maybe it's better to reference to also
> "53.6. Message Data Types" there because the messages for
> START_REPLICATION use the message data types.
> 
> 
> Add reference about “protocol message types” and “protocol message formats”.
> 
> index 4899bacda7..5793936b42 100644
> --- a/doc/src/sgml/protocol.sgml
> +++ b/doc/src/sgml/protocol.sgml
> @@ -2069,8 +2069,9 @@ The commands accepted in replication mode are:
>   
> 
>   
> -  WAL data is sent as a series of CopyData messages.  (This allows
> -  other information to be intermixed; in particular the server can send
> +  WAL data is sent as a series of CopyData messages
> +  (See  and  "protocol-message-formats"/>).
> +  (This allows other information to be intermixed; in particular the
> server can send
>an ErrorResponse message if it encounters a failure after beginning
>to stream.)  The payload of each CopyData message from server to the
>client contains a message of one of the following formats:
> 
> --
> Best regards
> Japin Li
> 

> diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml
> index 4899bacda7..5793936b42 100644
> --- a/doc/src/sgml/protocol.sgml
> +++ b/doc/src/sgml/protocol.sgml
> @@ -2069,8 +2069,9 @@ The commands accepted in replication mode are:
>   
>  
>   
> -  WAL data is sent as a series of CopyData messages.  (This allows
> -  other information to be intermixed; in particular the server can send
> +  WAL data is sent as a series of CopyData messages
> +  (See  and  linkend="protocol-message-formats"/>).
> +  (This allows other information to be intermixed; in particular the 
> server can send
>an ErrorResponse message if it encounters a failure after beginning
>to stream.)  The payload of each CopyData message from server to the
>client contains a message of one of the following formats:


-- 
  Bruce Momjian  https://momjian.us
  EDB  https://enterprisedb.com

  Only you can decide what is important to you.

Re: MERGE ... RETURNING

[Jeff Davis]

On Wed, 2023-11-01 at 10:12 +, Dean Rasheed wrote:
> Something I'm wondering about is to what extent this discussion is
> driven by concerns about aspects of the implementation (specifically,
> references to function OIDs in code), versus a desire for a different
> user-visible syntax. To a large extent, those are orthogonal
> questions.

Most of my concern is that parts of the implementation feel like a
hack, which makes me concerned that we're approaching it the wrong way.

At a language level, I'm also concerned that we don't have a way to
access the before/after versions of the tuple. I won't insist on this
because I'm hoping that could be solved as part of a later patch that
also addresses UPDATE ... RETURNING.

> (As an aside, I would note that there are already around a dozen
> references to specific function OIDs in the parse analysis code, and
> a
> lot more if you grep more widely across the whole of the backend
> code.)

If you can point to a precedent, then I'm much more inclined to be OK
with the implementation.

Regards,
Jeff Davis

Re: MERGE ... RETURNING

[Merlin Moncure]

On Wed, Nov 1, 2023 at 5:12 AM Dean Rasheed 
wrote:

> On Tue, 31 Oct 2023 at 23:19, Vik Fearing  wrote:
> >
> > On 10/31/23 19:28, Jeff Davis wrote:
> >
> > > Assuming we have one RETURNING clause at the end, then it creates the
> > > problem of how to communicate which WHEN clause a tuple came from,
> > > whether it's the old or the new version, and/or which action was
> > > performed on that tuple.
> > >
> > > How do we communicate any of those things? We need to get that
> > > information into the result table somehow, so it should probably be
> > > some kind of expression that can exist in the RETURNING clause. But
> > > what kind of expression?
> > >
> > > (a) It could be a totally new expression kind with a new keyword (or
> > > recycling some existing keywords for the same effect, or something that
> > > looks superficially like a function call but isn't) that's only valid
> > > in the RETURNING clause of a MERGE statement. If you use it in another
> > > expression (say the targetlist of a SELECT statement), then you'd get a
> > > failure at parse analysis time.
> >
> > This would be my choice, the same as how the standard GROUPING()
> > "function" for grouping sets is implemented by GroupingFunc.
> >
>
> Something I'm wondering about is to what extent this discussion is
> driven by concerns about aspects of the implementation (specifically,
> references to function OIDs in code), versus a desire for a different
> user-visible syntax. To a large extent, those are orthogonal
> questions.
>
> (As an aside, I would note that there are already around a dozen
> references to specific function OIDs in the parse analysis code, and a
> lot more if you grep more widely across the whole of the backend
> code.)
>
> At one point, as I was writing this patch, I went part-way down the
> route of adding a new node type (I think I called it MergeFunc), for
> these merge support functions, somewhat inspired by GroupingFunc. In
> the end, I backed out of that approach, because it seemed to be
> introducing a lot of unnecessary additional complexity, and I decided
> that a regular FuncExpr would suffice.
>
> If pg_merge_action() and pg_merge_when_clause_number() were
> implemented using a MergeFunc node, it would reduce the number of
> places that refer to specific function OIDs. Basically, a MergeFunc
> node would be very much like a FuncExpr node, except that it would
> have a "levels up" field, set during parse analysis, at the point
> where we check that it is being used in a merge returning clause, and
> this field would be used during subselect planning. Note, however,
> that that doesn't entirely eliminate references to specific function
> OIDs -- the parse analysis code would still do that. Also, additional
> special-case code in the executor would be required to handle
> MergeFunc nodes. Also, code like IncrementVarSublevelsUp() would need
> adjusting, and anything else like that.
>
> A separate question is what the syntax should be. We could invent a
> new syntax, like GROUPING(). Perhaps:
>
>   MERGE(ACTION) instead of pg_merge_action()
>   MERGE(CLAUSE NUMBER) instead of pg_merge_when_clause_number()
>

Hm, still struggling with this merge action and (especially) number stuff.
Currently we have:

 WHEN MATCHED [ AND *condition* ] THEN { *merge_update* |
*merge_delete* | DO NOTHING } |
  WHEN NOT MATCHED [ AND *condition* ] THEN { *merge_insert* | DO NOTHING } }

What about extending to something like:

WHEN MATCHED [ AND *condition* ] [ AS *merge_clause_name ]*

WHEN MATCHED AND tid > 2 AS giraffes THEN UPDATE SET balance = t.balance +
delta

...and have pg_merge_clause() return 'giraffes' (of name type).  If merge
clause is not identified, maybe don't return any data for that clause
through returning,, or return NULL.  Maybe 'returning' clause doesn't have
to be extended or molested in any way, it would follow mechanics as per
'update', and could not refer to identified merge_clauses, but would allow
for pg_merge_clause() functioning.  You wouldn't need to identify action or
number.  Food for thought, -- may have missed some finer details upthread.

for example,
with r as (
  merge into x using y on x.a = y.a
  when matched and x.c > 0 as good then do nothing
  when matched and x.c <= 0 as bad then do nothing
  returning pg_merge_clause(), x.*
) ...

yielding
pg_merge_clause a  c
good1  5
good2  7
bad 3  0
...

...maybe allow pg_merge_clause()  take to optionally yield column name:
  returning pg_merge_clause('result'), x.*
) ...

yielding
result a  c
good   1  5
good   2  7
bad3  0
...

merlin

max_standby_streaming_delay setting not cancelling query on replica

[Ben Snaidero]

Hi,

We are running a postgres streaming replica with
max_standby_streaming_delay set to 900 seconds (15min).  We encountered an
issue in our environment where we had a long running query that was running
against the replica for just over 4 hours causing 4 hours of replication
lag.  Looking at pg_stat_activity for this query it was stuck in
Client:ClientWrite wait state for pretty much all of this time (it ran for
less than 1 minute before going into ClientWrite wait state.  We capture
pg_stat_activity every minute and only first capture shows a DataFileRead
wait and there was only 1 other capture during the 4 hours where it was
active with no wait event).  From what we could tell the client process
tried to send cancellation and disconnected (our client application uses
npgsql) so there was no process to consume these results and after manually
cancelling the query the replication lag came back down so this query was
definitely the cause of the lag.

Question: Why did the max_standby_streaming_delay setting not cancel this
query?

I looked at the code in standby.c and if there are conflicting locks it
should be cancelled.  Unfortunately at the time this issue occurred we
weren't collecting pg_locks to see what locks are being held but given the
state the query was in would it have released the ACCESS SHARE lock it
acquired while executing the query given it just has to send data to the
client now?  I would think if it still held this lock then the query would
be cancelled.  If it didn't hold it anymore then maybe that is why
max_standby_streaming_delay setting didn't cause it to be cancelled.  Any
other ideas?

Thanks,
Ben

Re: "box" type description

[Bruce Momjian]

On Wed, Mar 31, 2021 at 01:43:47PM +0200, Christoph Berg wrote:
> Re: Kyotaro Horiguchi
> > Returing to the description of pg_types, it should be changed like
> > this following the discussion here.
> > 
> > - pg_catalog | box  | geometric box '(lower left,upper right)'
> > + pg_catalog | box  | geometric box 'lower left,upper right'
> > 
> > But I find it hard to read. I fixed it instead as the following in the
> > attached. However, it might rather be better not changing it..
> > 
> > + pg_catalog | box  | geometric box 'pt-lower-left,pt-upper-right'
> 
> I like that because it points to the "point" syntax so users can
> figure out how to spell a box.

I liked Horiguchi-san's patch from 2021, but once I started looking
further, I found a number of improvements that can be made in the \dTS
output beyond Horiguchi-san's changes:

*  boolean outputs 't'/'f', not 'true'/'false'
*  Added "format" ... for output
*  tid output format was at the start, not the end
*  I didn't add space between point x,y because the output has no space
*  I spelled out "point" instead of "pt"
*  "line" has two very different input formats so I listed both
   (more different than others like boolean)
*  I didn't see the need to say "datatype" for LSN and UUID
*  I improved the txid_snapshot description
*  There was no description for table_am_handler

Patch attached.

-- 
  Bruce Momjian  https://momjian.us
  EDB  https://enterprisedb.com

  Only you can decide what is important to you.
diff --git a/doc/src/sgml/datatype.sgml b/doc/src/sgml/datatype.sgml
index 5a6cfbd94d..55340b00ad 100644
--- a/doc/src/sgml/datatype.sgml
+++ b/doc/src/sgml/datatype.sgml
@@ -3366,13 +3366,13 @@ SELECT person.name, holidays.num_weeks FROM person, holidays
 lseg
 32 bytes
 Finite line segment
-((x1,y1),(x2,y2))
+[(x1,y1),(x2,y2)]


 box
 32 bytes
 Rectangular box
-((x1,y1),(x2,y2))
+(x1,y1),(x2,y2)


 path
diff --git a/src/include/catalog/pg_type.dat b/src/include/catalog/pg_type.dat
index 92bcaf2c73..6fd3c64e40 100644
--- a/src/include/catalog/pg_type.dat
+++ b/src/include/catalog/pg_type.dat
@@ -32,7 +32,7 @@
 # OIDS 1 - 99
 
 { oid => '16', array_type_oid => '1000',
-  descr => 'boolean, \'true\'/\'false\'',
+  descr => 'boolean, format \'t\'/\'f\'',
   typname => 'bool', typlen => '1', typbyval => 't', typcategory => 'B',
   typispreferred => 't', typinput => 'boolin', typoutput => 'boolout',
   typreceive => 'boolrecv', typsend => 'boolsend', typalign => 'c' },
@@ -90,7 +90,7 @@
   typispreferred => 't', typinput => 'oidin', typoutput => 'oidout',
   typreceive => 'oidrecv', typsend => 'oidsend', typalign => 'i' },
 { oid => '27', array_type_oid => '1010',
-  descr => '(block, offset), physical location of tuple',
+  descr => 'tuple physical location, format \'(block,offset)\'',
   typname => 'tid', typlen => '6', typbyval => 'f', typcategory => 'U',
   typinput => 'tidin', typoutput => 'tidout', typreceive => 'tidrecv',
   typsend => 'tidsend', typalign => 's' },
@@ -179,34 +179,34 @@
 # OIDS 600 - 699
 
 { oid => '600', array_type_oid => '1017',
-  descr => 'geometric point \'(x, y)\'',
+  descr => 'geometric point, format \'(x,y)\'',
   typname => 'point', typlen => '16', typbyval => 'f', typcategory => 'G',
   typsubscript => 'raw_array_subscript_handler', typelem => 'float8',
   typinput => 'point_in', typoutput => 'point_out', typreceive => 'point_recv',
   typsend => 'point_send', typalign => 'd' },
 { oid => '601', array_type_oid => '1018',
-  descr => 'geometric line segment \'(pt1,pt2)\'',
+  descr => 'geometric line segment, format \'[point1,point2]\'',
   typname => 'lseg', typlen => '32', typbyval => 'f', typcategory => 'G',
   typsubscript => 'raw_array_subscript_handler', typelem => 'point',
   typinput => 'lseg_in', typoutput => 'lseg_out', typreceive => 'lseg_recv',
   typsend => 'lseg_send', typalign => 'd' },
 { oid => '602', array_type_oid => '1019',
-  descr => 'geometric path \'(pt1,...)\'',
+  descr => 'geometric path, format \'(point1,...)\'',
   typname => 'path', typlen => '-1', typbyval => 'f', typcategory => 'G',
   typinput => 'path_in', typoutput => 'path_out', typreceive => 'path_recv',
   typsend => 'path_send', typalign => 'd', typstorage => 'x' },
 { oid => '603', array_type_oid => '1020',
-  descr => 'geometric box \'(lower left,upper right)\'',
+  descr => 'geometric box, format \'lower left point,upper right point\'',
   typname => 'box', typlen => '32', typbyval => 'f', typcategory => 'G',
   typdelim => ';', typsubscript => 'raw_array_subscript_handler',
   typelem => 'point', typinput => 'box_in', typoutput => 'box_out',
   typreceive => 'box_recv', typsend => 'box_send', typalign => 'd' },
 { oid => '604', array_type_oid => '1027',
-  descr => 'geometric polygon \'(pt1,...)\'',
+  descr => 'geometric polygon, format \'(point1,...)\'',
   

Re: Don't pass NULL pointer to strcmp().

[Tom Lane]

Xing Guo  writes:
> Thanks for your comments. I have updated the patch accordingly.

I'm leery of accepting this patch, as I see no reason that we
should consider it valid for an extension to have a string GUC
with a boot_val of NULL.

I realize that we have a few core GUCs that are like that, but
I'm pretty sure that every one of them has special-case code
that initializes the GUC to something non-null a bit later on
in startup.  I don't think there are any cases where a string
GUC's persistent value will be null, and I don't like the
idea of considering that to be an allowed case.  It would
open the door to more crash situations, and it brings up the
old question of how could a user tell NULL from empty string
(via SHOW or current_setting() or whatever).  Besides, what's
the benefit really?

regards, tom lane

Re: Commitfest manager November 2023

[shihao zhong]

On Wed, Nov 1, 2023 at 7:59 AM Aleksander Alekseev 
wrote:

> Hi,
>
> > I didn't see any recent mentions in the archives, so I'll volunteer to
> > be CF manager for 2023-11.
>
> I would love to help with that if you need.
>
> --
> Thanks,
>
   Shihao

Re: Tab completion regression test failed on illumos

[Tom Lane]

Japin Li  writes:
> I try to run regression test on illumos, the 010_tab_completion will
> failed because of timeout.

Why are you getting this?

>  Begin standard error
> psql::6: WARNING:  wal_level is insufficient to publish logical 
> changes
> HINT:  Set wal_level to "logical" before creating subscriptions.
>  End standard error

Not sure, but perhaps that unexpected output is confusing the test.

regards, tom lane

Re: GUC names in messages

[Tom Lane]

Daniel Gustafsson  writes:
> On 1 Nov 2023, at 10:22, Peter Smith  wrote:
>> One idea to achieve consistency might be to always substitute GUC
>> names using a macro.
>> 
>> #define GUC_NAME(s) ("\"" s "\"")
>> 
>> ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
>> errmsg("%s must be at least twice %s",
>> GUC_NAME("max_wal_size"),
>> GUC_NAME("wal_segment_size";

> Something like this might make translations harder since the remaining string
> leaves little context about the message.  We already have that today to some
> extent (so it might not be an issue), and it might be doable to automatically
> add translator comments, but it's something to consider.

Our error message style guidelines say not to assemble messages out
of separate parts, because it makes translation difficult.  Originally
we applied that rule to GUC names mentioned in messages as well.
Awhile ago the translation team decided that that made for too many
duplicative translations, so they'd be willing to compromise on
substituting GUC names.  That's only been changed in a haphazard
fashion though, mostly in cases where there actually were duplicative
messages that could be merged this way.  And there's never been any
real clarity about whether to quote GUC names, though certainly we're
more likely to quote anything injected with %s.  So that's why we have
a mishmash right now.

I'm not enamored of the GUC_NAME idea suggested above.  I don't
think it buys anything, and what it does do is make *every single
one* of our GUC-mentioning messages wrong.  I think if we want to
standardize here, we should standardize on something that's
already pretty common in the code base.

Another problem with the idea as depicted above is that it
mistakenly assumes that "..." is the correct quoting method
in all languages.  You could make GUC_NAME be a pure no-op
macro and continue to put quoting in the translatable string
where it belongs, but then the macro brings even less value.

regards, tom lane

Re: speed up a logical replica setup

[Euler Taveira]

On Tue, Oct 31, 2023, at 11:46 PM, shihao zhong wrote:
> I think this is duplicate with https://commitfest.postgresql.org/45/4637/
> 
> The new status of this patch is: Waiting on Author
> 

I withdrew the other entry.


--
Euler Taveira
EDB   https://www.enterprisedb.com/

Re: speed up a logical replica setup

[Ashutosh Bapat]

On Thu, Oct 26, 2023 at 5:17 PM Ashutosh Bapat
 wrote:
>
> On Mon, Oct 23, 2023 at 9:34 AM Euler Taveira  wrote:
>
> >
> > It is still a WIP but I would like to share it and get some feedback.
> >
> >
>
> I have started reviewing the patch. I have just read through all the
> code. It's well documented and clear. Next I will review the design in
> detail.

Here are some comments about functionality and design.

+ 
+ 
+ pg_subscriber creates one replication slot for
+ each specified database on the source server. The replication slot name
+ contains a pg_subscriber prefix. These replication
+ slots will be used by the subscriptions in a future step. Another
+ replication slot is used to get a consistent start location. This
+ consistent LSN will be used as a stopping point in the  parameter and by the
+ subscriptions as a replication starting point. It guarantees that no
+ transaction will be lost.
+ 
+ 

CREATE_REPLICATION_SLOT would wait for any incomplete transaction to
complete. So it may not be possible to have an incomplete transaction
on standby when it comes out of recovery. Am I correct? Can we please
have a testcase where we test this scenario? What about a prepared
transactions?

+
+ 
+ 
+ pg_subscriber writes recovery parameters into
+ the target data directory and start the target server. It specifies a LSN
+ (consistent LSN that was obtained in the previous step) of write-ahead
+ log location up to which recovery will proceed. It also specifies
+ promote as the action that the server should take once
+ the recovery target is reached. This step finishes once the server ends
+ standby mode and is accepting read-write operations.
+ 
+ 

At this stage the standby would have various replication objects like
publications, subscriptions, origins inherited from the upstream
server and possibly very much active. With failover slots, it might
inherit replication slots. Is it intended that the new subscriber also
acts as publisher for source's subscribers OR that the new subscriber
should subscribe to the upstreams of the source? Some use cases like
logical standby might require that but a multi-master multi-node setup
may not. The behaviour should be user configurable.

There may be other objects in this category which need special consideration on
the subscriber. I haven't fully thought through the list of such objects.

+ uses the replication slot that was created in a previous step. The
+ subscription is created but it is not enabled yet. The reason is the
+ replication progress must be set to the consistent LSN but replication
+ origin name contains the subscription oid in its name. Hence, the

Not able to understand the sentence "The reason is ... in its name".
Why is subscription OID in origin name matters?

+ 
+ pg_subscriber stops the target server to change
+ its system identifier.
+ 

I expected the subscriber to be started after this step.

Why do we need pg_resetwal?

+ appendPQExpBuffer(str, "CREATE_REPLICATION_SLOT \"%s\"", slot_name);
+ appendPQExpBufferStr(str, " LOGICAL \"pgoutput\" NOEXPORT_SNAPSHOT");

Hardcoding output plugin name would limit this utility only to
built-in plugin. Any reason for that limitation?

In its current form the utility creates a logical subscriber which
subscribes to all the tables (and sequences when we have sequence
replication). But it will be useful even in case of selective
replication from a very large database. In such a case the new
subscriber will need to a. remove the unwanted objects b. subscriber
will need to subscribe to publications publishing the "interesting"
objects. We don't need to support this case, but the current
functionality (including the interface) and design shouldn't limit us
from doing so. Have you thought about this case?

I noticed some differences between this and a similar utility
https://github.com/2ndQuadrant/pglogical/blob/REL2_x_STABLE/pglogical_create_subscriber.c.
I will be reviewing these differences next to see if we are missing
anything here.

-- 
Best Wishes,
Ashutosh Bapat

Re: Don't pass NULL pointer to strcmp().

[Xing Guo]

Hi Aleksander and Junwang,

Thanks for your comments. I have updated the patch accordingly.

Best Regards,
Xing








On Wed, Nov 1, 2023 at 7:44 PM Aleksander Alekseev 
wrote:

> Hi,
>
> > > I found that there's a nullable pointer being passed to strcmp() and
> > > can make the server crash. It can be reproduced on the latest master
> > > branch by crafting an extension[1]. Patch for fixing it is attatched.
> > >
> > > [1] https://github.com/higuoxing/guc_crash/tree/pg
>
> Thanks for reporting. I can confirm that the issue reproduces on the
> `master` branch and the proposed patch fixes it.
>
> > Can we set a string guc to NULL? If not, `*lconf->variable == NULL` would
> > be unnecessary.
>
> Judging by the rest of the code we better keep it, at least for consistenc.
>
> I see one more place with a similar code in guc.c around line 1472.
> Although I don't have exact steps to trigger a crash I suggest adding
> a similar check there.
>
> --
> Best regards,
> Aleksander Alekseev
>
From bf0a9f848e69097a034692ed7b0be0ad81a5d991 Mon Sep 17 00:00:00 2001
From: Xing Guo 
Date: Wed, 1 Nov 2023 21:00:13 +0800
Subject: [PATCH v2] Don't use strcmp() with nullable pointers.

Passing a NULL pointer to strcmp() is an undefined behavior. It can make
the PostgreSQL server crash. This patch helps fix it.
---
 src/backend/utils/misc/guc.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 39d3775e80..1d62523412 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -1473,7 +1473,8 @@ check_GUC_init(struct config_generic *gconf)
 			{
 struct config_string *conf = (struct config_string *) gconf;
 
-if (*conf->variable != NULL && strcmp(*conf->variable, conf->boot_val) != 0)
+if (*conf->variable != NULL &&
+	(conf->boot_val == NULL || strcmp(*conf->variable, conf->boot_val) != 0))
 {
 	elog(LOG, "GUC (PGC_STRING) %s, boot_val=%s, C-var=%s",
 		 conf->gen.name, conf->boot_val ? conf->boot_val : "", *conf->variable);
@@ -5255,7 +5256,9 @@ get_explain_guc_options(int *num)
 {
 	struct config_string *lconf = (struct config_string *) conf;
 
-	modified = (strcmp(lconf->boot_val, *(lconf->variable)) != 0);
+	modified = (lconf->boot_val == NULL ||
+*lconf->variable == NULL ||
+strcmp(lconf->boot_val, *(lconf->variable)) != 0);
 }
 break;
 
-- 
2.42.0

Re: trying again to get incremental backup

[Jakub Wartak]

On Mon, Oct 30, 2023 at 6:46 PM Robert Haas  wrote:
>
> On Thu, Sep 28, 2023 at 6:22 AM Jakub Wartak
>  wrote:
> > If that is still an area open for discussion: wouldn't it be better to
> > just specify LSN as it would allow resyncing standby across major lag
> > where the WAL to replay would be enormous? Given that we had
> > primary->standby where standby would be stuck on some LSN, right now
> > it would be:
> > 1) calculate backup manifest of desynced 10TB standby (how? using
> > which tool?)  - even if possible, that means reading 10TB of data
> > instead of just putting a number, isn't it?
> > 2) backup primary with such incremental backup >= LSN
> > 3) copy the incremental backup to standby
> > 4) apply it to the impaired standby
> > 5) restart the WAL replay
>
> As you may be able to tell from the flurry of posts and new patch
> sets, I'm trying hard to sort out the remaining open items that
> pertain to this patch set, and I'm now back to thinking about this
> one.
>
> TL;DR: I think the idea has some potential, but there are some
> pitfalls that I'm not sure how to address.
>
> I spent some time looking at how we currently use the data from the
> backup manifest. Currently, we do two things with it. First, when
> we're backing up each file, we check whether it's present in the
> backup manifest and, if not, we back it up in full. This actually
> feels fairly poor. If it makes any difference at all, then presumably
> the underlying algorithm is buggy and needs to be fixed. Maybe that
> should be ripped out altogether or turned into some kind of sanity
> check that causes a big explosion if it fails. Second, we check
> whether the WAL ranges reported by the client match up with the
> timeline history of the server (see PrepareForIncrementalBackup). This
> set of sanity checks seems fairly important to me, and I'd regret
> discarding them. I think there's some possibility that they might
> catch user error, like where somebody promotes multiple standbys and
> maybe they even get the same timeline on more than one of them, and
> then confusion might ensue.
[..]

> Another practical problem here is that, right now, pg_combinebackup
> doesn't have an in-place mode. It knows how to take a bunch of input
> backups and write out an output backup, but that output backup needs
> to go into a new, fresh directory (or directories plural, if there are
> user-defined tablespaces). I had previously considered adding such a
> mode, but the idea I had at the time wouldn't have worked for this
> case. I imagined that someone might want to run "pg_combinebackup
> --in-place full incr" and clobber the contents of the incr directory
> with the output, basically discarding the incremental backup you took
> in favor of a full backup that could have been taken at the same point
> in time.
[..]

Thanks for answering! It all sounds like this
resync-standby-using-primary-incrbackup idea isn't fit for the current
pg_combinebackup, but rather for a new tool hopefully in future. It
could take the current LSN from stuck standby, calculate manifest on
the lagged and offline standby (do we need to calculate manifest
Checksum in that case? I cannot find code for it), deliver it via
"UPLOAD_MANIFEST" to primary and start fetching and applying the
differences while doing some form of copy-on-write from old & incoming
incrbackup data to "$relfilenodeid.new" and then durable_unlink() old
one and durable_rename("$relfilenodeid.new", "$relfilenodeid". Would
it still be possible in theory? (it could use additional safeguards
like rename controlfile when starting and just before ending to
additionally block startup if it hasn't finished). Also it looks as
per comment nearby struct IncrementalBackupInfo.manifest_files that
even checksums are just more for safeguarding rather than core
implementation (?)

What I've meant in the initial idea is not to hinder current efforts,
but asking if the current design will not stand in a way for such a
cool new addition in future ?

> One thing I also realized when thinking about this is that you could
> probably hose yourself with the patch set as it stands today by taking
> a full backup, downgrading to wal_level=minimal for a while, doing
> some WAL-skipping operations, upgrading to a higher WAL-level again,
> and then taking an incremental backup. I think the solution to that is
> probably for the WAL summarizer to refuse to run if wal_level=minimal.
> Then there would be a gap in the summary files which an incremental
> backup attempt would detect.

As per earlier test [1], I've already tried to simulate that in
incrbackuptests-0.1.tgz/test_across_wallevelminimal.sh , but that
worked (but that was with CTAS-wal-minimal-optimization -> new
relfilenodeOID is used for CTAS which got included in the incremental
backup as it's new file) Even retested that with Your v7 patch with
asserts, same. When simulating with "BEGIN; TRUNCATE nightmare; COPY
nightmare FROM '/tmp/copy.out'; COMMIT;" on 

Re: Question about non-blocking mode in libpq

[Bruce Momjian]

On Tue, Oct 31, 2023 at 10:16:07PM -0400, Tom Lane wrote:
> Bruce Momjian  writes:
> > On Tue, Oct 31, 2023 at 09:11:06PM -0400, Tom Lane wrote:
> >> What I'm objecting to is removal of the bit about "if they need to be
> >> called again".  That provides a hint that retry is the appropriate
> >> response to a failure.  Admittedly, it's not 100% clear, but your
> >> version makes it 0% clear.
> 
> > I thought the original docs said you had to re-call on failure (it would
> > not block but it would fail if it could not be sent), while we are now
> > saying that it will be queued in the input buffer.
> 
> For these functions in nonblock mode, failure means "we didn't queue it".
> 
> > Is retry really something we need to mention now?  If out of memory is
> > our only failure case now ("unable to enlarge the buffer because OOM"),
> > is retry really a realistic option?
> 
> Well, ideally the application would do something to alleviate the
> OOM problem before retrying.  I don't know if we want to go so far
> as to discuss that.  I do object to giving the impression that
> failure is impossible, which I think your proposed wording does.
> 
> An orthogonal issue with your latest wording is that it's unclear
> whether *unsuccessful* calls to these functions will block.

Okay, I see your point now.  Here is an updated patch that addresses
both issues.

-- 
  Bruce Momjian  https://momjian.us
  EDB  https://enterprisedb.com

  Only you can decide what is important to you.
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 64b2910fee..0e528d 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -5316,11 +5316,12 @@ int PQsetnonblocking(PGconn *conn, int arg);
   
 
   
-   In the nonblocking state, calls to
+   In the nonblocking state, successful calls to
, ,
, ,
-   and  will not block but instead return
-   an error if they need to be called again.
+   and  will not block;  their changes
+   are stored in the local output buffer until they are flushed. 
+   Unsuccessful calls will return an error and must be retried.
   
 
   

Re: Tab completion regression test failed on illumos

[Aleksander Alekseev]

Hi,

> I try to run regression test on illumos, the 010_tab_completion will
> failed because of timeout.
>
> Here is my build commands and logs:
>
> [...]
>
> Any suggestions?  Thanks in advance!

It's hard to say what went wrong with this output due to lack of
backtrace. I would suggest adding debug output to
010_tab_completion.pl to figure out on which line the script fails.
Then I would figure out the exact command that failed. Then I would
execute it manually and compare the result with my expectations.

-- 
Best regards,
Aleksander Alekseev

Re: btree: implement dynamic prefix truncation (was: Improving btree performance through specializing by key shape, take 2)

[Pavel Stehule]

st 1. 11. 2023 v 11:32 odesílatel Matthias van de Meent <
boekewurm+postg...@gmail.com> napsal:

> On Wed, 1 Nov 2023 at 07:47, Pavel Stehule 
> wrote:
> >
> > Hi
> >
> > út 31. 10. 2023 v 22:12 odesílatel Matthias van de Meent <
> boekewurm+postg...@gmail.com> napsal:
> >> This patch was originally suggested at [0], but it was mentioned that
> >> they could be pulled out into it's own thread. Earlier, the
> >> performance gains were not clearly there for just this patch, but
> >> after further benchmarking this patch stands on its own for
> >> performance: it sees no obvious degradation of performance, while
> >> gaining 0-5% across various normal indexes on the cc-complete sample
> >> dataset, with the current worst-case index shape getting a 60%+
> >> improved performance on INSERTs in the tests at [0].
> >
> >
> > +1
>
> Thanks for showing interest.
>
> > This can be nice functionality. I had a customer with a very slow index
> scan - the main problem was a long common prefix like prg010203.
>
> I'll have to note that this patch doesn't cover cases where e.g. text
> attributes have large shared prefixes, but are still unique: the
> dynamic prefix compression in this patch is only implemented at the
> tuple attribute level; it doesn't implement type aware dynamic prefix
> compression inside the attributes. So, a unique index on a column of
> int32 formatted like '%0100i' would not materially benefit from this
> patch.
>
> While would certainly be possible to add some type-level prefix
> truncation in the framework of this patch, adding that would require
> significant code churn in btree compare operators, because we'd need
> an additional return argument to contain a numerical "shared prefix",
> and that is not something I was planning to implement in this patch.
>

Thanks for the explanation.

Pavel


> Kind regards,
>
> Matthias van de Meent
> Neon (https://neon.tech)
>

Re: remaining sql/json patches

[Nikita Malakhov]

Hi!

According to the discussion above, I've added the 'proerrsafe' attribute to
the PG_PROC relation.
The same was done some time ago by Nikita Glukhov but this part was
reverted.
This is a WIP patch, I am new to this part of Postgres, so please correct
me if I'm going the wrong way.

--
Nikita Malakhov
Postgres Professional
The Russian Postgres Company
https://postgrespro.ru/


0001_proerrsafe_attr_v1.patch
Description: Binary data

Re: Commitfest manager November 2023

[Aleksander Alekseev]

Hi,

> I didn't see any recent mentions in the archives, so I'll volunteer to
> be CF manager for 2023-11.

Many thanks for volunteering! If you need any help please let me know.

-- 
Best regards,
Aleksander Alekseev

Re: Don't pass NULL pointer to strcmp().

[Aleksander Alekseev]

Hi,

> > I found that there's a nullable pointer being passed to strcmp() and
> > can make the server crash. It can be reproduced on the latest master
> > branch by crafting an extension[1]. Patch for fixing it is attatched.
> >
> > [1] https://github.com/higuoxing/guc_crash/tree/pg

Thanks for reporting. I can confirm that the issue reproduces on the
`master` branch and the proposed patch fixes it.

> Can we set a string guc to NULL? If not, `*lconf->variable == NULL` would
> be unnecessary.

Judging by the rest of the code we better keep it, at least for consistenc.

I see one more place with a similar code in guc.c around line 1472.
Although I don't have exact steps to trigger a crash I suggest adding
a similar check there.

-- 
Best regards,
Aleksander Alekseev

Re: MERGE ... RETURNING

[Vik Fearing]

On 11/1/23 11:12, Dean Rasheed wrote:

On Tue, 31 Oct 2023 at 23:19, Vik Fearing  wrote:


On 10/31/23 19:28, Jeff Davis wrote:


Assuming we have one RETURNING clause at the end, then it creates the
problem of how to communicate which WHEN clause a tuple came from,
whether it's the old or the new version, and/or which action was
performed on that tuple.

How do we communicate any of those things? We need to get that
information into the result table somehow, so it should probably be
some kind of expression that can exist in the RETURNING clause. But
what kind of expression?

(a) It could be a totally new expression kind with a new keyword (or
recycling some existing keywords for the same effect, or something that
looks superficially like a function call but isn't) that's only valid
in the RETURNING clause of a MERGE statement. If you use it in another
expression (say the targetlist of a SELECT statement), then you'd get a
failure at parse analysis time.


This would be my choice, the same as how the standard GROUPING()
"function" for grouping sets is implemented by GroupingFunc.



Something I'm wondering about is to what extent this discussion is
driven by concerns about aspects of the implementation (specifically,
references to function OIDs in code), versus a desire for a different
user-visible syntax. To a large extent, those are orthogonal
questions.



For my part, I am most concerned about the language level.  I am 
sympathetic to the implementers' issues, but that is not my main focus.


So please do not take my implementation advice into account when I voice 
my opinions.

--
Vik Fearing

Re: pg_resetwal tests, logging, and docs update

[Aleksander Alekseev]

Hi,

> Hmm.  I think maybe we should fix the behavior of
> GetDataDirectoryCreatePerm() to be more consistent between Windows and
> non-Windows.  This is usually the first function a program uses on the
> proposed data directory, so it's also responsible for reporting if the
> data directory does not exist.  But then on Windows, because the
> function does nothing, those error scenarios end up on quite different
> code paths, and I'm not sure if those are really checked that carefully.
>   I think we can make this more robust if we have
> GetDataDirectoryCreatePerm() still run the stat() call on the proposed
> data directory and report the error.  See attached patch.

Yep, that would be much better.

Attaching all three patches together in order to make sure cfbot is
still happy with them while the `master` branch is evolving.

Assuming cfbot will have no complaints I suggest merging them.

-- 
Best regards,
Aleksander Alekseev


v4-0003-pg_resetwal-Add-more-tests-and-test-coverage.patch
Description: Binary data


v4-0001-More-consistent-behavior-of-GetDataDirectoryCreat.patch
Description: Binary data


v4-0002-doc-pg_resetwal-Add-comments-how-the-multipliers-.patch
Description: Binary data

Re: btree: implement dynamic prefix truncation (was: Improving btree performance through specializing by key shape, take 2)

[Matthias van de Meent]

On Wed, 1 Nov 2023 at 07:47, Pavel Stehule  wrote:
>
> Hi
>
> út 31. 10. 2023 v 22:12 odesílatel Matthias van de Meent 
>  napsal:
>> This patch was originally suggested at [0], but it was mentioned that
>> they could be pulled out into it's own thread. Earlier, the
>> performance gains were not clearly there for just this patch, but
>> after further benchmarking this patch stands on its own for
>> performance: it sees no obvious degradation of performance, while
>> gaining 0-5% across various normal indexes on the cc-complete sample
>> dataset, with the current worst-case index shape getting a 60%+
>> improved performance on INSERTs in the tests at [0].
>
>
> +1

Thanks for showing interest.

> This can be nice functionality. I had a customer with a very slow index scan 
> - the main problem was a long common prefix like prg010203.

I'll have to note that this patch doesn't cover cases where e.g. text
attributes have large shared prefixes, but are still unique: the
dynamic prefix compression in this patch is only implemented at the
tuple attribute level; it doesn't implement type aware dynamic prefix
compression inside the attributes. So, a unique index on a column of
int32 formatted like '%0100i' would not materially benefit from this
patch.

While would certainly be possible to add some type-level prefix
truncation in the framework of this patch, adding that would require
significant code churn in btree compare operators, because we'd need
an additional return argument to contain a numerical "shared prefix",
and that is not something I was planning to implement in this patch.

Kind regards,

Matthias van de Meent
Neon (https://neon.tech)

Re: Don't pass NULL pointer to strcmp().

[Junwang Zhao]

On Wed, Nov 1, 2023 at 5:25 PM Xing Guo  wrote:
>
> Hi hackers,
>
> I found that there's a nullable pointer being passed to strcmp() and
> can make the server crash. It can be reproduced on the latest master
> branch by crafting an extension[1]. Patch for fixing it is attatched.
>
> [1] https://github.com/higuoxing/guc_crash/tree/pg
>

Can we set a string guc to NULL? If not, `*lconf->variable == NULL` would
be unnecessary.

> --
> Best Regards,
> Xing



-- 
Regards
Junwang Zhao

Re: More new SQL/JSON item methods

[Andrew Dunstan]

On 2023-11-01 We 03:00, Jeevan Chalke wrote:

Hello,

On Tue, Oct 24, 2023 at 6:41 PM Andrew Dunstan  
wrote:



On 2023-10-19 Th 02:06, Jeevan Chalke wrote:

Thanks, Peter for the comments.

On Fri, Oct 6, 2023 at 5:13 PM Peter Eisentraut
 wrote:

On 29.08.23 09:05, Jeevan Chalke wrote:
> v1-0001-Implement-jsonpath-.bigint-.integer-and-.number-m.patch
>
> This commit implements jsonpath .bigint(), .integer(), and
.number()
> methods.  The JSON string or a numeric value is converted
to the
> bigint, int4, and numeric type representation.

A comment that applies to all of these: These add various
keywords,
switch cases, documentation entries in some order.  Are we
happy with
that?  Should we try to reorder all of that for better
maintainability
or readability?


Yeah, that's the better suggestion. While implementing these
methods, I was confused about where to put them exactly and tried
keeping them in some logical place.
I think once these methods get in, we can have a follow-up patch
reorganizing all of these.



I think it would be better to organize things how we want them
before adding in more stuff.


I have tried reordering all the jsonpath Operators and Methods 
consistently. With this patch, they all appear in the same order when 
together in the group.


In some switch cases, they are still divided, like in 
flattenJsonPathParseItem(), where 2-arg, 1-arg, and no-arg cases are 
clubbed together. But I have tried to keep them in order in those 
subgroups.


I will rebase my patches for this task on this patch, but before doing 
so, I  would like to get your views on this reordering.





This appears to be reasonable. Maybe we need to add a note in one or two 
places about maintaining the consistency?



cheers


andrew


--
Andrew Dunstan
EDB:https://www.enterprisedb.com

Re: MERGE ... RETURNING

[Dean Rasheed]

On Tue, 31 Oct 2023 at 23:19, Vik Fearing  wrote:
>
> On 10/31/23 19:28, Jeff Davis wrote:
>
> > Assuming we have one RETURNING clause at the end, then it creates the
> > problem of how to communicate which WHEN clause a tuple came from,
> > whether it's the old or the new version, and/or which action was
> > performed on that tuple.
> >
> > How do we communicate any of those things? We need to get that
> > information into the result table somehow, so it should probably be
> > some kind of expression that can exist in the RETURNING clause. But
> > what kind of expression?
> >
> > (a) It could be a totally new expression kind with a new keyword (or
> > recycling some existing keywords for the same effect, or something that
> > looks superficially like a function call but isn't) that's only valid
> > in the RETURNING clause of a MERGE statement. If you use it in another
> > expression (say the targetlist of a SELECT statement), then you'd get a
> > failure at parse analysis time.
>
> This would be my choice, the same as how the standard GROUPING()
> "function" for grouping sets is implemented by GroupingFunc.
>

Something I'm wondering about is to what extent this discussion is
driven by concerns about aspects of the implementation (specifically,
references to function OIDs in code), versus a desire for a different
user-visible syntax. To a large extent, those are orthogonal
questions.

(As an aside, I would note that there are already around a dozen
references to specific function OIDs in the parse analysis code, and a
lot more if you grep more widely across the whole of the backend
code.)

At one point, as I was writing this patch, I went part-way down the
route of adding a new node type (I think I called it MergeFunc), for
these merge support functions, somewhat inspired by GroupingFunc. In
the end, I backed out of that approach, because it seemed to be
introducing a lot of unnecessary additional complexity, and I decided
that a regular FuncExpr would suffice.

If pg_merge_action() and pg_merge_when_clause_number() were
implemented using a MergeFunc node, it would reduce the number of
places that refer to specific function OIDs. Basically, a MergeFunc
node would be very much like a FuncExpr node, except that it would
have a "levels up" field, set during parse analysis, at the point
where we check that it is being used in a merge returning clause, and
this field would be used during subselect planning. Note, however,
that that doesn't entirely eliminate references to specific function
OIDs -- the parse analysis code would still do that. Also, additional
special-case code in the executor would be required to handle
MergeFunc nodes. Also, code like IncrementVarSublevelsUp() would need
adjusting, and anything else like that.

A separate question is what the syntax should be. We could invent a
new syntax, like GROUPING(). Perhaps:

  MERGE(ACTION) instead of pg_merge_action()
  MERGE(CLAUSE NUMBER) instead of pg_merge_when_clause_number()

But note that those could equally well generate either FuncExpr nodes
or MergeFunc nodes, so the syntax question remains orthogonal to that
internal implementation question.

If MERGE(...) (or MERGING(...), or whatever) were part of the SQL
standard, then that would be the clear choice. But since it's not, I
don't see any real advantage to inventing special syntax here, rather
than just using a regular function call. In fact, it's worse, because
if this were to work like GROUPING(), it would require MERGE (or
MERGING, or whatever) to be a COL_NAME_KEYWORD, where currently MERGE
is an UNRESERVED_KEYWORD, and that would break any existing
user-defined functions with that name, whereas the "pg_" prefix of my
functions makes that much less likely.

So on the syntax question, in the absence of anything specific from
the SQL standard, I think we should stick to builtin functions,
without inventing special syntax. That doesn't preclude adding special
syntax later, if the SQL standard mandates it, but that might be
harder, if we invent our own syntax now.

On the implementation question, I'm not completely against the idea of
a MergeFunc node, but it does feel a little over-engineered.

Regards,
Dean

Re: "38.10.10. Shared Memory and LWLocks" may require a clarification

[Aleksander Alekseev]

> The result has been applied as fe705ef6fc1d.

Many thanks!

-- 
Best regards,
Aleksander Alekseev

Re: GUC names in messages

[Daniel Gustafsson]

> On 1 Nov 2023, at 10:22, Peter Smith  wrote:
> 
> On Wed, Nov 1, 2023 at 8:02 PM Peter Smith  wrote:
> ...
>> 
>> I had intended to make a patch to address the inconsistency, but
>> couldn't decide which of those styles was the preferred one.
>> 
>> Then I worried this could be the tip of the iceberg -- GUC names occur
>> in many other error messages where they are sometimes quoted and
>> sometimes not quoted:
>> e.g. Not quoted -- errhint("You might need to run fewer transactions
>> at a time or increase max_connections.")));
>> e.g. Quoted -- errmsg("\"max_wal_size\" must be at least twice
>> \"wal_segment_size\"")));
>> 
>> Ideally, they should all look the same everywhere, shouldn't they?
>> 
> 
> One idea to achieve consistency might be to always substitute GUC
> names using a macro.
> 
> #define GUC_NAME(s) ("\"" s "\"")
> 
> ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
>errmsg("%s must be at least twice %s",
>GUC_NAME("max_wal_size"),
>GUC_NAME("wal_segment_size";

Something like this might make translations harder since the remaining string
leaves little context about the message.  We already have that today to some
extent (so it might not be an issue), and it might be doable to automatically
add translator comments, but it's something to consider.

--
Daniel Gustafsson

Don't pass NULL pointer to strcmp().

[Xing Guo]

Hi hackers,

I found that there's a nullable pointer being passed to strcmp() and
can make the server crash. It can be reproduced on the latest master
branch by crafting an extension[1]. Patch for fixing it is attatched.

[1] https://github.com/higuoxing/guc_crash/tree/pg

-- 
Best Regards,
Xing
From dcd7a49190f0e19ba0a1e697cac45724450f6365 Mon Sep 17 00:00:00 2001
From: Xing Guo 
Date: Wed, 1 Nov 2023 16:41:49 +0800
Subject: [PATCH] Don't use strcmp() with nullable pointers.

Passing a NULL pointer to strcmp() is an undefined behavior. It can make
the PostgreSQL server crash. This patch helps fix it.
---
 src/backend/utils/misc/guc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 39d3775e80..b277c48925 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -5255,7 +5255,9 @@ get_explain_guc_options(int *num)
 {
 	struct config_string *lconf = (struct config_string *) conf;
 
-	modified = (strcmp(lconf->boot_val, *(lconf->variable)) != 0);
+	modified = (lconf->boot_val == NULL ||
+*lconf->variable == NULL ||
+strcmp(lconf->boot_val, *(lconf->variable)) != 0);
 }
 break;
 
-- 
2.42.0

Re: GUC names in messages

[Daniel Gustafsson]

> On 1 Nov 2023, at 10:02, Peter Smith  wrote:

> GUC_check_errdetail("effective_io_concurrency must be set to 0 on
> platforms that lack posix_fadvise().");
> src/backend/commands/variable.c:
> GUC_check_errdetail("maintenance_io_concurrency must be set to 0 on
> platforms that lack posix_fadvise().");

These should be substituted to reduce the number of distinct messages that need
to be translated.  I wouldn't be surprised if more like these have slipped
through.

> I had intended to make a patch to address the inconsistency, but
> couldn't decide which of those styles was the preferred one.

Given the variety in the codebase I don't think there is a preferred one.

> Then I worried this could be the tip of the iceberg

All good rabbit-holes uncovered during hacking are.. =)

> Ideally, they should all look the same everywhere, shouldn't they?

Having a policy would be good, having one which is known and enforced is even
better (like how we are consistent around error messages based on our Error
Message Style Guide).

--
Daniel Gustafsson

Re: GUC names in messages

[Peter Smith]

On Wed, Nov 1, 2023 at 8:02 PM Peter Smith  wrote:
...
>
> I had intended to make a patch to address the inconsistency, but
> couldn't decide which of those styles was the preferred one.
>
> Then I worried this could be the tip of the iceberg -- GUC names occur
> in many other error messages where they are sometimes quoted and
> sometimes not quoted:
> e.g. Not quoted -- errhint("You might need to run fewer transactions
> at a time or increase max_connections.")));
> e.g. Quoted -- errmsg("\"max_wal_size\" must be at least twice
> \"wal_segment_size\"")));
>
> Ideally, they should all look the same everywhere, shouldn't they?
>

One idea to achieve consistency might be to always substitute GUC
names using a macro.

#define GUC_NAME(s) ("\"" s "\"")

ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("%s must be at least twice %s",
GUC_NAME("max_wal_size"),
GUC_NAME("wal_segment_size";

Thoughts?

==
Kind Regards,
Peter Smith.
Fujitsu Australia

GUC names in messages

[Peter Smith]

Hi hackers,

While reviewing another patch I noticed how the GUCs are
inconsistently named within the GUC_check_errdetail messages:

==

below, the GUC name is embedded but not quoted:

src/backend/access/transam/xlogprefetcher.c:
GUC_check_errdetail("recovery_prefetch is not supported on platforms
that lack posix_fadvise().");
src/backend/access/transam/xlogrecovery.c:
GUC_check_errdetail("recovery_target_timeline is not a valid
number.");
src/backend/commands/variable.c:
GUC_check_errdetail("effective_io_concurrency must be set to 0 on
platforms that lack posix_fadvise().");
src/backend/commands/variable.c:
GUC_check_errdetail("maintenance_io_concurrency must be set to 0 on
platforms that lack posix_fadvise().");
src/backend/port/sysv_shmem.c:
GUC_check_errdetail("huge_page_size must be 0 on this platform.");
src/backend/port/win32_shmem.c:
GUC_check_errdetail("huge_page_size must be 0 on this platform.");
src/backend/replication/syncrep.c:
GUC_check_errdetail("synchronous_standby_names parser failed");
src/backend/storage/file/fd.c:
GUC_check_errdetail("debug_io_direct is not supported on this
platform.");
src/backend/storage/file/fd.c:
GUC_check_errdetail("debug_io_direct is not supported for WAL because
XLOG_BLCKSZ is too small");
src/backend/storage/file/fd.c:
GUC_check_errdetail("debug_io_direct is not supported for data because
BLCKSZ is too small");
src/backend/tcop/postgres.c:
GUC_check_errdetail("client_connection_check_interval must be set to 0
on this platform.");

~~~

below, the GUC name is embedded and double-quoted:

src/backend/commands/vacuum.c:
GUC_check_errdetail("\"vacuum_buffer_usage_limit\" must be 0 or
between %d kB and %d kB",
src/backend/commands/variable.c:
GUC_check_errdetail("Conflicting \"datestyle\" specifications.");
src/backend/storage/buffer/localbuf.c:
GUC_check_errdetail("\"temp_buffers\" cannot be changed after any
temporary tables have been accessed in the session.");
src/backend/tcop/postgres.c:
GUC_check_errdetail("\"max_stack_depth\" must not exceed %ldkB.",
src/backend/tcop/postgres.c:GUC_check_errdetail("Cannot enable
parameter when \"log_statement_stats\" is true.");
src/backend/tcop/postgres.c:GUC_check_errdetail("Cannot enable
\"log_statement_stats\" when "

~~~

below, the GUC name is substituted but not quoted:

src/backend/access/table/tableamapi.c:  GUC_check_errdetail("%s
cannot be empty.",
src/backend/access/table/tableamapi.c:  GUC_check_errdetail("%s is
too long (maximum %d characters).",

~~~

I had intended to make a patch to address the inconsistency, but
couldn't decide which of those styles was the preferred one.

Then I worried this could be the tip of the iceberg -- GUC names occur
in many other error messages where they are sometimes quoted and
sometimes not quoted:
e.g. Not quoted -- errhint("You might need to run fewer transactions
at a time or increase max_connections.")));
e.g. Quoted -- errmsg("\"max_wal_size\" must be at least twice
\"wal_segment_size\"")));

Ideally, they should all look the same everywhere, shouldn't they?

==
Kind Regards,
Peter Smith.
Fujitsu Australia

Re: Wrong function name in pgstatfuncs.c

[Daniel Gustafsson]

> On 1 Nov 2023, at 09:23, Kyotaro Horiguchi  wrote:
> 
> Since pgstatfuncs.c was refactored, the comments for synthesized
> function names are significant to find the function body.
> 
> I happened to find a misspelling among the function name
> comments. "pg_stat_get_mods_since_analyze" should be
> "pg_stat_get_mod_since_analyze".
> 
> Upon checking the file using a rudimentary script, I found no other
> similar mistakes in the same file.

Nice catch, that's indeed a tiny typo, will fix.

--
Daniel Gustafsson

Wrong function name in pgstatfuncs.c

[Kyotaro Horiguchi]

Since pgstatfuncs.c was refactored, the comments for synthesized
function names are significant to find the function body.

I happened to find a misspelling among the function name
comments. "pg_stat_get_mods_since_analyze" should be
"pg_stat_get_mod_since_analyze".

Upon checking the file using a rudimentary script, I found no other
similar mistakes in the same file.

(FWIW, I also feel that these macros might be going a bit too far by
synthesizing even the function names.)

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index 28ee97968b..1fb8b31863 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -78,7 +78,7 @@ PG_STAT_GET_RELENTRY_INT64(ins_since_vacuum)
 /* pg_stat_get_live_tuples */
 PG_STAT_GET_RELENTRY_INT64(live_tuples)
 
-/* pg_stat_get_mods_since_analyze */
+/* pg_stat_get_mod_since_analyze */
 PG_STAT_GET_RELENTRY_INT64(mod_since_analyze)
 
 /* pg_stat_get_numscans */

Re: Is this a problem in GenericXLogFinish()?

[Michael Paquier]

On Mon, Oct 30, 2023 at 03:54:39PM +0530, Amit Kapila wrote:
> On Sat, Oct 28, 2023 at 4:30 PM Michael Paquier  wrote:
>> On Sat, Oct 28, 2023 at 03:45:13PM +0530, Amit Kapila wrote:
>> > Yes, we need it to exclude any concurrent in-progress scans that could
>> > return incorrect tuples during bucket squeeze operation.
>>
>> Thanks.  So I assume that we should just set REGBUF_NO_CHANGE when the
>> primary and write buffers are the same and there are no tuples to
>> move.  Say with something like the attached?
> 
> What if the primary and write buffer are not the same but ntups is
> zero? Won't we hit the assertion again in that case?

The code tells that it should be able to handle such a case, so this
would mean to set REGBUF_NO_CHANGE only when we have no tuples to
move:
-XLogRegisterBuffer(1, wbuf, REGBUF_STANDARD);
+/*
+ * A cleanup lock is still required on the write buffer even
+ * if there are no tuples to move, so it needs to be registered
+ * in this case.
+ */
+wbuf_flags = REGBUF_STANDARD;
+if (xlrec.ntups == 0)
+wbuf_flags |= REGBUF_NO_CHANGE;
+XLogRegisterBuffer(1, wbuf, wbuf_flags);

Anyway, there is still a hole here: the regression tests have zero
coverage for the case where there are no tuples to move if
!is_prim_bucket_same_wrt.  There are only two queries that stress the
squeeze path with no tuples, both use is_prim_bucket_same_wrt:
INSERT INTO hash_split_heap SELECT a/2 FROM generate_series(1, 25000) a;
VACUUM hash_split_heap;

Perhaps this should have more coverage to make sure that all these
scenarios are covered at replay (likely with 027_stream_regress.pl)?
The case posted by Alexander at [1] falls under the same category
(is_prim_bucket_same_wrt with no tuples).

[1]: 
https://www.postgresql.org/message-id/f045c8f7-ee24-ead6-3679-c04a43d21...@gmail.com
--
Michael


signature.asc
Description: PGP signature

Tab completion regression test failed on illumos

[Japin Li]

Hi hackers,

I try to run regression test on illumos, the 010_tab_completion will
failed because of timeout.

Here is my build commands and logs:

$ ../configure --enable-cassert --enable-debug --enable-nls --with-perl \
  --with-python --with-tcl --with-openssl --with-libxml --with-libxslt \
  --without-icu --enable-tap-tests --prefix=/home/japin/postgres/build/pg
$ make -j $(nproc)
...
$ cd src/bin/psql/ && make check
make -C ../../../src/backend generated-headers
make[1]: Entering directory '/home/japin/postgres/build/src/backend'
make -C catalog distprep generated-header-symlinks
make[2]: Entering directory '/home/japin/postgres/build/src/backend/catalog'
make[2]: Nothing to be done for 'distprep'.
make[2]: Nothing to be done for 'generated-header-symlinks'.
make[2]: Leaving directory '/home/japin/postgres/build/src/backend/catalog'
make -C nodes distprep generated-header-symlinks
make[2]: Entering directory '/home/japin/postgres/build/src/backend/nodes'
make[2]: Nothing to be done for 'distprep'.
make[2]: Nothing to be done for 'generated-header-symlinks'.
make[2]: Leaving directory '/home/japin/postgres/build/src/backend/nodes'
make -C utils distprep generated-header-symlinks
make[2]: Entering directory '/home/japin/postgres/build/src/backend/utils'
make[2]: Nothing to be done for 'distprep'.
make[2]: Nothing to be done for 'generated-header-symlinks'.
make[2]: Leaving directory '/home/japin/postgres/build/src/backend/utils'
make[1]: Leaving directory '/home/japin/postgres/build/src/backend'
rm -rf '/home/japin/postgres/build'/tmp_install
/opt/local/bin/mkdir -p '/home/japin/postgres/build'/tmp_install/log
make -C '../../..' DESTDIR='/home/japin/postgres/build'/tmp_install install 
>'/home/japin/postgres/build'/tmp_install/log/install.log 2>&1
make -j1  checkprep 
>>'/home/japin/postgres/build'/tmp_install/log/install.log 2>&1

PATH="/home/japin/postgres/build/tmp_install/home/japin/postgres/build/pg/bin:/home/japin/postgres/build/src/bin/psql:$PATH"
 
LD_LIBRARY_PATH="/home/japin/postgres/build/tmp_install/home/japin/postgres/build/pg/lib"
 INITDB_TEMPLATE='/home/japin/postgres/build'/tmp_install/initdb-template  
initdb -A trust -N --no-instructions --no-locale 
'/home/japin/postgres/build'/tmp_install/initdb-template 
>>'/home/japin/postgres/build'/tmp_install/log/initdb-template.log 2>&1
echo "# +++ tap check in src/bin/psql +++" && rm -rf 
'/home/japin/postgres/build/src/bin/psql'/tmp_check && /opt/local/bin/mkdir -p 
'/home/japin/postgres/build/src/bin/psql'/tmp_check && cd 
/home/japin/postgres/build/../src/bin/psql && 
TESTLOGDIR='/home/japin/postgres/build/src/bin/psql/tmp_check/log' 
TESTDATADIR='/home/japin/postgres/build/src/bin/psql/tmp_check' 
PATH="/home/japin/postgres/build/tmp_install/home/japin/postgres/build/pg/bin:/home/japin/postgres/build/src/bin/psql:$PATH"
 
LD_LIBRARY_PATH="/home/japin/postgres/build/tmp_install/home/japin/postgres/build/pg/lib"
 INITDB_TEMPLATE='/home/japin/postgres/build'/tmp_install/initdb-template  
PGPORT='65432' top_builddir='/home/japin/postgres/build/src/bin/psql/../../..' 
PG_REGRESS='/home/japin/postgres/build/src/bin/psql/../../../src/test/regress/pg_regress'
 /opt/local/bin/prove -I /home/japin/postgres/build/../src/test/perl/ -I 
/home/japin/postgres/build/../src/bin/psql  t/*.pl
# +++ tap check in src/bin/psql +++
t/001_basic.pl ... ok
t/010_tab_completion.pl .. Dubious, test returned 25 (wstat 6400, 0x1900)
No subtests run
t/020_cancel.pl .. ok

Test Summary Report
---
t/010_tab_completion.pl (Wstat: 6400 Tests: 0 Failed: 0)
  Non-zero exit status: 25
  Parse errors: No plan found in TAP output

$ cat tmp_check/log/regress_log_010_tab_completion
# Checking port 59378
# Found port 59378
Name: main
Data directory: 
/home/japin/postgres/build/src/bin/psql/tmp_check/t_010_tab_completion_main_data/pgdata
Backup directory: 
/home/japin/postgres/build/src/bin/psql/tmp_check/t_010_tab_completion_main_data/backup
Archive directory: 
/home/japin/postgres/build/src/bin/psql/tmp_check/t_010_tab_completion_main_data/archives
Connection string: port=59378 host=/tmp/2tdG0Ck7Zb
Log file: 
/home/japin/postgres/build/src/bin/psql/tmp_check/log/010_tab_completion_main.log
[07:06:06.492](0.050s) # initializing database system by copying initdb 
template
# Running: cp -RPp /home/japin/postgres/build/tmp_install/initdb-template 
/home/japin/postgres/build/src/bin/psql/tmp_check/t_010_tab_completion_main_data/pgdata
# Running: 
/home/japin/postgres/build/src/bin/psql/../../../src/test/regress/pg_regress 
--config-auth 
/home/japin/postgres/build/src/bin/psql/tmp_check/t_010_tab_completion_main_data/pgdata
### Starting node "main"
# Running: pg_ctl -w -D 

Re: A recent message added to pg_upgade

[Peter Smith]

Hi, here are some minor review comments for the v3 patch.

==
src/backend/access/transam/xlog.c

1. check_max_slot_wal_keep_size

+/*
+ * GUC check_hook for max_slot_wal_keep_size
+ *
+ * If WALs required by logical replication slots are removed, the slots are
+ * unusable.  While pg_upgrade sets this variable to -1 via the command line to
+ * attempt to prevent such removal during binary upgrade, there are ways for
+ * users to override it. For the sake of completing the objective, ensure that
+ * this variable remains unchanged.  See InvalidatePossiblyObsoleteSlot() and
+ * start_postmaster() in pg_upgrade for more details.
+ */

I asked ChatGPT to suggest alternative wording for that comment, and
it came up with something that I felt was a slight improvement.

SUGGESTION
...
If WALs needed by logical replication slots are deleted, these slots
become inoperable. During a binary upgrade, pg_upgrade sets this
variable to -1 via the command line in an attempt to prevent such
deletions, but users have ways to override it. To ensure the
successful completion of the upgrade, it's essential to keep this
variable unaltered.
...

~~~

2.
+bool
+check_max_slot_wal_keep_size(int *newval, void **extra, GucSource source)
+{
+ if (IsBinaryUpgrade && *newval != -1)
+ {
+ GUC_check_errdetail("\"max_slot_wal_keep_size\" must be set to -1
during binary upgrade mode.");
+ return false;
+ }
+ return true;
+}

Some of the other GUC_check_errdetail()'s do not include the GUC name
in the translatable message text. Isn't that a preferred style?

SUGGESTION
GUC_check_errdetail("\"%s\" must be set to -1 during binary upgrade mode.",
  "max_slot_wal_keep_size");

==
src/backend/replication/slot.c

3. InvalidatePossiblyObsoleteSlot

- if (*invalidated && SlotIsLogical(s) && IsBinaryUpgrade)
- {
- ereport(ERROR,
- errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("replication slots must not be invalidated during the upgrade"),
- errhint("\"max_slot_wal_keep_size\" must be set to -1 during the upgrade"));
- }
+ Assert (!*invalidated || !SlotIsLogical(s) || !IsBinaryUpgrade);

IMO new Assert became trickier to understand than the original condition. YMMV.

SUGGESTION
Assert(!(*invalidated && SlotIsLogical(s) && IsBinaryUpgrade));

==
Kind Regards,
Peter Smith.
Fujitsu Australia

Re: More new SQL/JSON item methods

[Jeevan Chalke]

Hello,

On Tue, Oct 24, 2023 at 6:41 PM Andrew Dunstan  wrote:

>
> On 2023-10-19 Th 02:06, Jeevan Chalke wrote:
>
> Thanks, Peter for the comments.
>
> On Fri, Oct 6, 2023 at 5:13 PM Peter Eisentraut 
> wrote:
>
>> On 29.08.23 09:05, Jeevan Chalke wrote:
>> > v1-0001-Implement-jsonpath-.bigint-.integer-and-.number-m.patch
>> >
>> > This commit implements jsonpath .bigint(), .integer(), and .number()
>> > methods.  The JSON string or a numeric value is converted to the
>> > bigint, int4, and numeric type representation.
>>
>> A comment that applies to all of these: These add various keywords,
>> switch cases, documentation entries in some order.  Are we happy with
>> that?  Should we try to reorder all of that for better maintainability
>> or readability?
>>
>
> Yeah, that's the better suggestion. While implementing these methods, I
> was confused about where to put them exactly and tried keeping them in some
> logical place.
> I think once these methods get in, we can have a follow-up patch
> reorganizing all of these.
>
>
> I think it would be better to organize things how we want them before
> adding in more stuff.
>

I have tried reordering all the jsonpath Operators and Methods
consistently. With this patch, they all appear in the same order when
together in the group.

In some switch cases, they are still divided, like in
flattenJsonPathParseItem(), where 2-arg, 1-arg, and no-arg cases are
clubbed together. But I have tried to keep them in order in those subgroups.

I will rebase my patches for this task on this patch, but before doing so,
I  would like to get your views on this reordering.

Thanks


>
> cheers
>
>
> andrew
>
>
> --
> Andrew Dunstan
> EDB: https://www.enterprisedb.com
>
>

-- 
Jeevan Chalke

*Senior Staff SDE, Database Architect, and ManagerProduct Development*



edbpostgres.com


reorder-jsonpath-Operators-Methods.patch
Description: Binary data

Re: btree: implement dynamic prefix truncation (was: Improving btree performance through specializing by key shape, take 2)

[Pavel Stehule]

Hi

út 31. 10. 2023 v 22:12 odesílatel Matthias van de Meent <
boekewurm+postg...@gmail.com> napsal:

> Hi,
>
> Currently, nbtree code compares each and every column of an index
> tuple during the binary search on the index page. With large indexes
> that have many duplicate prefix column values (e.g. an index on (bool,
> bool, uuid) ) that means a lot of wasted time getting to the right
> column.
>
> The attached patch improves on that by doing per-page dynamic prefix
> truncation: If we know that on both the right and left side there are
> index tuples where the first two attributes are equal to the scan key,
> we skip comparing those attributes at the current index tuple and
> start with comparing attribute 3, saving two attribute compares. We
> gain performance whenever comparing prefixing attributes is expensive
> and when there are many tuples with a shared prefix - in unique
> indexes this doesn't gain much, but we also don't lose much in this
> case.
>
> This patch was originally suggested at [0], but it was mentioned that
> they could be pulled out into it's own thread. Earlier, the
> performance gains were not clearly there for just this patch, but
> after further benchmarking this patch stands on its own for
> performance: it sees no obvious degradation of performance, while
> gaining 0-5% across various normal indexes on the cc-complete sample
> dataset, with the current worst-case index shape getting a 60%+
> improved performance on INSERTs in the tests at [0].
>

+1

This can be nice functionality. I had a customer with a very slow index
scan - the main problem was a long common prefix like prg010203.

Regards

Pavel


> Kind regards,
>
> Matthias van de Meent
> Neon (https://neon.tech)
>
> PS. Best served with the downlink right separator/HIKEY optimization
> (separate patch to be submitted soon), and specialization over at [0].
>
> [0]
> https://www.postgresql.org/message-id/CAEze2WiqOONRQTUT1p_ZV19nyMA69UU2s0e2dp+jSBM=j8s...@mail.gmail.com
>

Commitfest manager November 2023

[John Naylor]

I didn't see any recent mentions in the archives, so I'll volunteer to
be CF manager for 2023-11.

Re: "38.10.10. Shared Memory and LWLocks" may require a clarification

[Michael Paquier]

On Tue, Oct 31, 2023 at 10:33:25AM +0900, Michael Paquier wrote:
> Sounds pretty much OK to me.  Thanks!

The main thing I have found annoying in the patch was the term
"tranche ID", so I have reworded that to use tranche_id to match with
the surroundings and the routines of lwlock.h.  LWLockInitialize()
should also be mentioned, as it is as important as the two others.

The result has been applied as fe705ef6fc1d.
--
Michael


signature.asc
Description: PGP signature