Re: SLRU optimization - configurable buffer pool and partitioning the SLRU lock

[Dilip Kumar]

On Sat, Oct 14, 2023 at 9:43 AM Amit Kapila  wrote:
>
> This and other results shared by you look promising. Will there be any
> improvement in workloads related to clog buffer usage?

I did not understand this question can you explain this a bit?  In
short, if it is regarding the performance then we will see it for all
the SLRUs as the control lock is not centralized anymore instead it is
a bank-wise lock.

 BTW, I remember
> that there was also a discussion of moving SLRU into a regular buffer
> pool [1]. You have not provided any explanation as to whether that
> approach will have any merits after we do this or whether that
> approach is not worth pursuing at all.
>
> [1] - https://commitfest.postgresql.org/43/3514/

Yeah, I haven't read that thread in detail about performance numbers
and all.  But both of these can not coexist because this patch is
improving the SLRU buffer pool access/configurable size and also lock
contention.  If we move SLRU to the main buffer pool then we might not
have a similar problem instead there might be other problems like SLRU
buffers getting swapped out due to other relation buffers and all and
OTOH the advantages of that approach would be that we can just use a
bigger buffer pool and SLRU can also take advantage of that.  But in
my opinion, most of the time we have limited page access in SLRU and
the SLRU buffer access pattern is also quite different from the
relation pages access pattern so keeping them under the same buffer
pool and comparing against relation pages for victim buffer selection
might cause different problems.  But anyway I would discuss those
points maybe in that thread.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

Re: Patch: Improve Boolean Predicate JSON Path Docs

[Erik Wienhold]

On 2023-10-20 05:20 +0200, David E. Wheeler wrote:
> On Oct 19, 2023, at 10:49 PM, Erik Wienhold  wrote:
> 
> > Just wanted to take a look at v5.  But it's an applefile again :P
> 
> I don’t get it. It was the other times too! Are you able to save it
> with a .patch suffix?

Saving it is not the problem, but the actual file contents:

$ xxd v5-0001-Improve-boolean-predicate-JSON-Path-docs.patch
: 0005 1600 0002       
0010:     0002  0009   
0020: 0032  000a  0003  003c   .2...<..
0030: 0036      7635 2d30  .6..v5-0
0040: 3030 312d 496d 7072 6f76 652d 626f 6f6c  001-Improve-bool
0050: 6561 6e2d 7072 6564 6963 6174 652d 4a53  ean-predicate-JS
0060: 4f4e 2d50 6174 682d 646f 6373 2e70 6174  ON-Path-docs.pat
0070: 6368 ch

I don't even know what that represents, probably not some fancy file
compression.

-- 
Erik

Re: Synchronizing slots from primary to standby

[shveta malik]

On Tue, Oct 17, 2023 at 9:06 PM Drouvot, Bertrand
 wrote:
>
> Hi,
>
> On 10/13/23 10:35 AM, shveta malik wrote:
> > On Thu, Oct 12, 2023 at 9:18 AM shveta malik  wrote:
> >>
> >
> > PFA v24 patch set which has below changes:
> >
> > 1) 'enable_failover' displayed in pg_replication_slots.
> > 2) Support for 'enable_failover' in
> > pg_create_logical_replication_slot(). It is an optional argument with
> > default value false.
> > 3) Addressed pending comments (1-30) from Peter in [1].
> > 4) Fixed an issue in patch002 due to which even slots with
> > enable_failover=false were getting synced.
> >
> > The changes for 1 and 2 are in patch001 while 3 and 4 are in patch0002
> >
> > Thanks Ajin, for working on 1 and 3.
>
> Thanks for the hard work!
>
> +   if (RecoveryInProgress())
> +   wrconn = slotsync_remote_connect(NULL);
>
> does produce at compilation time:
>
> launcher.c:1916:40: warning: too many arguments in call to 
> 'slotsync_remote_connect'
>  wrconn = slotsync_remote_connect(NULL);
>
> Looking at 0001:
>
> commit message:
>
> "is added at the slot level which
>  will be persistent information"
>
> what about "which is persistent information" ?
>
> Code:
>
> +   True if this logical slot is enabled to be synced to the physical 
> standbys
> +   so that logical replication is not blocked after failover. Always 
> false
> +   for physical slots.
>
> Not sure "not blocked" is the right wording. "can be resumed from the new 
> primary" maybe?
>
> +static void
> +ProcessRepliesAndTimeOut(void)
> +{
> +   CHECK_FOR_INTERRUPTS();
> +
> +   /* Process any requests or signals received recently */
> +   if (ConfigReloadPending)
> +   {
> +   ConfigReloadPending = false;
> +   ProcessConfigFile(PGC_SIGHUP);
> +   SyncRepInitConfig();
> +   SlotSyncInitConfig();
> +   }
>
> Do we want to do this at each place ProcessRepliesAndTimeOut() is being
> called? I mean before this change it was not done in ProcessPendingWrites().
>
> + * Wait for physical standby to confirm receiving give lsn.
>
> typo? s/give/given/
>
>
> diff --git a/src/test/recovery/t/050_verify_slot_order.pl 
> b/src/test/recovery/t/050_verify_slot_order.pl
> new file mode 100644
> index 00..25b3d5aac2
> --- /dev/null
> +++ b/src/test/recovery/t/050_verify_slot_order.pl
> @@ -0,0 +1,145 @@
> +
> +# Copyright (c) 2023, PostgreSQL Global Development Group
> +
>
> Regarding the TAP tests, should we also add some testing related to 
> enable_failover being set
> in pg_create_logical_replication_slot() and pg_logical_slot_get_changes() 
> behavior too?
>

We have added some basic tests in v25. More detailed tests to be added
in coming versions.

> Please note that current comments are coming while
> "quickly" going through 0001.
>
> I'm planning to have a closer look at 0001 and 0002 too.
>
> Regards,
>
> --
> Bertrand Drouvot
> PostgreSQL Contributors Team
> RDS Open Source Databases
> Amazon Web Services: https://aws.amazon.com

Re: Synchronizing slots from primary to standby

[shveta malik]

On Wed, Oct 18, 2023 at 4:24 PM Amit Kapila  wrote:
>
> On Tue, Oct 17, 2023 at 2:01 PM shveta malik  wrote:
> >
> > On Tue, Oct 17, 2023 at 12:44 PM Peter Smith  wrote:
> > >
> > > FYI - the latest patch failed to apply.
> > >
> > > [postgres@CentOS7-x64 oss_postgres_misc]$ git apply
> > > ../patches_misc/v24-0001-Allow-logical-walsenders-to-wait-for-the-physica.patch
> > > error: patch failed: src/include/utils/guc_hooks.h:160
> > > error: src/include/utils/guc_hooks.h: patch does not apply
> >
> > Rebased v24. PFA.
> >
>
> Few comments:
> ==
> 1.
> +List of physical replication slots that logical replication
> with failover
> +enabled waits for.
>
> /logical replication/logical replication slots
>
> 2.
>  If
> +enable_syncslot is not enabled on the
> +corresponding standbys, then it may result in indefinite waiting
> +on the primary for physical replication slots configured in
> +standby_slot_names
> +   
>
> Why the above leads to indefinite wait? I think we should just ignore
> standby_slot_names and probably LOG a message in the server for the
> same.
>

Sorry for confusion. This info was wrong, I have corrected it.

> 3.
> +++ b/src/backend/replication/logical/tablesync.c
> @@ -1412,7 +1412,8 @@ LogicalRepSyncTableStart(XLogRecPtr *origin_startpos)
>   */
>   walrcv_create_slot(LogRepWorkerWalRcvConn,
>  slotname, false /* permanent */ , false /* two_phase */ ,
> -CRS_USE_SNAPSHOT, origin_startpos);
> +false /* enable_failover */ , CRS_USE_SNAPSHOT,
> +origin_startpos);
>
> As per this code, we won't enable failover for tablesync slots. So,
> what happens if we need to failover to new node after the tablesync
> worker has reached SUBREL_STATE_FINISHEDCOPY or SUBREL_STATE_DATASYNC?
> I think we won't be able to continue replication from failed over
> node. If this theory is correct, we have two options (a) enable
> failover for sync slots as well, if it is enabled for main slot; but
> then after we drop the slot on primary once sync is complete, same
> needs to be taken care at standby. (b) enable failover even for the
> main slot after all tables are in ready state, something similar to
> what we do for two_phase.

I have adopted approach a) right now.  Table sync slot is created with
subscription's failover option and will be dropped by standby if it
dropped on primary.

>
> 4.
> + /* Verify syntax */
> + if (!validate_slot_names(newval, ))
> + return false;
> +
> + /* Now verify if these really exist and have correct type */
> + if (!validate_standby_slots(elemlist))
>
> These two functions serve quite similar functionality which makes
> their naming quite confusing. Can we directly move the functionality
> of validate_slot_names() into validate_standby_slots()?
>
> 5.
> +SlotSyncInitConfig(void)
> +{
> + char*rawname;
> +
> + /* Free the old one */
> + list_free(standby_slot_names_list);
> + standby_slot_names_list = NIL;
> +
> + if (strcmp(standby_slot_names, "") != 0)
> + {
> + rawname = pstrdup(standby_slot_names);
> + SplitIdentifierString(rawname, ',', _slot_names_list);
>
> How does this handle the case where '*' is specified for standby_slot_names?
>

I have removed '*' related doc info in this patch and has introduced
error if '*' is given for this GUC. The reason being, I do not see a
way to figure out all physical standbys slot names on a primary to
make '*' work. We have info about all the physical slots created on
primary but which all are actually being used by standbys is not
known. Thoughts?

thanks
Shveta

Re: [PoC] pg_upgrade: allow to upgrade publisher node

[vignesh C]

On Thu, 19 Oct 2023 at 16:16, Hayato Kuroda (Fujitsu)
 wrote:
>
> Dear Vignesh,
>
> Thanks for revieing! New patch can be available in [1].
>
> > Few comments:
> > 1) Even if we comment 3rd point "Emit a non-transactional message",
> > test_slot2 still appears in the invalid_logical_replication_slots.txt
> > file. There is something wrong here.
> > +   # 2. Advance the slot test_slot2 up to the current WAL location, but
> > +   #test_slot1 still has unconsumed WAL records.
> > +   $old_publisher->safe_psql('postgres',
> > +   "SELECT pg_replication_slot_advance('test_slot2', NULL);");
> > +
> > +   # 3. Emit a non-transactional message. test_slot2 detects the 
> > message
> > so
> > +   #that this slot will be also reported by upcoming 
> > pg_upgrade.
> > +   $old_publisher->safe_psql('postgres',
> > +   "SELECT count(*) FROM pg_logical_emit_message('false',
> > 'prefix', 'This is a non-transactional message');"
> > +   );
>
> The comment was updated based on others. How do you think?

I mean if we comment or remove this statement like in the attached
patch, the test is still passing with 'The slot "test_slot2" has not
consumed the WAL yet', in this case should the test_slot2 be still
invalid as we have called pg_replication_slot_advance for test_slot2.

Regards,
Vignesh
diff --git a/src/bin/pg_upgrade/t/003_upgrade_logical_replication_slots.pl b/src/bin/pg_upgrade/t/003_upgrade_logical_replication_slots.pl
index c6be7d5d9e..49720cc66d 100644
--- a/src/bin/pg_upgrade/t/003_upgrade_logical_replication_slots.pl
+++ b/src/bin/pg_upgrade/t/003_upgrade_logical_replication_slots.pl
@@ -74,7 +74,6 @@ sub test_upgrade_from_PG17_and_later
 		'postgres', qq[
 			CREATE TABLE tbl AS SELECT generate_series(1, 10) AS a;
 			SELECT pg_replication_slot_advance('test_slot2', NULL);
-			SELECT count(*) FROM pg_logical_emit_message('false', 'prefix', 'This is a non-transactional message');
 		]);
 	$old_publisher->stop;
 

Re: Patch: Improve Boolean Predicate JSON Path Docs

[David E. Wheeler]

On Oct 19, 2023, at 10:49 PM, Erik Wienhold  wrote:

> Just wanted to take a look at v5.  But it's an applefile again :P

I don’t get it. It was the other times too! Are you able to save it with a 
.patch suffix?

D

Re: [PoC] pg_upgrade: allow to upgrade publisher node

[vignesh C]

On Thu, 19 Oct 2023 at 16:14, Hayato Kuroda (Fujitsu)
 wrote:
>
> Dear Vignesh,
>
> Thanks for reviewing! New patch can be available in [1].
>
> >
> > Few comments:
> > 1) We will be able to override the value of max_slot_wal_keep_size by
> > using --new-options like '--new-options  "-c
> > max_slot_wal_keep_size=val"':
> > +   /*
> > +* Use max_slot_wal_keep_size as -1 to prevent the WAL removal by 
> > the
> > +* checkpointer process.  If WALs required by logical replication 
> > slots
> > +* are removed, the slots are unusable.  This setting prevents the
> > +* 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.
> > +*/
> > +   if (GET_MAJOR_VERSION(cluster->major_version) >= 1700)
> > +   appendPQExpBufferStr(, " -c
> > max_slot_wal_keep_size=-1");
> >
> > Should there be a check to throw an error if this option is specified
> > or do we need some documentation that this option should not be
> > specified?
>
> Hmm, I don't think we have to add checks. Other settings, like 
> synchronous_commit
> and fsync, can be also overwritten, but pg_upgrade has never checked. 
> Therefore,
> it's user's responsibility to not set max_slot_wal_keep_size to a dangerous
> value.
>
> > 2) Because we are able to override max_slot_wal_keep_size there is a
> > chance of slot getting invalidated and Assert being hit:
> > +   /*
> > +* 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.
> > +*/
> > +   if (*invalidated && SlotIsLogical(s) && IsBinaryUpgrade)
> > +   {
> > +   Assert(max_slot_wal_keep_size_mb == -1);
> > +   elog(ERROR, "replication slots must not be
> > invalidated during the upgrade");
> > +   }
>
> Hmm, so how about removing an assert and changing the error message more
> appropriate? I still think it seldom occurs.

As this scenario can occur by overriding max_slot_wal_keep_size, it is
better to remove the Assert.

Regards,
Vignesh

Re: Patch: Improve Boolean Predicate JSON Path Docs

[Erik Wienhold]

On 2023-10-19 15:39 +0200, David E. Wheeler wrote:
> On Oct 19, 2023, at 01:22, jian he  wrote:
> 
> Updated patch attached and also on GitHub.
> 
>   
> https://github.com/postgres/postgres/compare/master...theory:postgres:jsonpath-pred-docs

Just wanted to take a look at v5.  But it's an applefile again :P

-- 
Erik

Re: Remove last traces of HPPA support

[Thomas Munro]

On Fri, Oct 20, 2023 at 4:21 AM Tom Lane  wrote:
> We removed support for the HP-UX OS in v16, but left in support
> for the PA-RISC architecture, mainly because I thought that its
> spinlock mechanism is weird enough to be a good stress test
> for our spinlock infrastructure.  It still is that, but my
> one remaining HPPA machine has gone to the great recycle heap
> in the sky.  There seems little point in keeping around nominal
> support for an architecture that we can't test and no one is
> using anymore.
>
> Hence, the attached removes the remaining support for HPPA.

+1

Re: Guiding principle for dropping LLVM versions?

[Thomas Munro]

We could go further.  With LLVM 14 as the minimum we can just use
opaque pointers everywhere, and delete more conditional code in
master.  Tested on 14-18.

I explored using the new pass manager everywhere too.  It almost
worked, but I couldn't see how to override the inlining threshold
before LLVM 16[1], even in C++, so we couldn't fix that with a
llvmjit_wrap.cpp hack.

I like this.  How can I find out if someone would shout at me for
dropping LLVM 13?

[1] 
https://github.com/llvm/llvm-project/commit/4fa328074efd7eefdbb314b8f6e9f855e443ca20
From dd6005de803fb621cf1e21d9d7d31589e903e35b Mon Sep 17 00:00:00 2001
From: Thomas Munro 
Date: Thu, 19 Oct 2023 04:45:46 +1300
Subject: [PATCH v2 1/2] jit: Require at least LLVM 14, if enabled.

Remove support for a lot of older LLVM versions dating back to 3.9.  The
default on common software distritbutions will be at least LLVM 14 when
PostgreSQL 17 ships.

Discussion: https://postgr.es/m/CA%2BhUKGLhNs5geZaVNj2EJ79Dx9W8fyWUU3HxcpZy55sMGcY%3DiA%40mail.gmail.com
---
 config/llvm.m4  |   6 +-
 configure   |  39 +-
 doc/src/sgml/installation.sgml  |   4 +-
 meson.build |   2 +-
 src/backend/jit/llvm/llvmjit.c  | 158 +---
 src/backend/jit/llvm/llvmjit_error.cpp  |  35 --
 src/backend/jit/llvm/llvmjit_expr.c |   6 +-
 src/backend/jit/llvm/llvmjit_inline.cpp |  51 +---
 src/backend/jit/llvm/llvmjit_wrap.cpp   |  65 --
 src/include/jit/llvmjit.h   |  17 ---
 src/include/pg_config.h.in  |  12 --
 src/tools/msvc/Solution.pm  |   3 -
 12 files changed, 11 insertions(+), 387 deletions(-)

diff --git a/config/llvm.m4 b/config/llvm.m4
index 3a75cd8b4d..a99f29ffdc 100644
--- a/config/llvm.m4
+++ b/config/llvm.m4
@@ -25,8 +25,8 @@ AC_DEFUN([PGAC_LLVM_SUPPORT],
 AC_MSG_ERROR([$LLVM_CONFIG does not work])
   fi
   # and whether the version is supported
-  if echo $pgac_llvm_version | $AWK -F '.' '{ if ([$]1 >= 4 || ([$]1 == 3 && [$]2 >= 9)) exit 1; else exit 0;}';then
-AC_MSG_ERROR([$LLVM_CONFIG version is $pgac_llvm_version but at least 3.9 is required])
+  if echo $pgac_llvm_version | $AWK -F '.' '{ if ([$]1 >= 14) exit 1; else exit 0;}';then
+AC_MSG_ERROR([$LLVM_CONFIG version is $pgac_llvm_version but at least 14 is required])
   fi
 
   # need clang to create some bitcode files
@@ -114,8 +114,6 @@ AC_DEFUN([PGAC_CHECK_LLVM_FUNCTIONS],
   # Check which functionality is present
   SAVE_CPPFLAGS="$CPPFLAGS"
   CPPFLAGS="$CPPFLAGS $LLVM_CPPFLAGS"
-  AC_CHECK_DECLS([LLVMOrcGetSymbolAddressIn], [], [], [[#include ]])
-  AC_CHECK_DECLS([LLVMGetHostCPUName, LLVMGetHostCPUFeatures], [], [], [[#include ]])
   AC_CHECK_DECLS([LLVMCreateGDBRegistrationListener, LLVMCreatePerfJITEventListener], [], [], [[#include ]])
   CPPFLAGS="$SAVE_CPPFLAGS"
 ])# PGAC_CHECK_LLVM_FUNCTIONS
diff --git a/configure b/configure
index d47e0f8b26..f4a04485e1 100755
--- a/configure
+++ b/configure
@@ -5120,8 +5120,8 @@ fi
 as_fn_error $? "$LLVM_CONFIG does not work" "$LINENO" 5
   fi
   # and whether the version is supported
-  if echo $pgac_llvm_version | $AWK -F '.' '{ if ($1 >= 4 || ($1 == 3 && $2 >= 9)) exit 1; else exit 0;}';then
-as_fn_error $? "$LLVM_CONFIG version is $pgac_llvm_version but at least 3.9 is required" "$LINENO" 5
+  if echo $pgac_llvm_version | $AWK -F '.' '{ if ($1 >= 14) exit 1; else exit 0;}';then
+as_fn_error $? "$LLVM_CONFIG version is $pgac_llvm_version but at least 14 is required" "$LINENO" 5
   fi
 
   # need clang to create some bitcode files
@@ -16571,41 +16571,6 @@ if test "$with_llvm" = yes; then
   # Check which functionality is present
   SAVE_CPPFLAGS="$CPPFLAGS"
   CPPFLAGS="$CPPFLAGS $LLVM_CPPFLAGS"
-  ac_fn_c_check_decl "$LINENO" "LLVMOrcGetSymbolAddressIn" "ac_cv_have_decl_LLVMOrcGetSymbolAddressIn" "#include 
-"
-if test "x$ac_cv_have_decl_LLVMOrcGetSymbolAddressIn" = xyes; then :
-  ac_have_decl=1
-else
-  ac_have_decl=0
-fi
-
-cat >>confdefs.h <<_ACEOF
-#define HAVE_DECL_LLVMORCGETSYMBOLADDRESSIN $ac_have_decl
-_ACEOF
-
-  ac_fn_c_check_decl "$LINENO" "LLVMGetHostCPUName" "ac_cv_have_decl_LLVMGetHostCPUName" "#include 
-"
-if test "x$ac_cv_have_decl_LLVMGetHostCPUName" = xyes; then :
-  ac_have_decl=1
-else
-  ac_have_decl=0
-fi
-
-cat >>confdefs.h <<_ACEOF
-#define HAVE_DECL_LLVMGETHOSTCPUNAME $ac_have_decl
-_ACEOF
-ac_fn_c_check_decl "$LINENO" "LLVMGetHostCPUFeatures" "ac_cv_have_decl_LLVMGetHostCPUFeatures" "#include 
-"
-if test "x$ac_cv_have_decl_LLVMGetHostCPUFeatures" = xyes; then :
-  ac_have_decl=1
-else
-  ac_have_decl=0
-fi
-
-cat >>confdefs.h <<_ACEOF
-#define HAVE_DECL_LLVMGETHOSTCPUFEATURES $ac_have_decl
-_ACEOF
-
   ac_fn_c_check_decl "$LINENO" "LLVMCreateGDBRegistrationListener" "ac_cv_have_decl_LLVMCreateGDBRegistrationListener" "#include 
 "
 if test "x$ac_cv_have_decl_LLVMCreateGDBRegistrationListener" = xyes; then :
diff --git 

Re: Add the ability to limit the amount of memory that can be allocated to backends.

[Andrei Lepikhov]

On 20/10/2023 05:06, Stephen Frost wrote:

Greetings,

* Andrei Lepikhov (a.lepik...@postgrespro.ru) wrote:

On 19/10/2023 02:00, Stephen Frost wrote:

* Andrei Lepikhov (a.lepik...@postgrespro.ru) wrote:

On 29/9/2023 09:52, Andrei Lepikhov wrote:

On 22/5/2023 22:59, reid.thomp...@crunchydata.com wrote:

Attach patches updated to master.
Pulled from patch 2 back to patch 1 a change that was also pertinent
to patch 1.

+1 to the idea, have doubts on the implementation.

I have a question. I see the feature triggers ERROR on the exceeding of
the memory limit. The superior PG_CATCH() section will handle the error.
As I see, many such sections use memory allocations. What if some
routine, like the CopyErrorData(), exceeds the limit, too? In this case,
we could repeat the error until the top PG_CATCH(). Is this correct
behaviour? Maybe to check in the exceeds_max_total_bkend_mem() for
recursion and allow error handlers to slightly exceed this hard limit?



By the patch in attachment I try to show which sort of problems I'm worrying
about. In some PП_CATCH() sections we do CopyErrorData (allocate some
memory) before aborting the transaction. So, the allocation error can move
us out of this section before aborting. We await for soft ERROR message but
will face more hard consequences.


While it's an interesting idea to consider making exceptions to the
limit, and perhaps we'll do that (or have some kind of 'reserve' for
such cases), this isn't really any different than today, is it?  We
might have a malloc() failure in the main path, end up in PG_CATCH() and
then try to do a CopyErrorData() and have another malloc() failure.

If we can rearrange the code to make this less likely to happen, by
doing a bit more work to free() resources used in the main path before
trying to do new allocations, then, sure, let's go ahead and do that,
but that's independent from this effort.


I agree that rearranging efforts can be made independently. The code in the
letter above was shown just as a demo of the case I'm worried about.
IMO, the thing that should be implemented here is a recursion level for the
memory limit. If processing the error, we fall into recursion with this
limit - we should ignore it.
I imagine custom extensions that use PG_CATCH() and allocate some data
there. At least we can raise the level of error to FATAL.


Ignoring such would defeat much of the point of this effort- which is to
get to a position where we can say with some confidence that we're not
going to go over some limit that the user has set and therefore not
allow ourselves to end up getting OOM killed.  These are all the same
issues that already exist today on systems which don't allow overcommit
too, there isn't anything new here in regards to these risks, so I'm not
really keen to complicate this to deal with issues that are already
there.

Perhaps once we've got the basics in place then we could consider
reserving some space for handling such cases..  but I don't think it'll
actually be very clean and what if we have an allocation that goes
beyond what that reserved space is anyway?  Then we're in the same spot
again where we have the choice of either failing the allocation in a
less elegant way than we might like to handle that error, or risk
getting outright kill'd by the kernel.  Of those choices, sure seems
like failing the allocation is the better way to go.


I've got your point.
The only issue I worry about is the uncertainty and clutter that can be 
created by this feature. In the worst case, when we have a complex error 
stack (including the extension's CATCH sections, exceptions in stored 
procedures, etc.), the backend will throw the memory limit error 
repeatedly. Of course, one failed backend looks better than a 
surprisingly killed postmaster, but the mix of different error reports 
and details looks terrible and challenging to debug in the case of 
trouble. So, may we throw a FATAL error if we reach this limit while 
handling an exception?


--
regards,
Andrey Lepikhov
Postgres Professional

Re: [PoC/RFC] Multiple passwords, interval expirations

[Jeff Davis]

On Wed, 2023-10-18 at 14:48 -0400, Stephen Frost wrote:
> Right, we need more observability, agreed, but that's not strictly
> necessary of this patch and could certainly be added independently. 
> Is
> there really a need to make this observability a requirement of this
> particular change?

I won't draw a line in the sand, but it feels like something should be
there to help the user keep track of which password they might want to
keep. At least a "created on" date or something.

> > (Aside: is the uniqueness of the salt enforced in the current
> > patch?)
> 
> Err, the salt has to be *identical* for each password of a given
> user,
> not unique, so I'm a bit confused here.

Sorry, my mistake.

If the client needs to use the same salt as existing passwords, can you
still use PQencryptPasswordConn() on the client to avoid sending the
plaintext password to the server?

Regards,
Jeff Davis

Re: Fix output of zero privileges in psql

[Erik Wienhold]

On 2023-10-17 04:05 +0200, David G. Johnston wrote:
> Erik seems to favors (none)

Yes, with a slight favor for "(none)" because it's the least disruptive
to users who change \pset null to a non-blank string.  The output of \dp
etc. would still look the same for default privileges.

But I'm also okay with respecting \pset null because it is so simple.
We will no longer hide the already documented null representation of
default privileges by allowing the user to display the ACL as it is.
And with Laurenz' patch we will also document the special case of zero
privileges and how to distinguish it.

-- 
Erik

Re: Faster "SET search_path"

[Jeff Davis]

On Fri, 2023-09-15 at 11:31 -0700, Jeff Davis wrote:
> On Tue, 2023-09-12 at 13:55 -0700, Jeff Davis wrote:
> > On Mon, 2023-08-07 at 15:39 -0700, Nathan Bossart wrote:
> > > 0003 is looking pretty good, too, but I think we
> > > should get some more eyes on it, given the complexity.
> > 
> > Attached rebased version of 0003.
> 
> Is someone else planning to look at 0003, or should I just proceed?
> It
> seems to be clearly wanted, and I think it's better to get it in this
> 'fest than to wait.

Attaching a new version, as well as some additional optimizations.

Changes:

* I separated it into more small functions, and generally refactored
quite a bit trying to make it easier to review.

* The new version is more careful about what might happen during an
OOM, or in weird cases such as a huge number of distinct search_path
strings.

0003: Cache for recomputeNamespacePath.
0004: Use the same cache to optimize check_search_path().
0005: Optimize cache for repeated lookups of the same value.

Applying the same tests as described in the first message[1], the new
numbers are:

  baseline:   4.4s
  test query:
without patch:   12.3s
0003: 8.8s
0003,0004:7.4s
0003,0004,0005:   7.0s

This brings the slowdown from 180% on master down to about 60%. Still
not where we want to be exactly, but more tolerable.

The profile shows a few more areas worth looking at, so I suppose a bit
more effort could bring it down further. find_option(), for instance,
is annoyingly slow because it does case folding.

Regards,
Jeff Davis

[1]
https://www.postgresql.org/message-id/04c8592dbd694e4114a3ed87139a7a04e4363030.camel%40j-davis.com
From 955cce952e8fbc8d117d4df1876e56feaec1d944 Mon Sep 17 00:00:00 2001
From: Jeff Davis 
Date: Wed, 26 Jul 2023 12:55:09 -0700
Subject: [PATCH v7 3/6] Add cache for recomputeNamespacePath().

When search_path is changed to something that was previously set, and
no invalidation happened in between, use the cached list of namespace
OIDs rather than recomputing them. This avoids syscache lookups and
ACL checks.

Important when the search_path changes frequently, such as when set in
proconfig.

Discussion: https://postgr.es/m/04c8592dbd694e4114a3ed87139a7a04e4363030.camel%40j-davis.com
---
 src/backend/catalog/namespace.c | 440 ++--
 1 file changed, 365 insertions(+), 75 deletions(-)

diff --git a/src/backend/catalog/namespace.c b/src/backend/catalog/namespace.c
index ff1bfae1a3..258c97b590 100644
--- a/src/backend/catalog/namespace.c
+++ b/src/backend/catalog/namespace.c
@@ -41,6 +41,7 @@
 #include "catalog/pg_ts_template.h"
 #include "catalog/pg_type.h"
 #include "commands/dbcommands.h"
+#include "common/hashfn.h"
 #include "funcapi.h"
 #include "mb/pg_wchar.h"
 #include "miscadmin.h"
@@ -109,11 +110,12 @@
  * activeSearchPath is always the actually active path; it points to
  * to baseSearchPath which is the list derived from namespace_search_path.
  *
- * If baseSearchPathValid is false, then baseSearchPath (and other
- * derived variables) need to be recomputed from namespace_search_path.
- * We mark it invalid upon an assignment to namespace_search_path or receipt
- * of a syscache invalidation event for pg_namespace.  The recomputation
- * is done during the next lookup attempt.
+ * If baseSearchPathValid is false, then baseSearchPath (and other derived
+ * variables) need to be recomputed from namespace_search_path, or retrieved
+ * from the search path cache if there haven't been any syscache
+ * invalidations.  We mark it invalid upon an assignment to
+ * namespace_search_path or receipt of a syscache invalidation event for
+ * pg_namespace.  The recomputation is done during the next lookup attempt.
  *
  * Any namespaces mentioned in namespace_search_path that are not readable
  * by the current user ID are simply left out of baseSearchPath; so
@@ -153,6 +155,27 @@ static Oid	namespaceUser = InvalidOid;
 
 /* The above four values are valid only if baseSearchPathValid */
 static bool baseSearchPathValid = true;
+static bool searchPathCacheValid = false;
+static MemoryContext SearchPathCacheContext = NULL;
+
+typedef struct SearchPathCacheKey
+{
+	const char	*searchPath;
+	Oid			 roleid;
+} SearchPathCacheKey;
+
+typedef struct SearchPathCacheEntry
+{
+	SearchPathCacheKey	 key;
+	List*oidlist;	/* namespace OIDs that pass ACL checks */
+	List*finalPath;	/* cached final computed search path */
+	Oid	 firstNS;	/* first explicitly-listed namespace */
+	bool temp_missing;
+	bool forceRecompute; /* force recompute of finalPath */
+
+	/* needed for simplehash */
+	char status;
+} SearchPathCacheEntry;
 
 /*
  * myTempNamespace is InvalidOid until and unless a TEMP namespace is set up
@@ -206,6 +229,143 @@ static bool MatchNamedCall(HeapTuple proctup, int nargs, List *argnames,
 		   bool include_out_arguments, int pronargs,
 		   int **argnumbers);
 
+/*
+ * Recomputing the 

Re: [PoC] pg_upgrade: allow to upgrade publisher node

[Peter Smith]

Here are some review comments for v54-0001

==
src/backend/replication/slot.c

1.
+ 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 not be set to -1 during the
upgrade"));
+ }

This new error is replacing the old code:
+ Assert(max_slot_wal_keep_size_mb == -1);

Is that errhint correct? Shouldn't it say "must" instead of "must not"?

==
src/bin/pg_upgrade/t/003_upgrade_logical_replication_slots.pl

2. General formating

Some of the "]);" formatting and indenting for the multiple SQL
commands is inconsistent.

For example,

+ $old_publisher->safe_psql(
+ 'postgres', qq[
+ SELECT pg_create_logical_replication_slot('test_slot1', 'test_decoding');
+ SELECT pg_create_logical_replication_slot('test_slot2', 'test_decoding');
+ ]
+ );

versus

+ $old_publisher->safe_psql(
+ 'postgres', qq[
+ CREATE TABLE tbl AS SELECT generate_series(1, 10) AS a;
+ SELECT pg_replication_slot_advance('test_slot2', NULL);
+ SELECT count(*) FROM pg_logical_emit_message('false', 'prefix',
'This is a non-transactional message');
+ ]);

~~~

3.
+# Set up some settings for the old cluster, so that we can ensures that initdb
+# will be done.
+my @initdb_params = ();
+push @initdb_params, ('--encoding', 'UTF-8');
+push @initdb_params, ('--locale', 'C');
+$node_params{extra} = \@initdb_params;
+
+$old_publisher->init(%node_params);

Why would initdb not be done if these were not set? I didn't
understand the comment.

/so that we can ensures/to ensure/

~~~

4.
+# XXX: For PG9.6 and prior, the TAP Cluster.pm assigns 'max_wal_senders' and
+# 'max_connections' to the same value (10). But these versions considered
+# max_wal_senders as a subset of max_connections, so setting the same value
+# will fail. This adjustment will not be needed when packages for older
+#versions are defined.
+if ($old_publisher->pg_version->major <= 9.6)
+{
+ $old_publisher->append_conf(
+ 'postgresql.conf', qq[
+ max_wal_senders = 5
+ max_connections = 10
+ ]);
+}

4a.
IMO remove the complicated comment trying to explain the problem and
just to unconditionally set the values you want.

SUGGESTION#1
# Older PG version had different rules for the inter-dependency of
'max_wal_senders' and 'max_connections',
# so assign values which will work for all PG versions.
$old_publisher->append_conf(
  'postgresql.conf', qq[
  max_wal_senders = 5
  max_connections = 10
  ]);

~~

4b.
If you really want to put special code here then I think the comment
needs to be more descriptive like below. IMO this suggestion is
overkill, #4a above is much simpler.

SUGGESTION#2
# Versions prior to PG12 considered max_walsenders as a subset
max_connections, so setting the same value will fail.
#
# The TAP Cluster.pm assigns default 'max_wal_senders' and
'max_connections' as follows:
# PG_11:  'max_wal_senders=5' and 'max_connections=10'
# PG_10:  'max_wal_senders=5' and 'max_connections=10'
# Everything else: 'max_wal_senders=10' and 'max_connections=10'
#
# The following code is needed to make adjustments for versions not
already being handled by Cluster.pm.

~

4c.
Alternatively, make necessary adjustments in the Cluster.pm to set
appropriate defaults for all older versions. Then probably you can
remove all this code entirely.

==
Kind Regards,
Peter Smith.
Fujitsu Australia

Re: Remove last traces of HPPA support

[Tom Lane]

Noah Misch  writes:
> On Thu, Oct 19, 2023 at 11:16:28AM -0400, Tom Lane wrote:
>> Hence, the attached removes the remaining support for HPPA.

> I wouldn't do this.  NetBSD/hppa still claims to exist, as does the OpenBSD
> equivalent.  I presume its pkgsrc compiles this code.  The code is basically
> zero-maintenance, so there's not much to gain from deleting it preemptively.

I doubt it: I don't think anyone is routinely building very much of
pkgsrc for backwater hardware like HPPA, on either distro.  It takes
too much time (as cross-build doesn't work IME) and there are too few
potential users.  I certainly had to build all my own packages during
my experiments with running those systems on my machine.

Moreover, if they are compiling it they aren't testing it.
I filed a pile of bugs against NetBSD kernel and toolchains
on the way to getting the late lamented chickadee animal running.
While it was pretty much working when I retired chickadee, it was
obviously ground that nobody else had trodden in a long time.

As for OpenBSD, while I did have a working installation of 6.4
at one time, I completely failed to get 7.1 running on that
hardware.  I think it's maintained only for very small values
of "maintained".

Lastly, even when they're working those systems are about half
the speed of HP-UX on the same hardware; and even when using HP-UX
there is no HPPA hardware that's not insanely slow by modern
standards.  I can't believe that anyone would want to run modern
PG on that stack, and I don't believe that anyone but me has
tried in a long time.

regards, tom lane

Re: Remove wal_level settings for subscribers in tap tests

[Michael Paquier]

On Wed, Oct 18, 2023 at 03:39:16PM +0900, Michael Paquier wrote:
> Hmm, okay.  On top of your argument, this may be a good idea for a
> different reason: it makes the tests a bit cheaper as "logical"
> generates a bit more WAL.  Still the gain is marginal. 

And applied this one.
--
Michael


signature.asc
Description: PGP signature

Re: Remove last traces of HPPA support

[Noah Misch]

On Thu, Oct 19, 2023 at 11:16:28AM -0400, Tom Lane wrote:
> We removed support for the HP-UX OS in v16, but left in support
> for the PA-RISC architecture, mainly because I thought that its
> spinlock mechanism is weird enough to be a good stress test
> for our spinlock infrastructure.  It still is that, but my
> one remaining HPPA machine has gone to the great recycle heap
> in the sky.  There seems little point in keeping around nominal
> support for an architecture that we can't test and no one is
> using anymore.
> 
> Hence, the attached removes the remaining support for HPPA.
> Any objections?

I wouldn't do this.  NetBSD/hppa still claims to exist, as does the OpenBSD
equivalent.  I presume its pkgsrc compiles this code.  The code is basically
zero-maintenance, so there's not much to gain from deleting it preemptively.

Re: list of acknowledgments for PG16

[Zhang Mingli]

Hi,

> On Aug 22, 2023, at 17:33, Peter Eisentraut  wrote:
> 
> The list of acknowledgments for the PG16 release notes has been committed.  
> It should show up here sometime: 
> .
>   As usual, please check for problems such as wrong sorting, duplicate names 
> in different variants, or names in the wrong order etc.  (Our convention is 
> given name followed by surname.)
> 
> 

Could you help me with Mingli Zhang ->  Zhang Mingli

Thanks.

Zhang Mingli
HashData https://www.hashdata.xyz

Re: Remove last traces of HPPA support

[Tom Lane]

Michael Paquier  writes:
> On Thu, Oct 19, 2023 at 11:16:28AM -0400, Tom Lane wrote:
>> Hence, the attached removes the remaining support for HPPA.
>> Any objections?

> Would a refresh of config/config.guess and config/config.sub be
> suited?  This stuff still has references to HPPA.

AFAIK we just absorb those files verbatim from upstream.  There is plenty
of stuff in them for systems we don't support; it's not worth trying
to clean that out.

regards, tom lane

Re: Remove last traces of HPPA support

[Michael Paquier]

On Thu, Oct 19, 2023 at 11:16:28AM -0400, Tom Lane wrote:
> We removed support for the HP-UX OS in v16, but left in support
> for the PA-RISC architecture, mainly because I thought that its
> spinlock mechanism is weird enough to be a good stress test
> for our spinlock infrastructure.  It still is that, but my
> one remaining HPPA machine has gone to the great recycle heap
> in the sky.  There seems little point in keeping around nominal
> support for an architecture that we can't test and no one is
> using anymore.

Looks OK for the C parts.

> Hence, the attached removes the remaining support for HPPA.
> Any objections?

Would a refresh of config/config.guess and config/config.sub be
suited?  This stuff still has references to HPPA.
--
Michael


signature.asc
Description: PGP signature

Re: [patch] pg_basebackup: mention that spread checkpoints are the default in --help

[Michael Paquier]

On Thu, Oct 19, 2023 at 10:30:04PM +0200, Michael Banck wrote:
> Hrm right, but those have multiple options and they do not enumerate
> them in the help string as do -F and -c - not sure what general project
> policy here is for mentioning defaults in --help, I will check some of
> the other commands.

Then comes the point that this bloats the --help output.  A bunch of
system commands I use on a daily-basis outside Postgres don't do that,
so it's kind of hard to put a line on what's good or not in this area
while we have the SGML and man pages to do the job, with always more
details.
--
Michael


signature.asc
Description: PGP signature

Re: Add the ability to limit the amount of memory that can be allocated to backends.

[Stephen Frost]

Greetings,

* Andres Freund (and...@anarazel.de) wrote:
> On 2023-10-19 18:06:10 -0400, Stephen Frost wrote:
> > Ignoring such would defeat much of the point of this effort- which is to
> > get to a position where we can say with some confidence that we're not
> > going to go over some limit that the user has set and therefore not
> > allow ourselves to end up getting OOM killed.
> 
> I think that is a good medium to long term goal. I do however think that we'd
> be better off merging the visibility of memory allocations soon-ish and
> implement the limiting later. There's a lot of hairy details to get right for
> the latter, and even just having visibility will be a huge improvement.

I agree that having the visibility will be a great improvement and
perhaps could go in separately, but I don't know that I agree that the
limits are going to be that much of an issue.  In any case, there's been
work ongoing on this and that'll be posted soon.  I was just trying to
address the general comment raised in this sub-thread here.

> I think even patch 1 is doing too much at once. I doubt the DSM stuff is
> quite right.

Getting DSM right has certainly been tricky, along with other things,
but we've been working towards, and continue to work towards, getting
everything to line up nicely between memory context allocations of
various types and the amounts which are being seen as malloc'd/free'd.
There's been parts of this also reworked to allow us to see per-backend
reservations as well as total reserved and to get those numbers able to
be matched up inside of a given transaction using the statistics system.

> I'm unconvinced it's a good idea to split the different types of memory
> contexts out. That just exposes too much implementation detail stuff without a
> good reason.

DSM needs to be independent anyway ... as for the others, perhaps we
could combine them, though that's pretty easily done later and for now
it's been useful to see them split out as we've been working on the
patch.

> I think the overhead even just the tracking implies right now is likely too
> high and needs to be optimized. It should be a single math operation, not
> tracking things in multiple fields. I don't think pg_sub_u64_overflow() should
> be in the path either, that suddenly adds conditional branches.  You really
> ought to look at the difference in assembly for the hot functions.

This has been improved in the most recent work and we'll have that
posted soon, probably best to hold off from larger review of this right
now- as mentioned, I was just trying to address the specific question in
this sub-thread since a new patch is coming soon.

Thanks,

Stephen


signature.asc
Description: PGP signature

Re: Add the ability to limit the amount of memory that can be allocated to backends.

[Andres Freund]

Hi,

On 2023-10-19 18:06:10 -0400, Stephen Frost wrote:
> Ignoring such would defeat much of the point of this effort- which is to
> get to a position where we can say with some confidence that we're not
> going to go over some limit that the user has set and therefore not
> allow ourselves to end up getting OOM killed.

I think that is a good medium to long term goal. I do however think that we'd
be better off merging the visibility of memory allocations soon-ish and
implement the limiting later. There's a lot of hairy details to get right for
the latter, and even just having visibility will be a huge improvement.

I think even patch 1 is doing too much at once. I doubt the DSM stuff is
quite right.

I'm unconvinced it's a good idea to split the different types of memory
contexts out. That just exposes too much implementation detail stuff without a
good reason.

I think the overhead even just the tracking implies right now is likely too
high and needs to be optimized. It should be a single math operation, not
tracking things in multiple fields. I don't think pg_sub_u64_overflow() should
be in the path either, that suddenly adds conditional branches.  You really
ought to look at the difference in assembly for the hot functions.

Greetings,

Andres Freund

Re: controlling meson's parallelism (and some whining)

[Andres Freund]

Hi,

On 2023-10-19 13:44:20 -0400, Robert Haas wrote:
> Twice now, I've had 'meson test' fail because it tried to start too
> many copies of the server at the same time. In the server log, I get
> the complaint about needing to raise SHMMNI. This is a macos machine,
> with kern.sysv.shmmni=32.

Hm. Did you not run into simmilar issues with make check-world? I found the
concurrency of that to be even more variable over a run.


But perhaps there's something else wrong here? Perhaps we should deal with
this in Cluster.pm to some degree? Controlling this from the level of meson
(or make/prove for that matter) doesn't really work well, because different
tests start differently many postgres instances.

How many cores does your machine have? I've run the tests in a loop on my m1
mac mini in the past without running into this issue. It has "only" 8 cores
though, whereas I infer, from you mentioning -j8, that you have more cores?


> The obvious fix to this is to just tell 'meson test' how many processes I'd
> like it to run. I thought maybe I could just do 'meson -j8 test' but that
> does not work, because the option is --num-processes and has no short
> version. Even typing -j8 every time would be kind of annoying; typing
> --num-processes 8 every time is ridiculously verbose.

I've also wondered why there's no support for -j, maybe we should open an
issue...


> My next thought was that there ought to be some environmental variable
> that I could set to control this behavior. But I can't find a list of
> environment variables that affect meson behavior anywhere. I guess the
> authors don't believe in environment variable as a control mechanism.

They indeed do not like them - but there is one in this
case: MESON_TESTTHREADS

There's even documentation for it: 
https://mesonbuild.com/Unit-tests.html#parallelism


> Or, at the risk of sounding a bit testy, maybe their documentation
> just isn't quite up to par. It's not that hard to find lists of
> options for particular subcommands, either from the tool itself or on
> the web site. But unlike git, where you can do something like 'man
> git-checkout' and actually get more information than the command help
> itself provides, there are no man pages for the main subcommands, and
> I can't really find any good documentation on the web site either.
> Knowing that a certain subcommand has a flag called
> --pkgconfig.relocatable or that some other command has a flag called
> --cross-file CROSS_FILE whose argument is, and I quote, a "File
> describing cross compilation environment," is not good enough.

I agree that meson's documentation is of, let's say, varying quality. But
https://mesonbuild.com/Commands.html#test does link to
https://mesonbuild.com/Unit-tests.html which in turn has the bit about
MESON_TESTTHREADS

I do agree that it'd be nice if the online docs were converted to command
specific manpages...

Greetings,

Andres Freund

Re: boolin comment not moved when code was refactored

[Peter Smith]

On Fri, Oct 20, 2023 at 2:31 AM Tom Lane  wrote:
>
> Peter Smith  writes:
> > PSA v2.
>
> Pushed.
>

Thanks for pushing.

==
Kind Regards,
Peter Smith.
Fujitsu Australia

Re: Add the ability to limit the amount of memory that can be allocated to backends.

[Stephen Frost]

Greetings,

* Andrei Lepikhov (a.lepik...@postgrespro.ru) wrote:
> On 19/10/2023 02:00, Stephen Frost wrote:
> > * Andrei Lepikhov (a.lepik...@postgrespro.ru) wrote:
> > > On 29/9/2023 09:52, Andrei Lepikhov wrote:
> > > > On 22/5/2023 22:59, reid.thomp...@crunchydata.com wrote:
> > > > > Attach patches updated to master.
> > > > > Pulled from patch 2 back to patch 1 a change that was also pertinent
> > > > > to patch 1.
> > > > +1 to the idea, have doubts on the implementation.
> > > > 
> > > > I have a question. I see the feature triggers ERROR on the exceeding of
> > > > the memory limit. The superior PG_CATCH() section will handle the error.
> > > > As I see, many such sections use memory allocations. What if some
> > > > routine, like the CopyErrorData(), exceeds the limit, too? In this case,
> > > > we could repeat the error until the top PG_CATCH(). Is this correct
> > > > behaviour? Maybe to check in the exceeds_max_total_bkend_mem() for
> > > > recursion and allow error handlers to slightly exceed this hard limit?
> > 
> > > By the patch in attachment I try to show which sort of problems I'm 
> > > worrying
> > > about. In some PП_CATCH() sections we do CopyErrorData (allocate some
> > > memory) before aborting the transaction. So, the allocation error can move
> > > us out of this section before aborting. We await for soft ERROR message 
> > > but
> > > will face more hard consequences.
> > 
> > While it's an interesting idea to consider making exceptions to the
> > limit, and perhaps we'll do that (or have some kind of 'reserve' for
> > such cases), this isn't really any different than today, is it?  We
> > might have a malloc() failure in the main path, end up in PG_CATCH() and
> > then try to do a CopyErrorData() and have another malloc() failure.
> > 
> > If we can rearrange the code to make this less likely to happen, by
> > doing a bit more work to free() resources used in the main path before
> > trying to do new allocations, then, sure, let's go ahead and do that,
> > but that's independent from this effort.
> 
> I agree that rearranging efforts can be made independently. The code in the
> letter above was shown just as a demo of the case I'm worried about.
> IMO, the thing that should be implemented here is a recursion level for the
> memory limit. If processing the error, we fall into recursion with this
> limit - we should ignore it.
> I imagine custom extensions that use PG_CATCH() and allocate some data
> there. At least we can raise the level of error to FATAL.

Ignoring such would defeat much of the point of this effort- which is to
get to a position where we can say with some confidence that we're not
going to go over some limit that the user has set and therefore not
allow ourselves to end up getting OOM killed.  These are all the same
issues that already exist today on systems which don't allow overcommit
too, there isn't anything new here in regards to these risks, so I'm not
really keen to complicate this to deal with issues that are already
there.

Perhaps once we've got the basics in place then we could consider
reserving some space for handling such cases..  but I don't think it'll
actually be very clean and what if we have an allocation that goes
beyond what that reserved space is anyway?  Then we're in the same spot
again where we have the choice of either failing the allocation in a
less elegant way than we might like to handle that error, or risk
getting outright kill'd by the kernel.  Of those choices, sure seems
like failing the allocation is the better way to go.

Thanks,

Stephen


signature.asc
Description: PGP signature

Re: Parent/child context relation in pg_get_backend_memory_contexts()

[Stephen Frost]

Greetings,

* Andres Freund (and...@anarazel.de) wrote:
> On 2023-10-18 15:53:30 -0400, Stephen Frost wrote:
> > > Here how pg_backend_memory_contexts would look like with this patch:
> > > 
> > > postgres=# SELECT name, id, parent, parent_id, path
> > > FROM pg_backend_memory_contexts
> > > ORDER BY total_bytes DESC LIMIT 10;
> > >   name   | id  |  parent  | parent_id | path
> > > -+-+--+---+--
> > >  CacheMemoryContext  |  27 | TopMemoryContext | 0 | {0}
> > >  Timezones   | 124 | TopMemoryContext | 0 | {0}
> > >  TopMemoryContext|   0 |  |   |
> > >  MessageContext  |   8 | TopMemoryContext | 0 | {0}
> > >  WAL record construction | 118 | TopMemoryContext | 0 | {0}
> > >  ExecutorState   |  18 | PortalContext|17 | {0,16,17}
> > >  TupleSort main  |  19 | ExecutorState|18 | 
> > > {0,16,17,18}
> > >  TransactionAbortContext |  14 | TopMemoryContext | 0 | {0}
> > >  smgr relation table |  10 | TopMemoryContext | 0 | {0}
> > >  GUC hash table  | 123 | GUCMemoryContext |   122 | {0,122}
> > > (10 rows)
> > > 
> > > An example query to calculate the total_bytes including its children for a
> > > context (say CacheMemoryContext) would look like this:
> > > 
> > > WITH contexts AS (
> > > SELECT * FROM pg_backend_memory_contexts
> > > )
> > > SELECT sum(total_bytes)
> > > FROM contexts
> > > WHERE ARRAY[(SELECT id FROM contexts WHERE name = 'CacheMemoryContext')] 
> > > <@
> > > path;
> > 
> > I wonder if we should perhaps just include
> > "total_bytes_including_children" as another column?  Certainly seems
> > like a very useful thing that folks would like to see.
> 
> The "issue" is where to stop - should we also add that for some of the other
> columns? They are a bit less important, but not that much.

I'm not sure the others really make sense to aggregate in this way as
free space isn't able to be moved between contexts.  That said, if
someone wants it then I'm not against that.  I'm actively in support of
adding an aggregated total though as that, at least to me, seems to be
very useful to have.

Thanks,

Stephen


signature.asc
Description: PGP signature

Re: prevent non-superuser terminate bgworker running as superuser

[Jelte Fennema]

This seems like it should even be considered a security honestly.

On Thu, 19 Oct 2023, 19:49 Hemanth Sandrana, 
wrote:

> Hi All,
>
> Currently, BackgroundWorker connected to a database by calling
> BackgroundWorkerInitializeConnection with username as NULL can be
> terminated by non-superuser with pg_signal_backend privilege. When the
> username is NULL the bgworker process runs as superuser (which is
> expected as per the documentation -
> https://www.postgresql.org/docs/current/bgworker.html ), but can the
> non-superuser (with pg_signal_backend) terminate this superuser owned
> process?
> We (Mahendrakar and Myself) think that this is a bug and proposing a
> fix that sets MyProc->roleId to BOOTSTRAP_SUPERUSERID, similar to
> InitializeSessionUserId, to prevent non-superuser terminating it.
>
> Please let us know your comments.
>
> Thanks,
> Hemanth Sandrana
>

Re: [patch] pg_basebackup: mention that spread checkpoints are the default in --help

[Michael Banck]

Hi,

On Thu, Oct 19, 2023 at 04:21:19PM +0300, Aleksander Alekseev wrote:
> > I believed that spread (not fast) checkpoints are the default in
> > pg_basebackup, but noticed that --help does not specify which is which -
> > contrary to the reference documentation.
> >
> > So I propose the small attached patch to clarify that.
> 
> You are right and I believe this is a good change.
> 
> Maybe we should also display the defaults for -X,
> --manifest-checksums, etc for consistency.

Hrm right, but those have multiple options and they do not enumerate
them in the help string as do -F and -c - not sure what general project
policy here is for mentioning defaults in --help, I will check some of
the other commands.


Michael

Re: Is this a problem in GenericXLogFinish()?

[Robert Haas]

On Tue, Oct 17, 2023 at 12:38 PM Jeff Davis  wrote:
> I meant: are those cleanup operations frequent enough that dirtying
> those buffers in that case would matter?

Honestly, I'm not sure. Probably not? I mean, hashbucketcleanup()
seems to only be called during vacuum or a bucket split, and I don't
think you can have super-frequent calls to _hash_freeovflpage()
either. For what it's worth, though, I think it would be better to
just make these cases exceptions to your Assert, as you did in the
patch, rather than changing them to dirty the buffer. There doesn't
seem to be enough upside to making the assert unconditional to justify
changing stuff that might have a real-world performance cost ... even
if we don't think it would amount to much.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: trying again to get incremental backup

[Robert Haas]

On Thu, Oct 19, 2023 at 3:18 PM David Steele  wrote:
> 0001 looks pretty good to me. The only thing I find a little troublesome
> is the repeated construction of file names with/without segment numbers
> in ResetUnloggedRelationsInDbspaceDir(), .e.g.:
>
> +   if (segno == 0)
> +   snprintf(dstpath, sizeof(dstpath), "%s/%u",
> +dbspacedirname, relNumber);
> +   else
> +   snprintf(dstpath, sizeof(dstpath), "%s/%u.%u",
> +dbspacedirname, relNumber, 
> segno);
>
>
> If this happened three times I'd definitely want a helper function, but
> even with two I think it would be a bit nicer.

Personally I think that would make the code harder to read rather than
easier. I agree that repeating code isn't great, but this is a
relatively brief idiom and pretty self-explanatory. If other people
agree with you I can change it, but to me it's not an improvement.

> 0002 is definitely a good idea. FWIW pgBackRest does this conversion but
> also errors if it does not succeed. We have never seen a report of this
> error happening in the wild, so I think it must be pretty rare if it
> does happen.

Cool, but ... how about the main patch set? It's nice to get some of
these refactoring bits and pieces out of the way, but if I spend the
effort to work out what I think are the right answers to the remaining
design questions for the main patch set and then find out after I've
done all that that you have massive objections, I'm going to be
annoyed. I've been trying to get this feature into PostgreSQL for
years, and if I don't succeed this time, I want the reason to be
something better than "well, I didn't find out that David disliked X
until five minutes before I was planning to type 'git push'."

I'm not really concerned about detailed bug-hunting in the main
patches just yet. The time for that will come. But if you have views
on how to resolve the design questions that I mentioned in a couple of
emails back, or intend to advocate vigorously against the whole
concept for some reason, let's try to sort that out sooner rather than
later.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: trying again to get incremental backup

[David Steele]

On 10/19/23 12:05, Robert Haas wrote:

On Wed, Oct 4, 2023 at 4:08 PM Robert Haas  wrote:

Clearly there's a good amount of stuff to sort out here, but we've
still got quite a bit of time left before feature freeze so I'd like
to have a go at it. Please let me know your thoughts, if you have any.


Apparently, nobody has any thoughts, but here's an updated patch set
anyway. The main change, other than rebasing, is that I did a bunch
more documentation work on the main patch (0005). I'm much happier
with it now, although I expect it may need more adjustments here and
there as outstanding design questions get settled.

After some thought, I think that it should be fine to commit 0001 and
0002 as independent refactoring patches, and I plan to go ahead and do
that pretty soon unless somebody objects.


0001 looks pretty good to me. The only thing I find a little troublesome 
is the repeated construction of file names with/without segment numbers 
in ResetUnloggedRelationsInDbspaceDir(), .e.g.:


+   if (segno == 0)
+   snprintf(dstpath, sizeof(dstpath), "%s/%u",
+dbspacedirname, relNumber);
+   else
+   snprintf(dstpath, sizeof(dstpath), "%s/%u.%u",
+dbspacedirname, relNumber, 
segno);


If this happened three times I'd definitely want a helper function, but 
even with two I think it would be a bit nicer.


0002 is definitely a good idea. FWIW pgBackRest does this conversion but 
also errors if it does not succeed. We have never seen a report of this 
error happening in the wild, so I think it must be pretty rare if it 
does happen.


Regards,
-David

Re: Oversight in reparameterize_path_by_child leading to executor crash

[Alena Rybakina]

Hi!

Thank you for your work on the subject.


During review your patch I didn't understand why are you checking that 
the variable is path and not new_path of type T_SamplePath (I 
highlighted it)?


Path *
reparameterize_path_by_child(PlannerInfo *root, Path *path,
 RelOptInfo *child_rel)

...
switch (nodeTag(path))
    {
    case T_Path:
        new_path = path;
ADJUST_CHILD_ATTRS(new_path->parent->baserestrictinfo);
        if (*path*->pathtype == T_SampleScan)
        {

Is it a typo and should be new_path?


Besides, it may not be important, but reviewing your code all the time 
stumbled on the statement of the comments while reading it (I 
highlighted it too). This:


* path_is_reparameterizable_by_child
 *         Given a path parameterized by the parent of the given child 
relation,
 *         see if it can be translated to be parameterized by the child 
relation.

 *
 * This must return true if *and only if *reparameterize_path_by_child()
 * would succeed on this path.

Maybe is it better to rewrite it simplier:

 * This must return true *only if *reparameterize_path_by_child()
 * would succeed on this path.


And can we add assert in reparameterize_pathlist_by_child function that 
pathlist is not a NIL, because according to the comment it needs to be 
added there:


Returns NIL to indicate failure, so pathlist had better not be NIL.

--
Regards,
Alena Rybakina

Re: New WAL record to detect the checkpoint redo location

[Robert Haas]

On Thu, Oct 19, 2023 at 1:53 AM Michael Paquier  wrote:
> Seems fine to me.  Thanks for considering the idea.

I think it was a good idea!

I've committed the patch.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

prevent non-superuser terminate bgworker running as superuser

[Hemanth Sandrana]

Hi All,

Currently, BackgroundWorker connected to a database by calling
BackgroundWorkerInitializeConnection with username as NULL can be
terminated by non-superuser with pg_signal_backend privilege. When the
username is NULL the bgworker process runs as superuser (which is
expected as per the documentation -
https://www.postgresql.org/docs/current/bgworker.html ), but can the
non-superuser (with pg_signal_backend) terminate this superuser owned
process?
We (Mahendrakar and Myself) think that this is a bug and proposing a
fix that sets MyProc->roleId to BOOTSTRAP_SUPERUSERID, similar to
InitializeSessionUserId, to prevent non-superuser terminating it.

Please let us know your comments.

Thanks,
Hemanth Sandrana


v1-0001-prevent-non-superuser-terminating-superuser-owned.patch
Description: Binary data

controlling meson's parallelism (and some whining)

[Robert Haas]

Hi,

Twice now, I've had 'meson test' fail because it tried to start too
many copies of the server at the same time. In the server log, I get
the complaint about needing to raise SHMMNI. This is a macos machine,
with kern.sysv.shmmni=32. The obvious fix to this is to just tell
'meson test' how many processes I'd like it to run. I thought maybe I
could just do 'meson -j8 test' but that does not work, because the
option is --num-processes and has no short version. Even typing -j8
every time would be kind of annoying; typing --num-processes 8 every
time is ridiculously verbose.

My next thought was that there ought to be some environmental variable
that I could set to control this behavior. But I can't find a list of
environment variables that affect meson behavior anywhere. I guess the
authors don't believe in environment variable as a control mechanism.
Or, at the risk of sounding a bit testy, maybe their documentation
just isn't quite up to par. It's not that hard to find lists of
options for particular subcommands, either from the tool itself or on
the web site. But unlike git, where you can do something like 'man
git-checkout' and actually get more information than the command help
itself provides, there are no man pages for the main subcommands, and
I can't really find any good documentation on the web site either.
Knowing that a certain subcommand has a flag called
--pkgconfig.relocatable or that some other command has a flag called
--cross-file CROSS_FILE whose argument is, and I quote, a "File
describing cross compilation environment," is not good enough.

So my questions are:

1. Is there some better way to control testing parallelism than
specifying --num-processes N every single time?

2. Is there better documentation somewhere?

Thanks,

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: Allow ALTER SYSTEM SET on unrecognized custom GUCs

[Andrey M. Borodin]

> On 17 Oct 2023, at 05:19, Tom Lane  wrote:
> 
> In the original discussion about this [1], I initially leaned towards
> "they should both fail", but I reconsidered: there doesn't seem to be
> any harm in allowing ALTER SYSTEM SET to succeed for any custom GUC
> name, as long as you're superuser.

+1 for allowing non-existent custom GUCs.
From time to time we have to roll out custom binaries controlled by GUCs that 
do not exist in normal binaries. Juggling with postgresql.conf would be painful 
in this case.


Best regards, Andrey Borodin.

Re: Allow ALTER SYSTEM SET on unrecognized custom GUCs

[shihao zhong]

Thanks for the answer. The code looks good to me.

Thanks,
Shihao

On Thu, Oct 19, 2023 at 12:00 PM Tom Lane  wrote:

> shihao zhong  writes:
> > I do like the idea that we should keep the set and the altar system with
> > the same behavior. But one thing I am worried about is the typo detected
> > here because I usually make that type of mistake myself. I believe we
> > should have an extra log to explicitly tell the user this is a `custom
> > variable` guc.
>
> I don't think there's any chance of getting away with that.  As noted
> upthread, a lot of people use placeholder GUCs as a substitute for a
> proper session-variable feature.  If we ever get real session variables,
> we could start to nudge people away from using placeholders; but right
> now too many people would complain about the noise of a warning.
>
> > Btw, another aspect I want to better understand is if the superuser
> session
> > called pg_reload_conf with custom variables, does that mean these custom
> > variables will override the other active transaction's SET command?
>
> No, a per-session SET will override a value coming from the config file.
> That's independent of whether it's a regular or custom GUC.
>
> regards, tom lane
>

Re: The danger of deleting backup_label

[David Steele]

On 10/19/23 10:56, Robert Haas wrote:

On Thu, Oct 19, 2023 at 10:43 AM David Steele  wrote:

What I meant here (but said badly) is that in the case of snapshot
backups, the backup_label and tablespace_map will likely need to be
stored somewhere off the server since they can't be part of the
snapshot, perhaps in a key store. In that case the backup software would
still need to read the files from wherever we stored then and correctly
handle them when storing elsewhere. If you were moving the files to say,
S3, a similar thing needs to happen. In general, I think a locally
mounted filesystem is very unlikely to be the final destination for
these files, and if it is then probably pg_basebackup is your friend.


I mean, writing those tiny little files locally and then uploading
them should be fine in a case like that. It still reduces the surface
for mistakes. And you could also have --backup-label='| whatever' or
something if you wanted. The point is that right now we're asking
people to pull this information out of a query result, and that means
people are trying to do it by calling out to psql, and that is a GREAT
way to screw up the escaping or the newlines or whatever. I don't
think the mistakes people are making here are being made by people
using Perl and DBD::Pg, or Python and psycopg2, or C and libpq.
They're being made by people who are trying to shell script their way
through it, which entails using psql, which makes screwing it up a
breeze.


OK, I see what you mean and I agree. Better documentation might be the 
answer here, but I doubt that psql is a good tool for starting/stopping 
backup and not sure we want to encourage it.


Regards,
-David

Re: [PATCH] Add support function for containment operators

[Laurenz Albe]

On Fri, 2023-10-13 at 14:26 +0800, jian he wrote:
> Collation problem seems solved.

I didn't review your patch in detail, there is still a problem
with my example:

  CREATE TYPE textrange AS RANGE (
 SUBTYPE = text,
 SUBTYPE_OPCLASS = text_pattern_ops
  );

  CREATE TABLE tx (t text COLLATE "cs-CZ-x-icu");

  INSERT INTO tx VALUES ('a'), ('c'), ('d'), ('ch');

  SELECT * FROM tx WHERE t <@ textrange('a', 'd');

   t  
  
   a
   c
   ch
  (3 rows)

That was correct.

  EXPLAIN SELECT * FROM tx WHERE t <@ textrange('a', 'd');

   QUERY PLAN 
  
   Seq Scan on tx  (cost=0.00..30.40 rows=7 width=32)
 Filter: ((t >= 'a'::text) AND (t < 'd'::text))
  (2 rows)

But that was weird.  The operators seem wrong.  Look at that
query:

  SELECT * FROM tx WHERE t >= 'a' AND t < 'd';

   t 
  ═══
   a
   c
  (2 rows)

But the execution plan is identical...

I am not sure what is the problem here, but in my opinion the
operators shown in the execution plan should be like this:

  SELECT * FROM tx WHERE t ~>=~ 'a' AND t ~<~ 'd';

   t  
  
   a
   c
   ch
  (3 rows)

Yours,
Laurenz Albe

Re: Allow ALTER SYSTEM SET on unrecognized custom GUCs

[Tom Lane]

shihao zhong  writes:
> I do like the idea that we should keep the set and the altar system with
> the same behavior. But one thing I am worried about is the typo detected
> here because I usually make that type of mistake myself. I believe we
> should have an extra log to explicitly tell the user this is a `custom
> variable` guc.

I don't think there's any chance of getting away with that.  As noted
upthread, a lot of people use placeholder GUCs as a substitute for a
proper session-variable feature.  If we ever get real session variables,
we could start to nudge people away from using placeholders; but right
now too many people would complain about the noise of a warning.

> Btw, another aspect I want to better understand is if the superuser session
> called pg_reload_conf with custom variables, does that mean these custom
> variables will override the other active transaction's SET command?

No, a per-session SET will override a value coming from the config file.
That's independent of whether it's a regular or custom GUC.

regards, tom lane

Re: boolin comment not moved when code was refactored

[Tom Lane]

Peter Smith  writes:
> PSA v2.

Pushed.

regards, tom lane

Re: The danger of deleting backup_label

[David G. Johnston]

On Thursday, October 19, 2023, David Steele  wrote:

> On 10/19/23 10:24, Robert Haas wrote:
>
>> On Wed, Oct 18, 2023 at 7:15 PM David Steele  wrote:
>>
>>>
 pg_llbackup -d $CONNTR --backup-label=PATH --tablespace-map=PATH
 --copy-data-directory=SHELLCOMMAND

 I think in most cases where this would be useful the user should just be
>>> using pg_basebackup. If the backup is trying to use snapshots, then
>>> backup_label needs to be stored outside the snapshot and we won't be
>>> able to easily help.
>>>
>>
>> Right, the idea of the above was that you would specify paths for the
>> backup label and the tablespace map that were outside of the snapshot
>> directory in that kind of case. But you couldn't screw up the line
>> endings or whatever because pg_llbackup would take care of that aspect
>> of it for you.
>>
>
> What I meant here (but said badly) is that in the case of snapshot
> backups, the backup_label and tablespace_map will likely need to be stored
> somewhere off the server since they can't be part of the snapshot, perhaps
> in a key store. In that case the backup software would still need to read
> the files from wherever we stored then and correctly handle them when
> storing elsewhere. If you were moving the files to say, S3, a similar thing
> needs to happen. In general, I think a locally mounted filesystem is very
> unlikely to be the final destination for these files, and if it is then
> probably pg_basebackup is your friend.


>
We are choosing to not take responsibility if the procedure used by the dba
is one that takes a full live backup of the data directory as-is and then
brings that backup back into production without making any changes to it.
That restored copy will be bootable but quite probably corrupted.  That is
on them as we have no way to make it non-bootable without risking source
database being unable to be restarted automatically upon a crash.  In
short, a snapshot methodology is beyond what we are willing to provide
protections for.

What I do kinda gather from the above is we should be providing a
pg_baserestore application if we want to give our users fewer reasons to
write their own tooling against the API and/or want to add more complexity
to pg_basebackup to handle various needs that need corresponding recovery
needs that we should implement as code instead of documentation.

David J.

Remove last traces of HPPA support

[Tom Lane]

We removed support for the HP-UX OS in v16, but left in support
for the PA-RISC architecture, mainly because I thought that its
spinlock mechanism is weird enough to be a good stress test
for our spinlock infrastructure.  It still is that, but my
one remaining HPPA machine has gone to the great recycle heap
in the sky.  There seems little point in keeping around nominal
support for an architecture that we can't test and no one is
using anymore.

Hence, the attached removes the remaining support for HPPA.
Any objections?

regards, tom lane

diff --git a/contrib/pgcrypto/crypt-blowfish.c b/contrib/pgcrypto/crypt-blowfish.c
index 1264eccb3f..5a1b1e1009 100644
--- a/contrib/pgcrypto/crypt-blowfish.c
+++ b/contrib/pgcrypto/crypt-blowfish.c
@@ -41,7 +41,7 @@
 #ifdef __i386__
 #define BF_ASM0	/* 1 */
 #define BF_SCALE			1
-#elif defined(__x86_64__) || defined(__hppa__)
+#elif defined(__x86_64__)
 #define BF_ASM0
 #define BF_SCALE			1
 #else
diff --git a/doc/src/sgml/installation.sgml b/doc/src/sgml/installation.sgml
index f4b1f81189..3608aec595 100644
--- a/doc/src/sgml/installation.sgml
+++ b/doc/src/sgml/installation.sgml
@@ -3359,8 +3359,8 @@ export MANPATH
 
   
In general, PostgreSQL can be expected to work on
-   these CPU architectures: x86, PowerPC, S/390, SPARC, ARM, MIPS, RISC-V,
-   and PA-RISC, including
+   these CPU architectures: x86, PowerPC, S/390, SPARC, ARM, MIPS,
+   and RISC-V, including
big-endian, little-endian, 32-bit, and 64-bit variants where applicable.
It is often
possible to build on an unsupported CPU type by configuring with
@@ -3391,7 +3391,8 @@ export MANPATH
   
Historical versions of PostgreSQL or POSTGRES
also ran on CPU architectures including Alpha, Itanium, M32R, M68K,
-   M88K, NS32K, SuperH, and VAX, and operating systems including 4.3BSD, BEOS,
+   M88K, NS32K, PA-RISC, SuperH, and VAX,
+   and operating systems including 4.3BSD, BEOS,
BSD/OS, DG/UX, Dynix, HP-UX, IRIX, NeXTSTEP, QNX, SCO, SINIX, Sprite, SunOS,
Tru64 UNIX, and ULTRIX.
   
diff --git a/src/backend/storage/lmgr/s_lock.c b/src/backend/storage/lmgr/s_lock.c
index 327ac64f7c..0e3f04207c 100644
--- a/src/backend/storage/lmgr/s_lock.c
+++ b/src/backend/storage/lmgr/s_lock.c
@@ -110,12 +110,7 @@ s_lock(volatile slock_t *lock, const char *file, int line, const char *func)
 void
 s_unlock(volatile slock_t *lock)
 {
-#ifdef TAS_ACTIVE_WORD
-	/* HP's PA-RISC */
-	*TAS_ACTIVE_WORD(lock) = -1;
-#else
 	*lock = 0;
-#endif
 }
 #endif
 
diff --git a/src/include/port/atomics.h b/src/include/port/atomics.h
index bbff945eba..f6f62d68c0 100644
--- a/src/include/port/atomics.h
+++ b/src/include/port/atomics.h
@@ -69,8 +69,6 @@
 #include "port/atomics/arch-x86.h"
 #elif defined(__ppc__) || defined(__powerpc__) || defined(__ppc64__) || defined(__powerpc64__)
 #include "port/atomics/arch-ppc.h"
-#elif defined(__hppa) || defined(__hppa__)
-#include "port/atomics/arch-hppa.h"
 #endif
 
 /*
diff --git a/src/include/port/atomics/arch-hppa.h b/src/include/port/atomics/arch-hppa.h
deleted file mode 100644
index 4c89fbff71..00
--- a/src/include/port/atomics/arch-hppa.h
+++ /dev/null
@@ -1,17 +0,0 @@
-/*-
- *
- * arch-hppa.h
- *	  Atomic operations considerations specific to HPPA
- *
- * Portions Copyright (c) 1996-2023, PostgreSQL Global Development Group
- * Portions Copyright (c) 1994, Regents of the University of California
- *
- * NOTES:
- *
- * src/include/port/atomics/arch-hppa.h
- *
- *-
- */
-
-/* HPPA doesn't do either read or write reordering */
-#define pg_memory_barrier_impl()		pg_compiler_barrier_impl()
diff --git a/src/include/port/atomics/fallback.h b/src/include/port/atomics/fallback.h
index a9e8e77c03..d119e8cc50 100644
--- a/src/include/port/atomics/fallback.h
+++ b/src/include/port/atomics/fallback.h
@@ -75,11 +75,7 @@ typedef struct pg_atomic_flag
 	 * be content with just one byte instead of 4, but that's not too much
 	 * waste.
 	 */
-#if defined(__hppa) || defined(__hppa__)	/* HP PA-RISC, GCC and HP compilers */
-	int			sema[4];
-#else
 	int			sema;
-#endif
 	volatile bool value;
 } pg_atomic_flag;
 
@@ -93,11 +89,7 @@ typedef struct pg_atomic_flag
 typedef struct pg_atomic_uint32
 {
 	/* Check pg_atomic_flag's definition above for an explanation */
-#if defined(__hppa) || defined(__hppa__)	/* HP PA-RISC */
-	int			sema[4];
-#else
 	int			sema;
-#endif
 	volatile uint32 value;
 } pg_atomic_uint32;
 
@@ -111,11 +103,7 @@ typedef struct pg_atomic_uint32
 typedef struct pg_atomic_uint64
 {
 	/* Check pg_atomic_flag's definition above for an explanation */
-#if defined(__hppa) || defined(__hppa__)	/* HP PA-RISC */
-	int			sema[4];
-#else
 	int			sema;
-#endif
 	volatile uint64 value;
 } pg_atomic_uint64;
 
diff --git a/src/include/storage/s_lock.h b/src/include/storage/s_lock.h
index 

Re: The danger of deleting backup_label

[Robert Haas]

On Thu, Oct 19, 2023 at 10:43 AM David Steele  wrote:
> What I meant here (but said badly) is that in the case of snapshot
> backups, the backup_label and tablespace_map will likely need to be
> stored somewhere off the server since they can't be part of the
> snapshot, perhaps in a key store. In that case the backup software would
> still need to read the files from wherever we stored then and correctly
> handle them when storing elsewhere. If you were moving the files to say,
> S3, a similar thing needs to happen. In general, I think a locally
> mounted filesystem is very unlikely to be the final destination for
> these files, and if it is then probably pg_basebackup is your friend.

I mean, writing those tiny little files locally and then uploading
them should be fine in a case like that. It still reduces the surface
for mistakes. And you could also have --backup-label='| whatever' or
something if you wanted. The point is that right now we're asking
people to pull this information out of a query result, and that means
people are trying to do it by calling out to psql, and that is a GREAT
way to screw up the escaping or the newlines or whatever. I don't
think the mistakes people are making here are being made by people
using Perl and DBD::Pg, or Python and psycopg2, or C and libpq.
They're being made by people who are trying to shell script their way
through it, which entails using psql, which makes screwing it up a
breeze.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: Add support for AT LOCAL

[Tom Lane]

Robert Haas  writes:
> On Wed, Oct 18, 2023 at 7:33 PM Noah Misch  wrote:
>> I feel the gravity and longevity of xlc bugs has been out of proportion with
>> the compiler's contribution to PostgreSQL.  I would find it reasonable to
>> revoke xlc support in v17+, leaving AIX gcc support in place.

> +1 for this proposal.

WFM, too.

regards, tom lane

Re: The danger of deleting backup_label

[David Steele]

On 10/19/23 10:24, Robert Haas wrote:

On Wed, Oct 18, 2023 at 7:15 PM David Steele  wrote:

(b) be stored someplace
else,


I don't think the additional fields *need* to be stored anywhere at all,
at least not by us. We can provide them as output from pg_backup_stop()
and the caller can do as they please. None of those fields are part of
the restore process.


Not sure which fields we're talking about here. I agree that if
they're not really needed, we can return them and the user can keep
them or discard them as they wish. But in that case you might also ask
why bother even returning them.


I'm specifically talking about START TIME and LABEL. They are currently 
stored in backup_label but not used for recovery. START TIMELINE is also 
not used, except as a cross check against START WAL LOCATION.


I'd still like to see most or all of these fields exposed through 
pg_backup_stop(). The user can choose to store them or not, but none of 
them will be required for recovery.



pg_llbackup -d $CONNTR --backup-label=PATH --tablespace-map=PATH
--copy-data-directory=SHELLCOMMAND


I think in most cases where this would be useful the user should just be
using pg_basebackup. If the backup is trying to use snapshots, then
backup_label needs to be stored outside the snapshot and we won't be
able to easily help.


Right, the idea of the above was that you would specify paths for the
backup label and the tablespace map that were outside of the snapshot
directory in that kind of case. But you couldn't screw up the line
endings or whatever because pg_llbackup would take care of that aspect
of it for you.


What I meant here (but said badly) is that in the case of snapshot 
backups, the backup_label and tablespace_map will likely need to be 
stored somewhere off the server since they can't be part of the 
snapshot, perhaps in a key store. In that case the backup software would 
still need to read the files from wherever we stored then and correctly 
handle them when storing elsewhere. If you were moving the files to say, 
S3, a similar thing needs to happen. In general, I think a locally 
mounted filesystem is very unlikely to be the final destination for 
these files, and if it is then probably pg_basebackup is your friend.


Regards,
-David

Re: Add support for AT LOCAL

[Robert Haas]

On Wed, Oct 18, 2023 at 7:33 PM Noah Misch  wrote:
> I feel the gravity and longevity of xlc bugs has been out of proportion with
> the compiler's contribution to PostgreSQL.  I would find it reasonable to
> revoke xlc support in v17+, leaving AIX gcc support in place.

+1 for this proposal. I just think this is getting silly. We're saying
that we only have access to 1 or 2 AIX machines, and most of us have
access to none, and the compiler has serious code generation bugs that
are present in both a release 11 years old and also a release current
release, meaning they went unfixed for 10 years, and we can't report
bugs or get them fixed when we find them, and the use of this
particular compiler in the buildfarm isn't finding any issues that
matter anywhere else.

To be honest, I'm not entirely sure that even AIX gcc support is
delivering enough value per unit work to justify keeping it around.
But the xlc situation is worse.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: The danger of deleting backup_label

[Robert Haas]

On Wed, Oct 18, 2023 at 7:15 PM David Steele  wrote:
> > (b) be stored someplace
> > else,
>
> I don't think the additional fields *need* to be stored anywhere at all,
> at least not by us. We can provide them as output from pg_backup_stop()
> and the caller can do as they please. None of those fields are part of
> the restore process.

Not sure which fields we're talking about here. I agree that if
they're not really needed, we can return them and the user can keep
them or discard them as they wish. But in that case you might also ask
why bother even returning them.

> > pg_llbackup -d $CONNTR --backup-label=PATH --tablespace-map=PATH
> > --copy-data-directory=SHELLCOMMAND
> >
> I think in most cases where this would be useful the user should just be
> using pg_basebackup. If the backup is trying to use snapshots, then
> backup_label needs to be stored outside the snapshot and we won't be
> able to easily help.

Right, the idea of the above was that you would specify paths for the
backup label and the tablespace map that were outside of the snapshot
directory in that kind of case. But you couldn't screw up the line
endings or whatever because pg_llbackup would take care of that aspect
of it for you.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: Avoid race condition for event_triggers regress test

[Aleksander Alekseev]

Hi,

> The current problem is that a race condition may occur on some systems, when 
> oidjoins test starts a moment later than normally and affects logins count 
> for on-login trigger test. The problem is quite a rare one and I only faced 
> it once. But rare or not - the problem is a problem and it should be 
> addressed.

Thanks for the patch and the steps to reproduce.

I tested the patch and it does what is claimed. Including the steps to
reproduce as a separate patch with .txt extension so cfbot will ignore
it.

I think it's a good find and a good fix.

-- 
Best regards,
Aleksander Alekseev
commit 80df7cf26476a3ede42310354715e972fa40a8cf
Author: Aleksander Alekseev 
Date:   Thu Oct 19 16:30:27 2023 +0300

reproduce

diff --git a/src/test/regress/expected/event_trigger.out 
b/src/test/regress/expected/event_trigger.out
index eaaff6ba6f..741099747c 100644
--- a/src/test/regress/expected/event_trigger.out
+++ b/src/test/regress/expected/event_trigger.out
@@ -648,6 +648,13 @@ BEGIN
 END;
 $$ LANGUAGE plpgsql;
 CREATE EVENT TRIGGER on_login_trigger ON login EXECUTE PROCEDURE 
on_login_proc();
+-- AALEKSEEV DEBUG
+select pg_sleep(5);
+ pg_sleep 
+--
+ 
+(1 row)
+
 ALTER EVENT TRIGGER on_login_trigger ENABLE ALWAYS;
 \c
 NOTICE:  You are welcome!
diff --git a/src/test/regress/expected/oidjoins.out 
b/src/test/regress/expected/oidjoins.out
index 215eb899be..01f4959f21 100644
--- a/src/test/regress/expected/oidjoins.out
+++ b/src/test/regress/expected/oidjoins.out
@@ -1,3 +1,11 @@
+-- AALEKSEEV DEBUG
+select pg_sleep(2);
+ pg_sleep 
+--
+ 
+(1 row)
+
+\c
 --
 -- Verify system catalog foreign key relationships
 --
diff --git a/src/test/regress/sql/event_trigger.sql 
b/src/test/regress/sql/event_trigger.sql
index 9c2b7903fb..84b0b7fac8 100644
--- a/src/test/regress/sql/event_trigger.sql
+++ b/src/test/regress/sql/event_trigger.sql
@@ -506,6 +506,10 @@ BEGIN
 END;
 $$ LANGUAGE plpgsql;
 CREATE EVENT TRIGGER on_login_trigger ON login EXECUTE PROCEDURE 
on_login_proc();
+
+-- AALEKSEEV DEBUG
+select pg_sleep(5);
+
 ALTER EVENT TRIGGER on_login_trigger ENABLE ALWAYS;
 \c
 SELECT COUNT(*) FROM user_logins;
diff --git a/src/test/regress/sql/oidjoins.sql 
b/src/test/regress/sql/oidjoins.sql
index 8b22e6d10c..576d5f03a9 100644
--- a/src/test/regress/sql/oidjoins.sql
+++ b/src/test/regress/sql/oidjoins.sql
@@ -1,3 +1,7 @@
+-- AALEKSEEV DEBUG
+select pg_sleep(2);
+\c
+
 --
 -- Verify system catalog foreign key relationships
 --

Re: Allow ALTER SYSTEM SET on unrecognized custom GUCs

[shihao zhong]

I do like the idea that we should keep the set and the altar system with
the same behavior. But one thing I am worried about is the typo detected
here because I usually make that type of mistake myself. I believe we
should have an extra log to explicitly tell the user this is a `custom
variable` guc.

Btw, another aspect I want to better understand is if the superuser session
called pg_reload_conf with custom variables, does that mean these custom
variables will override the other active transaction's SET command?

Thanks,
Shihao

On Wed, Oct 18, 2023 at 1:59 AM Andrei Lepikhov 
wrote:

> On 18/10/2023 12:15, Tom Lane wrote:
> > Andrei Lepikhov  writes:
> >> "SET foo.bar TO 'smth'" can immediately alter the placeholder's value.
> >> But what is the reason that "ALTER SYSTEM SET foo.bar TO 'smth'" doesn't
> >> do the same?
> >
> > Because it's not supposed to take effect until you issue a reload
> > command (and maybe not even then, depending on which GUC we're
> > talking about).  I certainly think it wouldn't make sense for your
> > own session to adopt the value ahead of others.
>
> Thanks for the answer.
> Introducing the assignable_custom_variable_name can be helpful. The code
> looks good. I think it deserves to be committed - after the indentation
> fix, of course.
>
> --
> regards,
> Andrey Lepikhov
> Postgres Professional
>
>
>
>

Re: Patch: Improve Boolean Predicate JSON Path Docs

[David E. Wheeler]

On Oct 19, 2023, at 01:22, jian he  wrote:

> "Do not use with non-predicate", double negative is not easy to
> comprehend. Maybe we can simplify it.
> 
> 16933: value. Use only SQL-standard JSON path expressions, not not
> there are two "not".
> 
> 15842: SQL-standard JSON path expressions, not not
> there are two "not”.


Thank you, jian. Updated patch attached and also on GitHub.

  
https://github.com/postgres/postgres/compare/master...theory:postgres:jsonpath-pred-docs

Best,

David




v5-0001-Improve-boolean-predicate-JSON-Path-docs.patch
Description: application/applefile

Re: [patch] pg_basebackup: mention that spread checkpoints are the default in --help

[Aleksander Alekseev]

Hi,

> I believed that spread (not fast) checkpoints are the default in
> pg_basebackup, but noticed that --help does not specify which is which -
> contrary to the reference documentation.
>
> So I propose the small attached patch to clarify that.

You are right and I believe this is a good change.

Maybe we should also display the defaults for -X,
--manifest-checksums, etc for consistency.

-- 
Best regards,
Aleksander Alekseev

RE: [PoC] pg_upgrade: allow to upgrade publisher node

[Hayato Kuroda (Fujitsu)]

Dear hackers,

> Thanks for reviewing! PSA new version.

Hmm. The cfbot got angry, whereas it can pass on my machine.
It seems that the ordering in invalid_logical_replication_slots.txt is not 
fixed.

A change for checking the content was reverted. It could pass on my CI.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED



v54-0001-pg_upgrade-Allow-to-replicate-logical-replicatio.patch
Description:  v54-0001-pg_upgrade-Allow-to-replicate-logical-replicatio.patch

Re: BRIN minmax multi - incorrect distance for infinite timestamp/date

[Ashutosh Bapat]

On Thu, Oct 19, 2023 at 4:51 PM Tomas Vondra
 wrote:
>
> On 10/19/23 11:22, Ashutosh Bapat wrote:
> > On Thu, Oct 19, 2023 at 2:31 PM Tomas Vondra
> >  wrote:
> >
> >>
> >> Does that explain the algorithm? I'm not against clarifying the comment,
> >> of course.
> >
> > Thanks a lot for this explanation. It's clear now.
> >
> >> I tried to do that, but I ran into troubles with the "date" tests. I
> >> needed to build values that close to the min/max values, so I did
> >> something like
> >>
> >> SELECT '4713-01-01 BC'::date + (i || ' days')::interval FROM
> >> generate_series(1,10) s(i);
> >>
> >> And then the same for the max date, but that fails because of the
> >> date/timestamp conversion in date plus operator.
> >>
> >> However, maybe two simple generate_series() would work ...
> >>
> >
> > Something like this? select i::date from  generate_series('4713-02-01
> > BC'::date,  '4713-01-01 BC'::date, '-1 day'::interval) i;
>
> That works, but if you try the same thing with the largest date, that'll
> fail
>
>   select i::date from  generate_series('5874896-12-01'::date,
>'5874897-01-01'::date,
>'1 day'::interval) i;
>
>   ERROR:  date out of range for timestamp

Hmm, I see. This uses generate_series(timestamp, timestamp, interval) version.

date + integer -> date though, so the following works. It's also an
example at https://www.postgresql.org/docs/16/functions-srf.html.
#SELECT '5874896-12-01'::date + i FROM
generate_series(1,10) s(i);

I think we should provide generate_series(date, date, integer) which
will use date + integer -> date.

-- 
Best Wishes,
Ashutosh Bapat

Re: [PATCH] Tracking statements entry timestamp in pg_stat_statements

[Andrei Zubkov]

Hi hackers,

New version 23 attached. It contains rebase to the current master.
Noted that v1.11 adds new fields to the pg_stat_sstatements view, but
leaves the PGSS_FILE_HEADER constant unchanged. It this correct?

-- 
Andrei Zubkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
From 1bc4ccaed7cd3922d0b0bb83afd07f386792c8dc Mon Sep 17 00:00:00 2001
From: Andrei Zubkov 
Date: Thu, 19 Oct 2023 15:04:42 +0300
Subject: [PATCH 1/2] pg_stat_statements tests: Add NOT NULL checking of
 pg_stat_statements_reset

This is preliminary patch. It adds NOT NULL checking for the result of
pg_stat_statements_reset() function. It is needed for upcoming patch
"Track statement entry timestamp" that will change the result type of
this function to the timestamp of a reset performed.

Discussion: https://www.postgresql.org/message-id/flat/72e80e7b160a6eb189df9ef6f068cce3765d37f8.camel%40moonset.ru
---
 .../pg_stat_statements/expected/cursors.out   |  28 +--
 contrib/pg_stat_statements/expected/dml.out   |  28 +--
 .../expected/level_tracking.out   |  80 
 .../pg_stat_statements/expected/planning.out  |  18 +-
 .../pg_stat_statements/expected/select.out|  44 ++--
 .../expected/user_activity.out| 120 +--
 .../pg_stat_statements/expected/utility.out   | 192 +-
 contrib/pg_stat_statements/expected/wal.out   |  10 +-
 contrib/pg_stat_statements/sql/cursors.sql|   6 +-
 contrib/pg_stat_statements/sql/dml.sql|   6 +-
 .../pg_stat_statements/sql/level_tracking.sql |  12 +-
 contrib/pg_stat_statements/sql/planning.sql   |   4 +-
 contrib/pg_stat_statements/sql/select.sql |  10 +-
 .../pg_stat_statements/sql/user_activity.sql  |  15 +-
 contrib/pg_stat_statements/sql/utility.sql|  34 ++--
 contrib/pg_stat_statements/sql/wal.sql|   2 +-
 16 files changed, 307 insertions(+), 302 deletions(-)

diff --git a/contrib/pg_stat_statements/expected/cursors.out b/contrib/pg_stat_statements/expected/cursors.out
index 46375ea9051..0fc4b2c098d 100644
--- a/contrib/pg_stat_statements/expected/cursors.out
+++ b/contrib/pg_stat_statements/expected/cursors.out
@@ -3,10 +3,10 @@
 --
 -- These tests require track_utility to be enabled.
 SET pg_stat_statements.track_utility = TRUE;
-SELECT pg_stat_statements_reset();
- pg_stat_statements_reset 
---
- 
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+ t 
+---
+ t
 (1 row)
 
 -- DECLARE
@@ -20,13 +20,13 @@ SELECT calls, rows, query FROM pg_stat_statements ORDER BY query COLLATE "C";
 ---+--+---
  2 |0 | CLOSE cursor_stats_1
  2 |0 | DECLARE cursor_stats_1 CURSOR WITH HOLD FOR SELECT $1
- 1 |1 | SELECT pg_stat_statements_reset()
+ 1 |1 | SELECT pg_stat_statements_reset() IS NOT NULL AS t
 (3 rows)
 
-SELECT pg_stat_statements_reset();
- pg_stat_statements_reset 
---
- 
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+ t 
+---
+ t
 (1 row)
 
 -- FETCH
@@ -59,12 +59,12 @@ SELECT calls, rows, query FROM pg_stat_statements ORDER BY query COLLATE "C";
  1 |0 | DECLARE cursor_stats_2 CURSOR WITH HOLD FOR SELECT $1
  1 |1 | FETCH 1 IN cursor_stats_1
  1 |1 | FETCH 1 IN cursor_stats_2
- 1 |1 | SELECT pg_stat_statements_reset()
+ 1 |1 | SELECT pg_stat_statements_reset() IS NOT NULL AS t
 (9 rows)
 
-SELECT pg_stat_statements_reset();
- pg_stat_statements_reset 
---
- 
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+ t 
+---
+ t
 (1 row)
 
diff --git a/contrib/pg_stat_statements/expected/dml.out b/contrib/pg_stat_statements/expected/dml.out
index ede47a71acc..f6ac8da5ca2 100644
--- a/contrib/pg_stat_statements/expected/dml.out
+++ b/contrib/pg_stat_statements/expected/dml.out
@@ -81,16 +81,16 @@ SELECT calls, rows, query FROM pg_stat_statements ORDER BY query COLLATE "C";
  1 |   12 | SELECT * FROM pgss_dml_tab ORDER BY a
  2 |4 | SELECT * FROM pgss_dml_tab WHERE a > $1 ORDER BY a
  1 |8 | SELECT * FROM pgss_dml_tab WHERE a IN ($1, $2, $3, $4, $5)
- 1 |1 | SELECT pg_stat_statements_reset()
+ 1 |1 | SELECT pg_stat_statements_reset() IS NOT NULL AS t
  1 |0 | SET pg_stat_statements.track_utility = FALSE
  6 |6 | UPDATE pgss_dml_tab SET b = $1 WHERE a = $2
  1 |3 | UPDATE pgss_dml_tab SET b = $1 WHERE a > $2
 (10 rows)
 
-SELECT pg_stat_statements_reset();
- pg_stat_statements_reset 
---
- 
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+ t 
+---
+ t
 (1 row)
 
 -- MERGE
@@ -136,16 +136,16 @@ SELECT calls, rows, query FROM pg_stat_statements ORDER BY query COLLATE "C";
|  |  WHEN NOT MATCHED THEN INSERT (a, b) VALUES ($1, $2)
  1 |0 | MERGE INTO pgss_dml_tab USING pgss_dml_tab st ON (st.a = pgss_dml_tab.a)   +
|  |  WHEN NOT MATCHED THEN INSERT (b, a) 

Re: BRIN minmax multi - incorrect distance for infinite timestamp/date

[Tomas Vondra]

On 10/19/23 11:22, Ashutosh Bapat wrote:
> On Thu, Oct 19, 2023 at 2:31 PM Tomas Vondra
>  wrote:
> 
>>
>> Does that explain the algorithm? I'm not against clarifying the comment,
>> of course.
> 
> Thanks a lot for this explanation. It's clear now.
> 
>> I tried to do that, but I ran into troubles with the "date" tests. I
>> needed to build values that close to the min/max values, so I did
>> something like
>>
>> SELECT '4713-01-01 BC'::date + (i || ' days')::interval FROM
>> generate_series(1,10) s(i);
>>
>> And then the same for the max date, but that fails because of the
>> date/timestamp conversion in date plus operator.
>>
>> However, maybe two simple generate_series() would work ...
>>
> 
> Something like this? select i::date from  generate_series('4713-02-01
> BC'::date,  '4713-01-01 BC'::date, '-1 day'::interval) i;

That works, but if you try the same thing with the largest date, that'll
fail

  select i::date from  generate_series('5874896-12-01'::date,
   '5874897-01-01'::date,
   '1 day'::interval) i;

  ERROR:  date out of range for timestamp


regards

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

Re: [HACKERS] Allow INSTEAD OF DELETE triggers to modify the tuple for RETURNING

[Marko Tiikkaja]

Hi,

Thank you for the feedback.

Apparently it took me six years, but I've attached the latest version
of the patch which I believe addresses all issues.  I'll add it to the
open commitfest.


.m


instead_of_delete_returning_v3.patch
Description: Binary data

Re: Use virtual tuple slot for Unique node

[David Rowley]

On Thu, 19 Oct 2023 at 22:29, David Rowley  wrote:
> It's hard to imagine why there would be a slowdown as this query uses
> a TTSOpsMinimalTuple slot type in the patch and the unpatched version.

I shrunk down your table sizes to 10k rows instead of 1 million rows
to reduce the CPU cache pressure on the queries.

I ran pgbench for 1 minute on each query and did pg_prewarm on each
table. Here are the times I got in milliseconds:

Query   master   Master + 0001   compare
Q12.576 1.979 130.17%
Q29.546 9.941   96.03%
Q39.069 9.536   95.10%
Q47.285 7.208 101.07%
Q57.585 6.904 109.86%
Q6 162.253 161.434100.51%
Q7   62.507   58.922106.08%

I also noted down the slot type that nodeUnique.c is using in each of
the queries:

Q1 TTSOpsVirtual
Q2 TTSOpsVirtual
Q3 TTSOpsVirtual
Q4 TTSOpsMinimalTuple
Q5 TTSOpsVirtual
Q6 TTSOpsMinimalTuple
Q7 TTSOpsMinimalTuple

So, I'm not really expecting Q4, Q6 or Q7 to change much. However, Q7
does seem to be above noise level faster and I'm not sure why.

We can see that Q2 and Q3 become a bit slower.  This makes sense as
tts_virtual_materialize() is quite a bit more complex than
heap_copy_minimal_tuple() which is a simple palloc/memcpy.

We'd likely see Q2 and Q3 do better with the patched version if there
were more duplicates as there'd be less tuple deforming going on
because of the virtual slots.

Overall, the patched version is 5.55% faster than master.  However,
it's pretty hard to say if we should do this or not. Q3 has a mix of
varlena and byval types and that came out slower with the patched
version.

I've attached the script I used to get the results and the setup,
which is just your tables shrunk down to 10k rows.

David
#!/bin/bash

psql -c "select 
pg_prewarm('t_int'),pg_prewarm('t_text'),pg_prewarm('t_mixed');" postgres

for sql in "select distinct a,b from t_int;" "select distinct a,b from t_text;" 
"select distinct a,b from t_mixed;" "select distinct a,b from (select sum(a) 
over (order by a rows 2 preceding) a, b from t_int) q;" "select distinct a,b 
from (select sum(a) over (order by a rows 2 preceding) a, b from t_int order by 
a, b) q;" "select distinct a,b from (select string_agg(a, ', ') over (order by 
a rows 2 preceding) a, b from t_text) q;" "select distinct a,b from (select 
string_agg(left(a, 100), ', ') over (order by a rows 2 preceding) a, b from 
t_text) q;"
do
echo "set enable_hashagg=0;" > bench.sql
echo "set work_mem = '10GB';" >> bench.sql
echo "$sql" >> bench.sql
pgbench -n -f bench.sql -M prepared -T 60 postgres | grep latency
done




setup.sql
Description: Binary data

RE: [PoC] pg_upgrade: allow to upgrade publisher node

[Hayato Kuroda (Fujitsu)]

Dear Vignesh,

Thanks for revieing! New patch can be available in [1].

> Few comments:
> 1) Even if we comment 3rd point "Emit a non-transactional message",
> test_slot2 still appears in the invalid_logical_replication_slots.txt
> file. There is something wrong here.
> +   # 2. Advance the slot test_slot2 up to the current WAL location, but
> +   #test_slot1 still has unconsumed WAL records.
> +   $old_publisher->safe_psql('postgres',
> +   "SELECT pg_replication_slot_advance('test_slot2', NULL);");
> +
> +   # 3. Emit a non-transactional message. test_slot2 detects the message
> so
> +   #that this slot will be also reported by upcoming pg_upgrade.
> +   $old_publisher->safe_psql('postgres',
> +   "SELECT count(*) FROM pg_logical_emit_message('false',
> 'prefix', 'This is a non-transactional message');"
> +   );

The comment was updated based on others. How do you think?

> 2) If the test fails here, it is difficult to debug as the
> pg_upgrade_output.d directory was removed, so better to keep the
> directory as it is this case:
> +   # Check the file content. Both slots should be reporting that they 
> have
> +   # unconsumed WAL records.
> +   like(
> +   slurp_file($slots_filename),
> +   qr/The slot \"test_slot1\" has not consumed the WAL yet/m,
> +   'the previous test failed due to unconsumed WALs');
> +   like(
> +   slurp_file($slots_filename),
> +   qr/The slot \"test_slot2\" has not consumed the WAL yet/m,
> +   'the previous test failed due to unconsumed WALs');
> +
> +   # Clean up
> +   rmtree($new_publisher->data_dir . "/pg_upgrade_output.d");

Right. Current style just follows the 002 test. I removed rmtree().

> 3) The below could be changed:
> +   # Check the file content. Both slots should be reporting that they 
> have
> +   # unconsumed WAL records.
> +   like(
> +   slurp_file($slots_filename),
> +   qr/The slot \"test_slot1\" has not consumed the WAL yet/m,
> +   'the previous test failed due to unconsumed WALs');
> +   like(
> +   slurp_file($slots_filename),
> +   qr/The slot \"test_slot2\" has not consumed the WAL yet/m,
> +   'the previous test failed due to unconsumed WALs');
> 
> to:
> my $result = slurp_file($slots_filename);
> is( $result, qq(The slot "test_slot1" has not consumed the WAL yet
> The slot "test_slot2" has not consumed the WAL yet
> ),
> 'the previous test failed due to unconsumed WALs');
>

Replaced, but the formatting seems not good. I wanted to hear opinions from 
others.

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

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

RE: [PoC] pg_upgrade: allow to upgrade publisher node

[Hayato Kuroda (Fujitsu)]

Dear Shlok,

> 
> I have tested the above scenario. We are able to override the
> max_slot_wal_keep_size by using  '--new-options  "-c
> max_slot_wal_keep_size=val"'. And also with some insert statements
> during pg_upgrade, old WAL file were deleted and logical replication
> slots were invalidated. Since the slots were invalidated replication
> was not happening after the upgrade.

Yeah, theoretically it could be overwritten, but I still think we do not have to
guard. Also, connections must not be established during the upgrade [1].
I improved the ereport() message in the new patch[2]. How do you think?

[1]: https://www.postgresql.org/message-id/ZNZ4AxUMIrnMgRbo%40momjian.us
[2]: 
https://www.postgresql.org/message-id/TYCPR01MB587007EA2F9AB92F0E1F5957F5D4A%40TYCPR01MB5870.jpnprd01.prod.outlook.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

RE: [PoC] pg_upgrade: allow to upgrade publisher node

[Hayato Kuroda (Fujitsu)]

Dear Hou,

Thanks for reviewing! New patch can be available in [1].

> Thanks for updating the patch, here are few comments for the test.
> 
> 1.
> 
> >
> # The TAP Cluster.pm assigns default 'max_wal_senders' and 'max_connections'
> to
> # the same value (10) but PG12 and prior considered max_walsenders as a subset
> # of max_connections, so setting the same value will fail.
> if ($old_publisher->pg_version->major < 12)
> {
>   $old_publisher->append_conf(
>   'postgresql.conf', qq[
>   max_wal_senders = 5
>   max_connections = 10
>   ]);
> >
> 
> I think we already set max_wal_senders to 5 in init() function(in Cluster.pm),
> so is this necessary ? And 002_pg_upgrade.pl doesn't seems set this.

I thought you mentioned about Cluster::V_11::init(). I analyzed based on that 
and
found a fault. Could you please check [1]?

> 2.
> 
>   SELECT pg_create_logical_replication_slot('test_slot1',
> 'test_decoding', false, true);
>   SELECT pg_create_logical_replication_slot('test_slot2',
> 'test_decoding', false, true);
> 
> I think we don't need to set the last two parameters here as we don't check
> these info in the tests.

Removed.

> 3.
> 
> # Set extra params if cross-version checks are required. This is needed to
> # avoid using previously initdb'd cluster
> if (defined($ENV{oldinstall}))
> {
>   my @initdb_params = ();
>   push @initdb_params, ('--encoding', 'UTF-8');
>   push @initdb_params, ('--locale', 'C');
> 
> I am not sure I understand the comment, would it be possible provide a bit 
> more
> explanation about the purpose of this setting ? And I see 002_pg_upgrade 
> always
> have these setting even if oldinstall is not defined, so shall we follow the
> same ?

Fixed.
Actually settings are not needed for new cluster, but seems better to follow 
002.

> 4.
> 
> + command_ok(
> + [
> + 'pg_upgrade', '--no-sync',
> + '-d', $old_publisher->data_dir,
> + '-D', $new_publisher->data_dir,
> + '-b', $oldbindir,
> + '-B', $newbindir,
> + '-s', $new_publisher->host,
> + '-p', $old_publisher->port,
> + '-P', $new_publisher->port,
> + $mode,
> + ],
> 
> I think all the pg_upgrade commands in the test are the same, so we can save 
> the
> cmd
> in a variable and pass them to command_xx(). I think it can save some effort 
> to
> check the difference of each command and can also reduce some codes.

Fixed.

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

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

RE: [PoC] pg_upgrade: allow to upgrade publisher node

[Hayato Kuroda (Fujitsu)]

Dear Vignesh,

Thanks for reviewing! New patch can be available in [1].

> 
> Few comments:
> 1) We will be able to override the value of max_slot_wal_keep_size by
> using --new-options like '--new-options  "-c
> max_slot_wal_keep_size=val"':
> +   /*
> +* Use max_slot_wal_keep_size as -1 to prevent the WAL removal by the
> +* checkpointer process.  If WALs required by logical replication 
> slots
> +* are removed, the slots are unusable.  This setting prevents the
> +* 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.
> +*/
> +   if (GET_MAJOR_VERSION(cluster->major_version) >= 1700)
> +   appendPQExpBufferStr(, " -c
> max_slot_wal_keep_size=-1");
> 
> Should there be a check to throw an error if this option is specified
> or do we need some documentation that this option should not be
> specified?

Hmm, I don't think we have to add checks. Other settings, like 
synchronous_commit
and fsync, can be also overwritten, but pg_upgrade has never checked. Therefore,
it's user's responsibility to not set max_slot_wal_keep_size to a dangerous
value.

> 2) Because we are able to override max_slot_wal_keep_size there is a
> chance of slot getting invalidated and Assert being hit:
> +   /*
> +* 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.
> +*/
> +   if (*invalidated && SlotIsLogical(s) && IsBinaryUpgrade)
> +   {
> +   Assert(max_slot_wal_keep_size_mb == -1);
> +   elog(ERROR, "replication slots must not be
> invalidated during the upgrade");
> +   }

Hmm, so how about removing an assert and changing the error message more
appropriate? I still think it seldom occurs.

> 3) File 003_logical_replication_slots.pl is now changed to
> 003_upgrade_logical_replication_slots.pl, it should be change here too
> accordingly:
> index 5834513add..815d1a7ca1 100644
> --- a/src/bin/pg_upgrade/Makefile
> +++ b/src/bin/pg_upgrade/Makefile
> @@ -3,6 +3,9 @@
>  PGFILEDESC = "pg_upgrade - an in-place binary upgrade utility"
>  PGAPPICON = win32
> 
> +# required for 003_logical_replication_slots.pl
> +EXTRA_INSTALL=contrib/test_decoding
> +

Fixed.

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

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

RE: [PoC] pg_upgrade: allow to upgrade publisher node

[Hayato Kuroda (Fujitsu)]

Dear Peter,

Thanks for reviewing! PSA new version.

> ==
> src/bin/pg_upgrade/t/003_upgrade_logical_replication_slots.pl
> 
> 1.
> + # 2. max_replication_slots is set to smaller than the number of slots (2)
> + # present on the old cluster
> 
> SUGGESTION
> 2. Set 'max_replication_slots' to be less than the number of slots (2)
> present on the old cluster.

Fixed.

> 2.
> + # Set max_replication_slots to the same value as the number of slots. Both
> + # of slots will be used for subsequent tests.
> 
> SUGGESTION
> Set 'max_replication_slots' to match the number of slots (2) present
> on the old cluster.
> Both slots will be used for subsequent tests.

Fixed.

> 
> 3.
> + # 3. Emit a non-transactional message. test_slot2 detects the message so
> + # that this slot will be also reported by upcoming pg_upgrade.
> + $old_publisher->safe_psql('postgres',
> + "SELECT count(*) FROM pg_logical_emit_message('false', 'prefix',
> 'This is a non-transactional message');"
> + );
> 
> SUGGESTION
> 3. Emit a non-transactional message. This will cause test_slot2 to
> detect the unconsumed WAL record.

Fixed.

> 
> 4.
> + # Preparations for the subsequent test:
> + # 1. Generate extra WAL records. At this point neither test_slot1 nor
> + # test_slot2 has consumed them.
> + $old_publisher->start;
> + $old_publisher->safe_psql('postgres',
> + "CREATE TABLE tbl AS SELECT generate_series(1, 10) AS a;");
> +
> + # 2. Advance the slot test_slot2 up to the current WAL location, but
> + # test_slot1 still has unconsumed WAL records.
> + $old_publisher->safe_psql('postgres',
> + "SELECT pg_replication_slot_advance('test_slot2', NULL);");
> +
> + # 3. Emit a non-transactional message. test_slot2 detects the message so
> + # that this slot will be also reported by upcoming pg_upgrade.
> + $old_publisher->safe_psql('postgres',
> + "SELECT count(*) FROM pg_logical_emit_message('false', 'prefix',
> 'This is a non-transactional message');"
> + );
> +
> + $old_publisher->stop;
> 
> All of the above are sequentially executed on the
> old_publisher->safe_psql, so consider if it is worth combining them
> all in a single call (keeping the comments 1,2,3 separate still)
> 
> For example,
> 
> $old_publisher->start;
> $old_publisher->safe_psql('postgres', qq[
>   CREATE TABLE tbl AS SELECT generate_series(1, 10) AS a;
>   SELECT pg_replication_slot_advance('test_slot2', NULL);
>   SELECT count(*) FROM pg_logical_emit_message('false', 'prefix',
> 'This is a non-transactional message');
> ]);
> $old_publisher->stop;

Fixed.

> 
> 5.
> + # Clean up
> + $subscriber->stop();
> + $new_publisher->stop();
> 
> Should this also drop the 'test_slot1' and 'test_slot2'?

'test_slot1' and 'test_slot2' have already been removed while preparing in
"Successful upgrade" case. Also, I don't think objects have to be removed at the
end. It is tested by other parts, and it may make the test more difficult to
debug, if there are some failures.

> 6.
> +# Verify that logical replication slots cannot be migrated.  This function
> +# will be executed when the old cluster is PG16 and prior.
> +sub test_upgrade_from_pre_PG17
> +{
> + my ($old_publisher, $new_publisher, $mode) = @_;
> +
> + my $oldbindir = $old_publisher->config_data('--bindir');
> + my $newbindir = $new_publisher->config_data('--bindir');
> 
> SUGGESTION (let's not mention lots of different numbers; just refer to 17)
> This function will be executed when the old cluster version is prior to PG17.

Fixed.


> 7.
> + # Actual run, successful upgrade is expected
> + command_ok(
> + [
> + 'pg_upgrade', '--no-sync',
> + '-d', $old_publisher->data_dir,
> + '-D', $new_publisher->data_dir,
> + '-b', $oldbindir,
> + '-B', $newbindir,
> + '-s', $new_publisher->host,
> + '-p', $old_publisher->port,
> + '-P', $new_publisher->port,
> + $mode,
> + ],
> + 'run of pg_upgrade of old cluster');
> +
> + ok( !-d $new_publisher->data_dir . "/pg_upgrade_output.d",
> + "pg_upgrade_output.d/ removed after pg_upgrade success");
> 
> 7a.
> The comment is wrong?
> 
> SUGGESTION
> # pg_upgrade should NOT be successful

No, pg_uprade will success but no logical replication slots are migrated.
Comments docs were added.

> 7b.
> There is a blank line here before the ok() function, but in the other
> tests, there was none. Better to be consistent.

Removed.

> 8.
> + # Clean up
> + $new_publisher->stop();
> 
> Should this also drop the 'test_slot'?

I don't think so. Please see above.

> 
> 9.
> +# The TAP Cluster.pm assigns default 'max_wal_senders' and 'max_connections'
> to
> +# the same value (10) but PG12 and prior considered max_walsenders as a
> subset
> +# of max_connections, so setting the same value will fail.
> +if ($old_publisher->pg_version->major < 12)
> +{
> + $old_publisher->append_conf(
> + 'postgresql.conf', qq[
> + max_wal_senders = 5
> + max_connections = 10
> + ]);
> +}
> 
> If the comment is correct, then PG12 *and* prior, should be testing
> "<= 12", not "< 12". right?

I analyzed more and I was wrong - we 

Re: Special-case executor expression steps for common combinations

[Daniel Gustafsson]

> On 12 Oct 2023, at 19:52, Andres Freund  wrote:
> On 2023-10-12 13:24:27 +0300, Heikki Linnakangas wrote:
>> On 12/10/2023 12:48, Daniel Gustafsson wrote:

>>> The attached patch adds special-case expression steps for common sets of 
>>> steps
>>> in the executor to shave a few cycles off during execution, and make the JIT
>>> generated code simpler.
>>> 
>>> * Adds EEOP_FUNCEXPR_STRICT_1 and EEOP_FUNCEXPR_STRICT_2 for function calls 
>>> of
>>>   strict functions with 1 or 2 arguments (EEOP_FUNCEXPR_STRICT remains used 
>>> for
 2 arguments).
>>> * Adds EEOP_AGG_STRICT_INPUT_CHECK_ARGS_1 which is a special case for the
>>>   common case of one arg aggs.
>> 
>> Are these relevant when JITting? I'm a little sad if the JIT compiler cannot
>> unroll these on its own. Is there something we could do to hint it, so that
>> it could treat the number of arguments as a constant?
> 
> I think it's mainly important for interpreted execution.

Agreed.

>>>   skip extra setup for steps which are only interested in the side effects.
>> 
>> I'm a little surprised if this makes a measurable performance difference,
>> but sure, why not. It seems nice to be more explicit when you don't expect a
>> return value.

Right, performance benefits aside it does improve readability IMHO.

> IIRC this is more interesting for JIT than the above, because it allows LLVM
> to know that the return value isn't needed and thus doesn't need to be
> computed.

Correct, this is important to the JIT code which no longer has to perform two
Loads and one Store in order to get nothing, but can instead fastpath to
building a zero returnvalue.

--
Daniel Gustafsson

Re: Initial Schema Sync for Logical Replication

[vignesh C]

On Fri, 7 Jul 2023 at 12:41, Masahiko Sawada  wrote:
>
> On Wed, Jul 5, 2023 at 11:14 AM Masahiko Sawada  wrote:
> >
> > On Mon, Jun 19, 2023 at 5:29 PM Peter Smith  wrote:
> > >
> > > Hi,
> > >
> > > Below are my review comments for the PoC patch 0001.
> > >
> > > In addition,  the patch needed rebasing, and, after I rebased it
> > > locally in my private environment there were still test failures:
> > > a) The 'make check' tests fail but only in a minor way due to changes 
> > > colname
> > > b) the subscription TAP test did not work at all for me -- many errors.
> >
> > Thank you for reviewing the patch.
> >
> > While updating the patch, I realized that the current approach won't
> > work well or at least has the problem with partition tables. If a
> > publication has a partitioned table with publish_via_root = false, the
> > subscriber launches tablesync workers for its partitions so that each
> > tablesync worker copies data of each partition. Similarly, if it has a
> > partition table with publish_via_root = true, the subscriber launches
> > a tablesync worker for the parent table. With the current design,
> > since the tablesync worker is responsible for both schema and data
> > synchronization for the target table, it won't be possible to
> > synchronize both the parent table's schema and partitions' schema. For
> > example, there is no pg_subscription_rel entry for the parent table if
> > the publication has publish_via_root = false. In addition to that, we
> > need to be careful about the order of synchronization of the parent
> > table and its partitions. We cannot start schema synchronization for
> > partitions before its parent table. So it seems to me that we need to
> > consider another approach.
>
> So I've implemented a different approach; doing schema synchronization
> at a CREATE SUBSCRIPTION time. The backend executing CREATE
> SUBSCRIPTION uses pg_dump and restores the table schemas including
> both partitioned tables and their partitions regardless of
> publish_via_partition_root option, and then creates
> pg_subscription_rel entries for tables while respecting
> publish_via_partition_root option.
>
> There is a window between table creations and the tablesync workers
> starting to process the tables. If DDLs are executed in this window,
> the tablesync worker might fail because the table schema might have
> already been changed. We need to mention this note in the
> documentation. BTW, I think we will be able to get rid of this
> downside if we support DDL replication. DDLs executed in the window
> are applied by the apply worker and it takes over the data copy to the
> tablesync worker at a certain LSN.
>
> I've attached PoC patches. It has regression tests but doesn't have
> the documentation yet.

Few thoughts:
1) There might be a scenario where we will create multiple
subscriptions with the tables overlapping across the subscription, in
that case, the table will be present when the 2nd subscription is
being created, can we do something in this case:
+   /*
+* Error if the table is already present on the
subscriber. Please note
+* that concurrent DDLs can create the table as we
don't acquire any lock
+* on the table.
+*
+* XXX: do we want to overwrite it (or optionally)?
+*/
+   if (OidIsValid(RangeVarGetRelid(rv, AccessShareLock, true)))
+   ereport(ERROR,
+   (errmsg("existing table %s
cannot synchronize table schema",
+   rv->relname)));

2) Should we clean the replication slot in case of failures, currently
the replication slot is left over.

3) Is it expected that all of the dependencies like type/domain etc
should be created by the user before creating a subscription with
copy_schema, currently we are taking care of creating the sequences
for tables, is this an exception?

4) If a column list publication is created, currently we are getting
all of the columns, should we get only the specified columns in this
case?

Regards,
Vignesh

Re: Initial Schema Sync for Logical Replication

[vignesh C]

On Thu, 31 Aug 2023 at 17:18, Kumar, Sachin  wrote:
>
> Hi Everyone, based on internal discussion with Masahiko
> I have implemented concurrent DDL support for initial schema sync.
>
> Concurrent Patch workflow
>
> 1. When TableSync worker creates a replicaton slot, It will
> save the slot lsn into pg_subscription_rel with
> SUBREL_SYNC_SCHEMA_DATA_SYNC state, and it will wait for
> its state to be SUBREL_STATE_DATASYNC.
>
> 2. Applier process will apply DDLs till tablesync lsn, and then
> it will change pg_subscription_rel state to SUBREL_STATE_DATASYNC.
>
> 3. TableSync will continue applying pending DML/DDls till it catch up.
>
> This patch needs DDL replication to apply concurrent DDLs, I have cherry-
> picked this DDL patch [0]

Can you rebase the patch and post the complete set of required changes
for the concurrent DDL, I will have a look at them.

Regards,
Vignesh

[patch] pg_basebackup: mention that spread checkpoints are the default in --help

[Michael Banck]

Hi,

I believed that spread (not fast) checkpoints are the default in
pg_basebackup, but noticed that --help does not specify which is which -
contrary to the reference documentation.

So I propose the small attached patch to clarify that.


Michael
>From 2fc49eae5ccc82e144c3f683689757e014e331bd Mon Sep 17 00:00:00 2001
From: Michael Banck 
Date: Thu, 19 Oct 2023 11:37:11 +0200
Subject: [PATCH] pg_basebackup: Mention that spread checkpoints are the
 default in --help

---
 src/bin/pg_basebackup/pg_basebackup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 1a8cef345d..f2cf38a773 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -407,7 +407,7 @@ usage(void)
 	printf(_("  -Z, --compress=nonedo not compress tar output\n"));
 	printf(_("\nGeneral options:\n"));
 	printf(_("  -c, --checkpoint=fast|spread\n"
-			 " set fast or spread checkpointing\n"));
+			 " set fast or spread (default) checkpointing\n"));
 	printf(_("  -C, --create-slot  create replication slot\n"));
 	printf(_("  -l, --label=LABEL  set backup label\n"));
 	printf(_("  -n, --no-clean do not clean up after errors\n"));
-- 
2.39.2

Re: Use virtual tuple slot for Unique node

[David Rowley]

On Thu, 12 Oct 2023 at 23:06, Ashutosh Bapat
 wrote:
> Q7 select distinct a,b from (select string_agg(left(a, 100), ', ')
> over (order by a rows 2 preceding) a, b from t_text) q
> HEAD: 16070.62 ms
> patched: 16182.16 ms

Did you time the SELECT or EXPLAIN ANALYZE?

With SELECT, I'm unable to recreate this slowdown. Using your setup:

$ cat bench.sql
set enable_hashagg=0;
set work_mem='10GB';
select distinct a,b from (select string_agg(left(a, 100), ', ') over
(order by a rows 2 preceding) a, b from t_text) q;

Master @ 13d00729d
$ pgbench -n -f bench.sql -T 300 postgres | grep latency
latency average = 7739.250 ms

Master + use_subnode_slot_type_for_nodeunique.patch
$ pgbench -n -f bench.sql -T 300 postgres | grep latency
latency average = 7718.007 ms

It's hard to imagine why there would be a slowdown as this query uses
a TTSOpsMinimalTuple slot type in the patch and the unpatched version.

David

RE: [PoC] pg_upgrade: allow to upgrade publisher node

[Hayato Kuroda (Fujitsu)]

Dear Shlok,

Thanks for testing the feature!

> 
> I tested a test scenario:
> I started a new publisher with 'max_replication_slots' parameter set
> to '1' and created a streaming replication with the new publisher as
> primary node.

Just to confirm what you did - you set up a physical replication and the
target of pg_upgrade was set to the primary, right?

I think we can assume that new cluster (target of pg_upgrade) is not used yet.
The documentation describes the usage [1] and it says that we must initialize
the cluster (at step 4) and then run the pg_upgrade (at step 10).

Therefore I don't think we should document anything about it.

[1]: 
https://www.postgresql.org/docs/devel/pgupgrade.html#:~:text=Initialize%20the%20new%20PostgreSQL%20cluster

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

Re: BRIN minmax multi - incorrect distance for infinite timestamp/date

[Ashutosh Bapat]

On Thu, Oct 19, 2023 at 2:31 PM Tomas Vondra
 wrote:

>
> Does that explain the algorithm? I'm not against clarifying the comment,
> of course.

Thanks a lot for this explanation. It's clear now.

> I tried to do that, but I ran into troubles with the "date" tests. I
> needed to build values that close to the min/max values, so I did
> something like
>
> SELECT '4713-01-01 BC'::date + (i || ' days')::interval FROM
> generate_series(1,10) s(i);
>
> And then the same for the max date, but that fails because of the
> date/timestamp conversion in date plus operator.
>
> However, maybe two simple generate_series() would work ...
>

Something like this? select i::date from  generate_series('4713-02-01
BC'::date,  '4713-01-01 BC'::date, '-1 day'::interval) i;
   i
---
 4713-02-01 BC
 4713-01-31 BC
 4713-01-30 BC
 4713-01-29 BC
 4713-01-28 BC
 4713-01-27 BC
 4713-01-26 BC
 4713-01-25 BC
 4713-01-24 BC
 4713-01-23 BC
 4713-01-22 BC
 4713-01-21 BC
 4713-01-20 BC
 4713-01-19 BC
 4713-01-18 BC
 4713-01-17 BC
 4713-01-16 BC
 4713-01-15 BC
 4713-01-14 BC
 4713-01-13 BC
 4713-01-12 BC
 4713-01-11 BC
 4713-01-10 BC
 4713-01-09 BC
 4713-01-08 BC
 4713-01-07 BC
 4713-01-06 BC
 4713-01-05 BC
 4713-01-04 BC
 4713-01-03 BC
 4713-01-02 BC
 4713-01-01 BC
(32 rows)

-- 
Best Wishes,
Ashutosh Bapat

Re: BRIN minmax multi - incorrect distance for infinite timestamp/date

[Tomas Vondra]

On 10/19/23 09:04, Dean Rasheed wrote:
> On Thu, 19 Oct 2023, 05:32 Ashutosh Bapat,  > wrote:
> 
> On Wed, Oct 18, 2023 at 8:23 PM Tomas Vondra
>  > wrote:
> 
> >
> > I did use that many values to actually force "compaction" and
> merging of
> > points into ranges. We only keep 32 values per page range, so with 2
> > values we'll not build a range. You're right it may still trigger the
> > overflow (we probably still calculate distances, I didn't realize
> that),
> > but without the compaction we can't check the query plans.
> >
> > However, I agree 60 values may be a bit too much. And I realized
> we can
> > reduce the count quite a bit by using the values_per_range option. We
> > could set it to 8 (which is the minimum).
> >
> 
> I haven't read BRIN code, except the comments in the beginning of the
> file. From what you describe it seems we will store first 32 values as
> is, but later as the number of values grow create ranges from those?
> Please point me to the relevant source code/documentation. Anyway, if
> we can reduce the number of rows we insert, that will be good.
> 
> 
> I don't think 60 values is excessive, but instead of listing them out by
> hand, perhaps use generate_series().
> 

I tried to do that, but I ran into troubles with the "date" tests. I
needed to build values that close to the min/max values, so I did
something like

SELECT '4713-01-01 BC'::date + (i || ' days')::interval FROM
generate_series(1,10) s(i);

And then the same for the max date, but that fails because of the
date/timestamp conversion in date plus operator.

However, maybe two simple generate_series() would work ...

regards

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

Re: BRIN minmax multi - incorrect distance for infinite timestamp/date

[Tomas Vondra]

On 10/19/23 06:32, Ashutosh Bapat wrote:
> On Wed, Oct 18, 2023 at 8:23 PM Tomas Vondra
>  wrote:
> 
>>
>> I did use that many values to actually force "compaction" and merging of
>> points into ranges. We only keep 32 values per page range, so with 2
>> values we'll not build a range. You're right it may still trigger the
>> overflow (we probably still calculate distances, I didn't realize that),
>> but without the compaction we can't check the query plans.
>>
>> However, I agree 60 values may be a bit too much. And I realized we can
>> reduce the count quite a bit by using the values_per_range option. We
>> could set it to 8 (which is the minimum).
>>
> 
> I haven't read BRIN code, except the comments in the beginning of the
> file. From what you describe it seems we will store first 32 values as
> is, but later as the number of values grow create ranges from those?
> Please point me to the relevant source code/documentation. Anyway, if
> we can reduce the number of rows we insert, that will be good.
> 

I don't think we have documentation other than what's at the beginning
of the file. What the comment tries to explain is that the summary has a
maximum size (32 values by default), and each value can be either a
point or a range. A point requires one value, range requires two. So we
accumulate values one by one - until 32 values that's fine. Once we get
33, we have to merge some of the points into ranges, and we do that in a
greedy way by distance.

For example, this may happen:

33 values
-> 31 values + 1 range  [requires 33]
-> 30 values + 1 range  [requires 32]
...

The exact steps depend on which values/ranges are picked for the merge,
of course. In any case, there's no difference between the initial 32
values and the values added later.

Does that explain the algorithm? I'm not against clarifying the comment,
of course.

>>>
>>
>> Not sure what you mean by "crash". Yes, it doesn't hit an assert,
>> because there's none when calculating distance for date. It however
>> should fail in the query plan check due to the incorrect order of
>> subtractions.
>>
>> Also, the commit message does not claim to fix overflow. In fact, it
>> says it can't overflow ...
>>
> 
> 
> Reading the commit message
> "Tests for overflows with dates and timestamps in BRIN ...
> 
> ...
> 
> The new regression tests check this for date and timestamp data types.
> It adds tables with data close to the allowed min/max values, and builds
> a minmax-multi index on it."
> 
> I expected the CREATE INDEX statement to throw an error or fail the
> "Assert(delta >= 0);" in brin_minmax_multi_distance_date(). But a
> later commit mentions that the overflow is not possible.
> 

Hmmm, yeah. The comment should mention the date doesn't have issue with
overflows, but other bugs.


regards

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

Re: pgsql: Generate automatically code and documentation related to wait ev

[Michael Paquier]

On Thu, Oct 19, 2023 at 09:38:30AM +0200, Christoph Berg wrote:
> Thanks for the lightning-fast fix :)

No pb.  Not the first one, not the last one.  ;)
--
Michael


signature.asc
Description: PGP signature

Re: list of acknowledgments for PG16

[Peter Eisentraut]

On 16.10.23 15:46, Alvaro Herrera wrote:

On 2023-Aug-27, Peter Eisentraut wrote:


On 22.08.23 15:48, Vik Fearing wrote:



I think these might be the same person:

      Zhihong Yu
      Zihong Yu

I did not spot any others.


Fixed.


Hm, I noticed we also list Ted Yu, but that's the same person as Zhihong Yu.


fixed

Re: [PoC] pg_upgrade: allow to upgrade publisher node

[Shlok Kyal]

I tested a test scenario:
I started a new publisher with 'max_replication_slots' parameter set
to '1' and created a streaming replication with the new publisher as
primary node.
Then I did a pg_upgrade from old publisher to new publisher. The
upgrade failed with following error:

Restoring logical replication slots in the new cluster
SQL command failed
SELECT * FROM pg_catalog.pg_create_logical_replication_slot('test1',
'pgoutput', false, false);
ERROR:  all replication slots are in use
HINT:  Free one or increase max_replication_slots.

Failure, exiting

Should we document that the existing replication slots are taken in
consideration while setting 'max_replication_slots' value in the new
publisher?

Thanks
Shlok Kumar Kyal

On Wed, 18 Oct 2023 at 15:01, Hayato Kuroda (Fujitsu)
 wrote:
>
> Dear Peter,
>
> Thank you for reviewing! PSA new version.
> Note that 0001 and 0002 are combined into one patch.
>
> > Here are some review comments for v51-0001
> >
> > ==
> > src/bin/pg_upgrade/check.c
> >
> > 0.
> > +check_old_cluster_for_valid_slots(bool live_check)
> > +{
> > + char output_path[MAXPGPATH];
> > + FILE*script = NULL;
> > +
> > + prep_status("Checking for valid logical replication slots");
> > +
> > + snprintf(output_path, sizeof(output_path), "%s/%s",
> > + log_opts.basedir,
> > + "invalid_logical_relication_slots.txt");
> >
> > 0a
> > typo /invalid_logical_relication_slots/invalid_logical_replication_slots/
>
> Fixed.
>
> > 0b.
> > Since the non-upgradable slots are not strictly "invalid", is this an
> > appropriate filename for the bad ones?
> >
> > But I don't have very good alternatives. Maybe:
> > - non_upgradable_logical_replication_slots.txt
> > - problem_logical_replication_slots.txt
>
> Per discussion [1], I kept current style.
>
> > src/bin/pg_upgrade/t/003_upgrade_logical_replication_slots.pl
> >
> > 1.
> > +# --
> > +# TEST: Confirm pg_upgrade fails when wrong GUC is set on new cluster
> > +#
> > +# There are two requirements for GUCs - wal_level and 
> > max_replication_slots,
> > +# but only max_replication_slots will be tested here. This is because to
> > +# reduce the execution time of the test.
> >
> >
> > SUGGESTION
> > # TEST: Confirm pg_upgrade fails when the new cluster has wrong GUC values.
> > #
> > # Two GUCs are required - 'wal_level' and 'max_replication_slots' - but to
> > # reduce the test execution time, only 'max_replication_slots' is tested 
> > here.
>
> First part was fixed. Second part was removed per [1].
>
> > 2.
> > +# Preparations for the subsequent test:
> > +# 1. Create two slots on the old cluster
> > +$old_publisher->start;
> > +$old_publisher->safe_psql('postgres',
> > + "SELECT pg_create_logical_replication_slot('test_slot1',
> > 'test_decoding', false, true);"
> > +);
> > +$old_publisher->safe_psql('postgres',
> > + "SELECT pg_create_logical_replication_slot('test_slot2',
> > 'test_decoding', false, true);"
> > +);
> >
> >
> > Can't you combine those SQL in the same $old_publisher->safe_psql.
>
> Combined.
>
> > 3.
> > +# Clean up
> > +rmtree($new_publisher->data_dir . "/pg_upgrade_output.d");
> > +# Set max_replication_slots to the same value as the number of slots. Both 
> > of
> > +# slots will be used for subsequent tests.
> > +$new_publisher->append_conf('postgresql.conf', "max_replication_slots = 
> > 1");
> >
> > The code doesn't seem to match the comment - is this correct? The
> > old_publisher created 2 slots, so why are you setting new_publisher
> > "max_replication_slots = 1" again?
>
> Fixed to "max_replication_slots = 2" Note that previous test worked well 
> because
> GUC checking on new cluster is done after checking the status of slots.
>
> > 4.
> > +# Preparations for the subsequent test:
> > +# 1. Generate extra WAL records. Because these WAL records do not get
> > consumed
> > +# it will cause the upcoming pg_upgrade test to fail.
> > +$old_publisher->start;
> > +$old_publisher->safe_psql('postgres',
> > + "CREATE TABLE tbl AS SELECT generate_series(1, 10) AS a;");
> > +
> > +# 2. Advance the slot test_slot2 up to the current WAL location
> > +$old_publisher->safe_psql('postgres',
> > + "SELECT pg_replication_slot_advance('test_slot2', NULL);");
> > +
> > +# 3. Emit a non-transactional message. test_slot2 detects the message so 
> > that
> > +# this slot will be also reported by upcoming pg_upgrade.
> > +$old_publisher->safe_psql('postgres',
> > + "SELECT count(*) FROM pg_logical_emit_message('false', 'prefix',
> > 'This is a non-transactional message');"
> > +);
> >
> >
> > I felt this test would be clearer if you emphasised the state of the
> > test_slot1 also. e.g.
> >
> > 4a.
> > BEFORE
> > +# 1. Generate extra WAL records. Because these WAL records do not get
> > consumed
> > +# it will cause the upcoming pg_upgrade test to fail.
> >
> > SUGGESTION
> > # 1. Generate extra WAL records. At this point neither test_slot1 nor 
> > test_slot2
> > #has consumed them.
>
> Fixed.
>
> > 4b.
> > BEFORE

Re: pgsql: Generate automatically code and documentation related to wait ev

[Christoph Berg]

Re: Michael Paquier
> Will fix as per the attached.  Thanks for the report.

Thanks for the lightning-fast fix :)

Christoph

Re: BRIN minmax multi - incorrect distance for infinite timestamp/date

[Dean Rasheed]

On Thu, 19 Oct 2023, 05:32 Ashutosh Bapat, 
wrote:

> On Wed, Oct 18, 2023 at 8:23 PM Tomas Vondra
>  wrote:
>
> >
> > I did use that many values to actually force "compaction" and merging of
> > points into ranges. We only keep 32 values per page range, so with 2
> > values we'll not build a range. You're right it may still trigger the
> > overflow (we probably still calculate distances, I didn't realize that),
> > but without the compaction we can't check the query plans.
> >
> > However, I agree 60 values may be a bit too much. And I realized we can
> > reduce the count quite a bit by using the values_per_range option. We
> > could set it to 8 (which is the minimum).
> >
>
> I haven't read BRIN code, except the comments in the beginning of the
> file. From what you describe it seems we will store first 32 values as
> is, but later as the number of values grow create ranges from those?
> Please point me to the relevant source code/documentation. Anyway, if
> we can reduce the number of rows we insert, that will be good.
>

I don't think 60 values is excessive, but instead of listing them out by
hand, perhaps use generate_series().

Regards,
Dean

Re: [PoC] pg_upgrade: allow to upgrade publisher node

[vignesh C]

On Wed, 18 Oct 2023 at 14:55, Hayato Kuroda (Fujitsu)
 wrote:
>
> Dear Peter,
>
> Thank you for reviewing! PSA new version.
> Note that 0001 and 0002 are combined into one patch.
>
> > Here are some review comments for v51-0001
> >
> > ==
> > src/bin/pg_upgrade/check.c
> >
> > 0.
> > +check_old_cluster_for_valid_slots(bool live_check)
> > +{
> > + char output_path[MAXPGPATH];
> > + FILE*script = NULL;
> > +
> > + prep_status("Checking for valid logical replication slots");
> > +
> > + snprintf(output_path, sizeof(output_path), "%s/%s",
> > + log_opts.basedir,
> > + "invalid_logical_relication_slots.txt");
> >
> > 0a
> > typo /invalid_logical_relication_slots/invalid_logical_replication_slots/
>
> Fixed.
>
> > 0b.
> > Since the non-upgradable slots are not strictly "invalid", is this an
> > appropriate filename for the bad ones?
> >
> > But I don't have very good alternatives. Maybe:
> > - non_upgradable_logical_replication_slots.txt
> > - problem_logical_replication_slots.txt
>
> Per discussion [1], I kept current style.
>
> > src/bin/pg_upgrade/t/003_upgrade_logical_replication_slots.pl
> >
> > 1.
> > +# --
> > +# TEST: Confirm pg_upgrade fails when wrong GUC is set on new cluster
> > +#
> > +# There are two requirements for GUCs - wal_level and 
> > max_replication_slots,
> > +# but only max_replication_slots will be tested here. This is because to
> > +# reduce the execution time of the test.
> >
> >
> > SUGGESTION
> > # TEST: Confirm pg_upgrade fails when the new cluster has wrong GUC values.
> > #
> > # Two GUCs are required - 'wal_level' and 'max_replication_slots' - but to
> > # reduce the test execution time, only 'max_replication_slots' is tested 
> > here.
>
> First part was fixed. Second part was removed per [1].
>
> > 2.
> > +# Preparations for the subsequent test:
> > +# 1. Create two slots on the old cluster
> > +$old_publisher->start;
> > +$old_publisher->safe_psql('postgres',
> > + "SELECT pg_create_logical_replication_slot('test_slot1',
> > 'test_decoding', false, true);"
> > +);
> > +$old_publisher->safe_psql('postgres',
> > + "SELECT pg_create_logical_replication_slot('test_slot2',
> > 'test_decoding', false, true);"
> > +);
> >
> >
> > Can't you combine those SQL in the same $old_publisher->safe_psql.
>
> Combined.
>
> > 3.
> > +# Clean up
> > +rmtree($new_publisher->data_dir . "/pg_upgrade_output.d");
> > +# Set max_replication_slots to the same value as the number of slots. Both 
> > of
> > +# slots will be used for subsequent tests.
> > +$new_publisher->append_conf('postgresql.conf', "max_replication_slots = 
> > 1");
> >
> > The code doesn't seem to match the comment - is this correct? The
> > old_publisher created 2 slots, so why are you setting new_publisher
> > "max_replication_slots = 1" again?
>
> Fixed to "max_replication_slots = 2" Note that previous test worked well 
> because
> GUC checking on new cluster is done after checking the status of slots.
>
> > 4.
> > +# Preparations for the subsequent test:
> > +# 1. Generate extra WAL records. Because these WAL records do not get
> > consumed
> > +# it will cause the upcoming pg_upgrade test to fail.
> > +$old_publisher->start;
> > +$old_publisher->safe_psql('postgres',
> > + "CREATE TABLE tbl AS SELECT generate_series(1, 10) AS a;");
> > +
> > +# 2. Advance the slot test_slot2 up to the current WAL location
> > +$old_publisher->safe_psql('postgres',
> > + "SELECT pg_replication_slot_advance('test_slot2', NULL);");
> > +
> > +# 3. Emit a non-transactional message. test_slot2 detects the message so 
> > that
> > +# this slot will be also reported by upcoming pg_upgrade.
> > +$old_publisher->safe_psql('postgres',
> > + "SELECT count(*) FROM pg_logical_emit_message('false', 'prefix',
> > 'This is a non-transactional message');"
> > +);
> >
> >
> > I felt this test would be clearer if you emphasised the state of the
> > test_slot1 also. e.g.
> >
> > 4a.
> > BEFORE
> > +# 1. Generate extra WAL records. Because these WAL records do not get
> > consumed
> > +# it will cause the upcoming pg_upgrade test to fail.
> >
> > SUGGESTION
> > # 1. Generate extra WAL records. At this point neither test_slot1 nor 
> > test_slot2
> > #has consumed them.
>
> Fixed.
>
> > 4b.
> > BEFORE
> > +# 2. Advance the slot test_slot2 up to the current WAL location
> >
> > SUGGESTION
> > # 2. Advance the slot test_slot2 up to the current WAL location, but 
> > test_slot2
> > #still has unconsumed WAL records.
>
> IIUC, test_slot2 is caught up by pg_replication_slot_advance('test_slot2'). I 
> think
> "but test_slot1 still has unconsumed WAL records." is appropriate. Fixed.
>
> > 5.
> > +# pg_upgrade will fail because the slot still has unconsumed WAL records
> > +command_checks_all(
> >
> > /because the slot still has/because there are slots still having/
>
> Fixed.
>
> > 6.
> > + [qr//],
> > + 'run of pg_upgrade of old cluster with slot having unconsumed WAL records'
> > +);
> >
> > /slot/slots/
>
> Fixed.
>
> > 7.

Re: [PoC] pg_upgrade: allow to upgrade publisher node

[Shlok Kyal]

> Few comments:
> 1) We will be able to override the value of max_slot_wal_keep_size by
> using --new-options like '--new-options  "-c
> max_slot_wal_keep_size=val"':
> +   /*
> +* Use max_slot_wal_keep_size as -1 to prevent the WAL removal by the
> +* checkpointer process.  If WALs required by logical replication 
> slots
> +* are removed, the slots are unusable.  This setting prevents the
> +* 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.
> +*/
> +   if (GET_MAJOR_VERSION(cluster->major_version) >= 1700)
> +   appendPQExpBufferStr(, " -c
> max_slot_wal_keep_size=-1");
>
> Should there be a check to throw an error if this option is specified
> or do we need some documentation that this option should not be
> specified?

I have tested the above scenario. We are able to override the
max_slot_wal_keep_size by using  '--new-options  "-c
max_slot_wal_keep_size=val"'. And also with some insert statements
during pg_upgrade, old WAL file were deleted and logical replication
slots were invalidated. Since the slots were invalidated replication
was not happening after the upgrade.

Thanks,
Shlok Kumar Kyal

Re: More new SQL/JSON item methods

[Jeevan Chalke]

On Wed, Oct 18, 2023 at 4:50 PM jian he  wrote:

> On Fri, Oct 6, 2023 at 7:47 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?
> >
> > > v1-0002-Implement-.date-.time-.time_tz-.timestamp-and-.ti.patch
> > >
> > > This commit implements jsonpath .date(), .time(), .time_tz(),
> > > .timestamp(), .timestamp_tz() methods.  The JSON string representing
> > > a valid date/time is converted to the specific date or time type
> > > representation.
> > >
> > > The changes use the infrastructure of the .datetime() method and
> > > perform the datatype conversion as appropriate.  All these methods
> > > accept no argument and use ISO datetime formats.
> >
> > These should accept an optional precision argument.  Did you plan to add
> > that?
>
> compiler warnings issue resolved.
>

Thanks for pitching in, Jian.
I was slightly busy with other stuff and thus could not spend time on this.

I will start looking into it and expect a patch in a couple of days.


> I figured out how to use the precision argument.
> But I don't know how to get the precision argument in the parse stage.
>
> attached is my attempt to implement: select
> jsonb_path_query('"2017-03-10 11:11:01.123"', '$.timestamp(2)');
> not that familiar with src/backend/utils/adt/jsonpath_gram.y. imitate
> decimal method failed. decimal has precision and scale two arguments.
> here only one argument.
>
> looking for hints.
>

You may refer to how .datetime() is implemented.

Thanks


-- 
Jeevan Chalke

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



edbpostgres.com

Re: More new SQL/JSON item methods

[Jeevan Chalke]

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.


>
> > v1-0002-Implement-.date-.time-.time_tz-.timestamp-and-.ti.patch
> >
> > This commit implements jsonpath .date(), .time(), .time_tz(),
> > .timestamp(), .timestamp_tz() methods.  The JSON string representing
> > a valid date/time is converted to the specific date or time type
> > representation.
> >
> > The changes use the infrastructure of the .datetime() method and
> > perform the datatype conversion as appropriate.  All these methods
> > accept no argument and use ISO datetime formats.
>
> These should accept an optional precision argument.  Did you plan to add
> that?
>

Yeah, will add that.


>
> > v1-0003-Implement-jsonpath-.boolean-and-.string-methods.patch
> >
> > This commit implements jsonpath .boolean() and .string() methods.
>
> This contains a compiler warning:
>
> ../src/backend/utils/adt/jsonpath_exec.c: In function
> 'executeItemOptUnwrapTarget':
> ../src/backend/utils/adt/jsonpath_exec.c:1162:86: error: 'tmp' may be
> used uninitialized [-Werror=maybe-uninitialized]
>
> > v1-0004-Implement-jasonpath-.decimal-precision-scale-meth.patch
> >
> > This commit implements jsonpath .decimal() method with optional
> > precision and scale.  If precision and scale are provided, then
> > it is converted to the equivalent numerictypmod and applied to the
> > numeric number.
>
> This also contains compiler warnings:
>

Thanks, for reporting these warnings. I don't get those on my machine, thus
missed them. Will fix them.


>
> ../src/backend/utils/adt/jsonpath_exec.c: In function
> 'executeItemOptUnwrapTarget':
> ../src/backend/utils/adt/jsonpath_exec.c:1403:53: error: declaration of
> 'numstr' shadows a previous local [-Werror=shadow=compatible-local]
> ../src/backend/utils/adt/jsonpath_exec.c:1442:54: error: declaration of
> 'elem' shadows a previous local [-Werror=shadow=compatible-local]
>
> There is a typo in the commit message: "Implement jasonpath"
>

Will fix.


>
> Any reason this patch is separate from 0002?  Isn't number() and
> decimal() pretty similar?
>

Since DECIMAL has precision and scale arguments, I have implemented that at
the end. I tried merging that with 0001, but other patches ended up with
the conflicts and thus I didn't merge that and kept it as a separate patch.
But yes, logically it belongs to the 0001 group. My bad that I haven't put
in that extra effort. Will do that in the next version. Sorry for the same.


>
> You could also update src/backend/catalog/sql_features.txt in each patch
> (features T865 through T878).
>

OK.

Thanks

-- 
Jeevan Chalke

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



edbpostgres.com