Re: [PATCH] Add native windows on arm64 support

[Michael Paquier]

On Thu, Aug 31, 2023 at 01:06:53AM -0400, Tom Lane wrote:
> That's a good point.  The hardware resources to support a buildfarm
> animal are really pretty trivial these days.  What is important is
> an animal owner who is willing to help chase down problems when they
> arise.

Unfortunately I don't see myself running that at home.  It is too hot
and humid in Summer so the machine would not last.  Something in the
cloud would be OK for me, but AWS has no option for a Windows host
with ARM yet.
--
Michael


signature.asc
Description: PGP signature

Re: should frontend tools use syncfs() ?

[Michael Paquier]

On Tue, Aug 29, 2023 at 06:14:08PM -0700, Nathan Bossart wrote:
> That seems fair enough.  I did this in v7.  I restructured fsync_pgdata()
> and fsync_dir_recurse() so that any new sync methods should cause compiler
> warnings until they are implemented.

That's pretty cool and easier to maintain in the long term.

After sleeping on it, there are two things that popped up in my mind
that may be worth considering:
- Should we have some regression tests?  We should only need one test
in one of the binaries to be able to stress the new code paths of
file_utils.c with syncfs.   The cheapest one may be pg_dump with a
dump in directory format?  Note that we have tests there that depend
on lz4 or gzip existing, which are conditional.
- Perhaps 0002 should be split into two parts?  The first patch could
introduce DataDirSyncMethod in file_utils.h with the new routines in
file_utils.h (including syncfs support), and the second patch would
plug the new option to all the binaries.  In the first patch, I would
hardcode DATA_DIR_SYNC_METHOD_FSYNC.
--
Michael


signature.asc
Description: PGP signature

Re: [17] CREATE SUBSCRIPTION ... SERVER

[Ashutosh Bapat]

On Wed, Aug 30, 2023 at 9:00 PM Jeff Davis  wrote:
>
> On Wed, 2023-08-30 at 19:11 +0530, Ashutosh Bapat wrote:
> > Are you suggesting that SERVERs created with FDW can not be used as
> > publishers?
>
> Correct. Without that, how would the subscription know that the FDW
> contains valid postgres connection information? I suppose it could
> create a connection string out of the options itself and do another
> round of validation, is that what you had in mind?

The server's FDW has to be postgres_fdw. So we have to handle the
awkward dependency between core and postgres_fdw (an extension). The
connection string should be created from options itself. A special
user mapping for replication may be used. That's how I see it at a
high level.

-- 
Best Wishes,
Ashutosh Bapat

Re: [PATCH] Add native windows on arm64 support

[Tom Lane]

Thomas Munro  writes:
> But, supposing Azure credits were suddenly available (just between
> you, me and the mailing list, I heard that this should be imminent as
> some kind of grant to PG US, as a result of the Cirrus thing), do we
> have a Windows-savvy volunteer to feed and water an animal?  I feel
> like this case is inevitable anyway because people in our own
> community are probably going to be using Windows ARM laptops sooner or
> later...

That's a good point.  The hardware resources to support a buildfarm
animal are really pretty trivial these days.  What is important is
an animal owner who is willing to help chase down problems when they
arise.

Maybe we just need to wait for some community members to acquire
those laptops.

regards, tom lane

Re: [PATCH] Add native windows on arm64 support

[Thomas Munro]

On Thu, Aug 31, 2023 at 4:22 PM Michael Paquier  wrote:
> Honestly, I am not sure how to proceed here.  Having something in the
> buildfarm is necessary IMO, because this is the central place that
> community members look at when it comes to monitoring the stability of
> a patch committed.

It's tricky, because usually the org that wants the thing to work
supplies the thing, and I personally think that provides a meaningful
"demand" signal too.  For example, we have external Solaris
representation, but no users of AIX represented at this point, and I
consider that to be quite interesting information.  Plenty of people
want PG to work well on their OS or architecture.

But, supposing Azure credits were suddenly available (just between
you, me and the mailing list, I heard that this should be imminent as
some kind of grant to PG US, as a result of the Cirrus thing), do we
have a Windows-savvy volunteer to feed and water an animal?  I feel
like this case is inevitable anyway because people in our own
community are probably going to be using Windows ARM laptops sooner or
later...

Re: [PATCH] Add native windows on arm64 support

[Tom Lane]

Michael Paquier  writes:
> On Wed, Aug 30, 2023 at 04:12:16PM +0100, Anthony Roberts wrote:
>> Just a follow-up: having spoken to the relevant people, sadly, right now,
>> we do not have the capacity to be pulling machines out of our day-to-day CI
>> tasks to dedicate to specific projects.

> Okay, thanks for letting us know.

Too bad.

> Honestly, I am not sure how to proceed here.  Having something in the
> buildfarm is necessary IMO, because this is the central place that
> community members look at when it comes to monitoring the stability of
> a patch committed.
> Any opinions from the others?

I agree.  I'm really uncomfortable with claiming support for
Windows-on-ARM if we don't have a buildfarm member testing it.
For other platforms that have a track record of multiple
hardware support, it might not be a stretch ... but Windows was
so resolutely Intel-only for so long that "it works on ARM" is
a proposition that I won't trust without hard evidence.  There
are too many bits of that system that might not have gotten the
word yet, or at least not gotten sufficient testing.

My vote for this is we don't commit without a buildfarm member.

(As a comparison point, I'd still be unsure about our support
for macOS-on-ARM if we didn't have buildfarm support for that.
That exists, and is being paid for out of my own pocket.
I do not have much sympathy for a corporation that claims
it can't afford to support one measly buildfarm animal.)

regards, tom lane

Re: New WAL record to detect the checkpoint redo location

[Michael Paquier]

On Thu, Aug 31, 2023 at 09:55:45AM +0530, Dilip Kumar wrote:
> Yeah, good catch.  With this, it seems like we can not move this new
> WAL Insert out of the Exclusive WAL insertion lock right?  Because if
> we want to set the LSN of this record as the checkpoint.redo then
> there should not be any concurrent insertion until we expose the
> XLogCtl->Insert.RedoRecPtr.  Otherwise, we will miss the FPW for all
> the record which has been inserted after the checkpoint.redo before
> we acquired the exclusive WAL insertion lock.

Yes.

> So maybe I need to restart from the first version of the patch but
> instead of moving the insertion of the new record out of the exclusive
> lock need to do some better refactoring so that XLogInsertRecord()
> doesn't look ugly.

Yes, I am not sure which interface would be less ugli-ish, but that's
enough material for a refactoring patch of the WAL insert routines on
top of the main patch that introduces the REDO record.
--
Michael


signature.asc
Description: PGP signature

Re: New WAL record to detect the checkpoint redo location

[Dilip Kumar]

On Thu, Aug 31, 2023 at 9:36 AM Michael Paquier  wrote:
>
> On Wed, Aug 30, 2023 at 04:51:19PM +0530, Dilip Kumar wrote:
> > Your suggestions LGTM so modified accordingly
>
> I have been putting my HEAD on this patch for a few hours, reviewing
> the surroundings, and somewhat missed that this computation is done
> while we do not hold the WAL insert locks:
> +   checkPoint.redo = ProcLastRecPtr;
>
> Then a few lines down the shared Insert.RedoRecPtr is updated while
> holding an exclusive lock.
> RedoRecPtr = XLogCtl->Insert.RedoRecPtr = checkPoint.redo;
>
> If we have a bunch of records inserted between the moment when the
> REDO record is inserted and the moment when the checkpointer takes the
> exclusive WAL lock, aren't we potentially missing a lot of FPW's that
> should exist since the redo LSN?

Yeah, good catch.  With this, it seems like we can not move this new
WAL Insert out of the Exclusive WAL insertion lock right? because if
we want to set the LSN of this record as the checkpoint. redo then
there should not be any concurrent insertion until we expose the
XLogCtl->Insert.RedoRecPtr. Otherwise, we will miss the FPW for all
the record which has been inserted after the checkpoint. redo before
we acquired the exclusive WAL insertion lock.

So maybe I need to restart from the first version of the patch but
instead of moving the insertion of the new record out of the exclusive
lock need to do some better refactoring so that XLogInsertRecord()
doesn't look ugly.

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

Re: pg_stat_get_backend_subxact() and backend IDs?

[Michael Paquier]

On Wed, Aug 30, 2023 at 02:56:48PM -0700, Nathan Bossart wrote:
> Committed.

Cool, thanks!
--
Michael


signature.asc
Description: PGP signature

Re: [PATCH] Add native windows on arm64 support

[Michael Paquier]

On Wed, Aug 30, 2023 at 04:12:16PM +0100, Anthony Roberts wrote:
> Just a follow-up: having spoken to the relevant people, sadly, right now,
> we do not have the capacity to be pulling machines out of our day-to-day CI
> tasks to dedicate to specific projects.

Okay, thanks for letting us know.

> I can, however, offer a few alternatives:
> 
> 1. If you have capacity on your linux x64 machines, it is possible to
> emulate WoA, and run tests that way:
> https://www.linaro.org/blog/emulate-windows-on-arm/
> This is something that works well for projects that we have contributed it
> to (ie, sse2neon uses it via github actions) - if you run into any issues
> with this, we are able to try and fix them on our side, as likely more than
> postgres would benefit.
> 
> 2. If there is any scope for it on your side at all, a Samsung Galaxy Book
> Go 14" can be had from ebay for ~£140.
> Understandably, this might not be an option for you.
> 
> If it helps, if this is merged, we do have the possibility of running our
> own downstream CI builds nightly for the tip of your main branch here:
> https://gitlab.com/Linaro/windowsonarm/nightly
> This is something we do for a few projects that do not have full upstream
> CI.

Honestly, I am not sure how to proceed here.  Having something in the
buildfarm is necessary IMO, because this is the central place that
community members look at when it comes to monitoring the stability of
a patch committed.

Any opinions from the others?
--
Michael


signature.asc
Description: PGP signature

Re: New WAL record to detect the checkpoint redo location

[Michael Paquier]

On Wed, Aug 30, 2023 at 04:51:19PM +0530, Dilip Kumar wrote:
> Your suggestions LGTM so modified accordingly

I have been putting my HEAD on this patch for a few hours, reviewing
the surroundings, and somewhat missed that this computation is done
while we do not hold the WAL insert locks:
+   checkPoint.redo = ProcLastRecPtr;

Then a few lines down the shared Insert.RedoRecPtr is updated while
holding an exclusive lock.
RedoRecPtr = XLogCtl->Insert.RedoRecPtr = checkPoint.redo;

If we have a bunch of records inserted between the moment when the
REDO record is inserted and the moment when the checkpointer takes the
exclusive WAL lock, aren't we potentially missing a lot of FPW's that
should exist since the redo LSN?
--
Michael


signature.asc
Description: PGP signature

Re: Use virtual tuple slot for Unique node

[Denis Smirnov]

It looks like my patch was not analyzed by the hackers mailing list due to 
incorrect mime type, so I duplicate it here.
commit 2852a3f2fab8e723f208d81c1ad1eb6a6a377b09
Author: Denis Smirnov 
Date:   Thu Aug 31 08:51:14 2023 +0700

Change tuple table slot for Unique node to "virtual"

The Unique node does a very simple thing in the plan - it processes
the incoming sorted tuple stream and adds the unique tuples to the
resulting tuple table slot. Previously the Unique node palloc'ed
the results with the "miminal" tuples. It is redundant and for now
we simply collect references to the child node tuples with the
"virtual" tuples.

diff --git a/src/backend/executor/nodeUnique.c 
b/src/backend/executor/nodeUnique.c
index 45035d74fa..c859add6e0 100644
--- a/src/backend/executor/nodeUnique.c
+++ b/src/backend/executor/nodeUnique.c
@@ -141,7 +141,7 @@ ExecInitUnique(Unique *node, EState *estate, int eflags)
 * Initialize result slot and type. Unique nodes do no projections, so
 * initialize projection info for this node appropriately.
 */
-   ExecInitResultTupleSlotTL(>ps, );
+   ExecInitResultTupleSlotTL(>ps, );
uniquestate->ps.ps_ProjInfo = NULL;
 
/*


> 31 авг. 2023 г., в 10:06, Denis Smirnov  написал(а):
> 
> 

Re: Replace some cstring_to_text to cstring_to_text_with_len

[Michael Paquier]

On Wed, Aug 30, 2023 at 03:00:13PM -0300, Ranier Vilela wrote:
> cstring_to_text has a small overhead, because call strlen for
> pointer to char parameter.
> 
> Is it worth the effort to avoid this, where do we know the size of the
> parameter?

Are there workloads where this matters?
--
Michael


signature.asc
Description: PGP signature

Re: lockup in parallel hash join on dikkop (freebsd 14.0-current)

[Thomas Munro]

On Thu, Aug 31, 2023 at 12:16 AM Tomas Vondra
 wrote:
> I have another case of this on dikkop (on 11 again). Is there anything
> else we'd want to try? Or maybe someone would want access to the machine
> and do some investigation directly?

Sounds interesting -- I'll ping you off-list.

Re: Query execution in Perl TAP tests needs work

[Thomas Munro]

On Thu, Aug 31, 2023 at 10:32 AM Andrew Dunstan  wrote:
> #!/usr/bin/perl
>
> use strict; use warnings;
>
> use FFI::Platypus;
>
> my $ffi = FFI::Platypus->new(api=>1);
> $ffi->lib("inst/lib/libpq.so");
>
>
> $ffi->type('opaque' => 'PGconn');
> $ffi->attach(PQconnectdb => [ 'string' ] => 'PGconn');
> $ffi->attach(PQfinish => [ 'PGconn' ] => 'void');
>
> $ffi->type('opaque' => 'PGresult');
> $ffi->attach(PQexec => [ 'PGconn', 'string' ] => 'PGresult');
> $ffi->attach(PQgetvalue => [ 'PGresult', 'int', 'int' ] => 'string');
>
> my $pgconn = PQconnectdb("dbname=postgres host=/tmp");
> my $res = PQexec($pgconn, "select count(*) from pg_class");
> my $count = PQgetvalue( $res, 0, 0);
>
> print "count: $count\n";
>
> PQfinish($pgconn);

It looks very promising so far.  How hard would it be for us to add
this dependency?  Mostly pinging build farm owners?

I'm still on the fence, but the more I know about IPC::Run, the better
the various let's-connect-directly-from-Perl options sound...

Re: Eliminate redundant tuple visibility check in vacuum

[Melanie Plageman]

> On Tue, Aug 29, 2023 at 5:07 AM David Geier  wrote:
> > Could you measure any performance difference?
> >
> > If so could you provide your test case?
>
> I created a large table and then updated a tuple on every page in the
> relation and vacuumed it. I saw a consistent slight improvement in
> vacuum execution time. I profiled a bit with perf stat as well. The
> difference is relatively small for this kind of example, but I can
> work on a more compelling, realistic example. I think eliminating the
> retry loop is also useful, as I have heard that users have had to
> cancel vacuums which were in this retry loop for too long.

Just to provide a specific test case, if you create a small table like this

create table foo (a int, b int, c int) with(autovacuum_enabled=false);
insert into foo select i, i, i from generate_series(1, 1000);

And then vacuum it. I find that with my patch applied I see a
consistent ~9% speedup (averaged across multiple runs).

master: ~533ms
patch: ~487ms

And in the profile, with my patch applied, you notice less time spent
in HeapTupleSatisfiesVacuumHorizon()

master:
11.83%  postgres  postgres   [.] heap_page_prune
11.59%  postgres  postgres   [.] heap_prepare_freeze_tuple
 8.77%  postgres  postgres   [.] lazy_scan_prune
 8.01%  postgres  postgres   [.] HeapTupleSatisfiesVacuumHorizon
 7.79%  postgres  postgres   [.] heap_tuple_should_freeze

patch:
13.41%  postgres  postgres   [.] heap_prepare_freeze_tuple
 9.88%  postgres  postgres   [.] heap_page_prune
 8.61%  postgres  postgres   [.] lazy_scan_prune
 7.00%  postgres  postgres   [.] heap_tuple_should_freeze
 6.43%  postgres  postgres   [.] HeapTupleSatisfiesVacuumHorizon

- Melanie

Re: Fix shadow warnings in logical replication code

[Peter Smith]

On Thu, Aug 31, 2023 at 9:39 AM Michael Paquier  wrote:
>
> On Wed, Aug 30, 2023 at 09:50:17AM +0800, Richard Guo wrote:
> > Yeah, IIRC the source tree currently is able to be built without any
> > shadow-related warnings with -Wshadow=compatible-local.  But with
> > -Wshadow or -Wshadow=local, you can still see a lot of warnings.
>
> Yep.  I've addressed on HEAD the ones proposed on this thread.
> --

Thanks for pushing!

3 gone, ~150 remaining :)

--
Kind Regards,
Peter Smith.
Fujitsu Australia

~

[postgres@CentOS7-x64 oss_postgres_misc]$ cat make.txt | grep
'warning: declaration'
controldata_utils.c:52:29: warning: declaration of ‘DataDir’ shadows a
global declaration [-Wshadow]
controldata_utils.c:145:32: warning: declaration of ‘DataDir’ shadows
a global declaration [-Wshadow]
brin.c:485:16: warning: declaration of ‘tmp’ shadows a previous local [-Wshadow]
gistbuild.c:1225:23: warning: declaration of ‘splitinfo’ shadows a
previous local [-Wshadow]
xlogreader.c:108:24: warning: declaration of ‘wal_segment_size’
shadows a global declaration [-Wshadow]
xlogrecovery.c:1170:13: warning: declaration of ‘backupEndRequired’
shadows a global declaration [-Wshadow]
xlogrecovery.c:1832:33: warning: declaration of ‘xlogreader’ shadows a
global declaration [-Wshadow]
xlogrecovery.c:3047:28: warning: declaration of ‘xlogprefetcher’
shadows a global declaration [-Wshadow]
xlogrecovery.c:3051:19: warning: declaration of ‘xlogreader’ shadows a
global declaration [-Wshadow]
xlogrecovery.c:3214:31: warning: declaration of ‘xlogreader’ shadows a
global declaration [-Wshadow]
xlogrecovery.c:3965:38: warning: declaration of ‘xlogprefetcher’
shadows a global declaration [-Wshadow]
objectaddress.c:2173:12: warning: declaration of ‘nulls’ shadows a
previous local [-Wshadow]
objectaddress.c:2190:12: warning: declaration of ‘nulls’ shadows a
previous local [-Wshadow]
objectaddress.c:2227:12: warning: declaration of ‘nulls’ shadows a
previous local [-Wshadow]
pg_constraint.c:811:22: warning: declaration of ‘cooked’ shadows a
parameter [-Wshadow]
parse_target.c:1647:13: warning: declaration of ‘levelsup’ shadows a
parameter [-Wshadow]
extension.c:1079:13: warning: declaration of ‘schemaName’ shadows a
parameter [-Wshadow]
schemacmds.c:208:12: warning: declaration of ‘stmt’ shadows a
parameter [-Wshadow]
statscmds.c:291:16: warning: declaration of ‘attnums’ shadows a
previous local [-Wshadow]
tablecmds.c:14034:20: warning: declaration of ‘cmd’ shadows a
parameter [-Wshadow]
tablecmds.c:14107:20: warning: declaration of ‘cmd’ shadows a
parameter [-Wshadow]
trigger.c:1170:13: warning: declaration of ‘qual’ shadows a previous
local [-Wshadow]
execExprInterp.c:407:27: warning: declaration of ‘dispatch_table’
shadows a global declaration [-Wshadow]
nodeAgg.c:3978:20: warning: declaration of ‘phase’ shadows a previous
local [-Wshadow]
nodeValuesscan.c:145:16: warning: declaration of ‘estate’ shadows a
previous local [-Wshadow]
be-secure-common.c:110:44: warning: declaration of ‘ssl_key_file’
shadows a global declaration [-Wshadow]
main.c:217:27: warning: declaration of ‘progname’ shadows a global
declaration [-Wshadow]
main.c:327:18: warning: declaration of ‘progname’ shadows a global
declaration [-Wshadow]
main.c:386:24: warning: declaration of ‘progname’ shadows a global
declaration [-Wshadow]
equivclass.c:727:16: warning: declaration of ‘rel’ shadows a parameter
[-Wshadow]
createplan.c:1245:12: warning: declaration of ‘plan’ shadows a
previous local [-Wshadow]
createplan.c:2560:12: warning: declaration of ‘plan’ shadows a
previous local [-Wshadow]
partdesc.c:218:16: warning: declaration of ‘key’ shadows a previous
local [-Wshadow]
logicalfuncs.c:184:13: warning: declaration of ‘name’ shadows a
previous local [-Wshadow]
reorderbuffer.c:4843:10: warning: declaration of ‘isnull’ shadows a
previous local [-Wshadow]
walsender.c:3543:14: warning: declaration of ‘sentPtr’ shadows a
global declaration [-Wshadow]
dependencies.c:377:23: warning: declaration of ‘DependencyGenerator’
shadows a global declaration [-Wshadow]
dependencies.c:1194:14: warning: declaration of ‘expr’ shadows a
parameter [-Wshadow]
dependencies.c:1228:22: warning: declaration of ‘expr’ shadows a
parameter [-Wshadow]
extended_stats.c:1047:10: warning: declaration of ‘isnull’ shadows a
previous local [-Wshadow]
date.c:506:9: warning: declaration of ‘days’ shadows a global
declaration [-Wshadow]
date.c:530:9: warning: declaration of ‘days’ shadows a global
declaration [-Wshadow]
date.c:2015:9: warning: declaration of ‘days’ shadows a global
declaration [-Wshadow]
datetime.c:637:8: warning: declaration of ‘days’ shadows a global
declaration [-Wshadow]
formatting.c:3181:25: warning: declaration of ‘months’ shadows a
global declaration [-Wshadow]
jsonpath_exec.c:410:17: warning: declaration of ‘found’ shadows a
previous local [-Wshadow]
jsonpath_exec.c:1328:24: warning: declaration of ‘res’ shadows a
previous local [-Wshadow]
jsonpath_exec.c:1339:24: warning: 

Re: Fix shadow warnings in logical replication code

[Michael Paquier]

On Wed, Aug 30, 2023 at 09:50:17AM +0800, Richard Guo wrote:
> Yeah, IIRC the source tree currently is able to be built without any
> shadow-related warnings with -Wshadow=compatible-local.  But with
> -Wshadow or -Wshadow=local, you can still see a lot of warnings.

Yep.  I've addressed on HEAD the ones proposed on this thread.
--
Michael


signature.asc
Description: PGP signature

Re: Sync scan & regression tests

[Melanie Plageman]

On Wed, Aug 30, 2023 at 5:15 PM David Rowley  wrote:
>
> On Tue, 29 Aug 2023 at 22:35, Heikki Linnakangas  wrote:
> > Looking the new heapgettup_advance_block() function and the code that it
> > replaced, it's now skipping this ss_report_location() on the last call,
> > when it has reached the end of the scan:
> >
> > >
> > >   /*
> > >* Report our new scan position for synchronization purposes. We
> > >* don't do that when moving backwards, however. That would just
> > >* mess up any other forward-moving scanners.
> > >*
> > >* Note: we do this before checking for end of scan so that the
> > >* final state of the position hint is back at the start of the
> > >* rel.  That's not strictly necessary, but otherwise when you run
> > >* the same query multiple times the starting position would shift
> > >* a little bit backwards on every invocation, which is confusing.
> > >* We don't guarantee any specific ordering in general, though.
> > >*/
> > >   if (scan->rs_base.rs_flags & SO_ALLOW_SYNC)
> > >   ss_report_location(scan->rs_base.rs_rd, block);
> >
> > The comment explicitly says that we do that before checking for
> > end-of-scan, but that commit moved it to after. I propose the attached
> > to move it back to before the end-of-scan checks.
> >
> > Melanie, David, any thoughts?
>
> I just looked at v15's code and I agree that the ss_report_location()
> would be called even when the scan is finished.  It wasn't intentional
> that that was changed in v16, so I'm happy for your patch to be
> applied and backpatched to 16.  Thanks for digging into that.

Yes, thanks for catching my mistake.
LGTM.

Re: [PoC] Federated Authn/z with OAUTHBEARER

[Jacob Champion]

v11 is a quick rebase over the recent Cirrus changes, and I've dropped
0006 now that psycopg2 can build against BSD/Meson setups (thanks Daniele!).

--Jacob1:  0278c7ba90 = 1:  36409a76ce common/jsonapi: support FRONTEND clients
2:  bb3ce4b6a9 = 2:  1356b729db libpq: add OAUTHBEARER SASL mechanism
3:  20b758 ! 3:  863a49f863 backend: add OAUTHBEARER SASL mechanism
@@ src/backend/utils/misc/guc_tables.c
  #include "nodes/queryjumble.h"
  #include "optimizer/cost.h"
 @@ src/backend/utils/misc/guc_tables.c: struct config_string 
ConfigureNamesString[] =
-   check_io_direct, assign_io_direct, NULL
+   check_debug_io_direct, assign_debug_io_direct, NULL
},
  
 +  {
4:  f3cec068f9 = 4:  348554e5f4 Add pytest suite for OAuth
5:  da1933ac1d ! 5:  16d3984a45 squash! Add pytest suite for OAuth
@@ Commit message
 - The with_oauth test skip logic should probably be integrated into the
   Makefile side as well...
 
- ## .cirrus.yml ##
-@@ .cirrus.yml: env:
+ ## .cirrus.tasks.yml ##
+@@ .cirrus.tasks.yml: env:
MTEST_ARGS: --print-errorlogs --no-rebuild -C build
PGCTLTIMEOUT: 120 # avoids spurious failures during parallel tests
TEMP_CONFIG: ${CIRRUS_WORKING_DIR}/src/tools/ci/pg_ci_base.conf
@@ .cirrus.yml: env:
  
  
  # What files to preserve in case tests fail
-@@ .cirrus.yml: task:
+@@ .cirrus.tasks.yml: task:
  chown root:postgres /tmp/cores
  sysctl kern.corefile='/tmp/cores/%N.%P.core'
setup_additional_packages_script: |
@@ .cirrus.yml: task:
  
# NB: Intentionally build without -Dllvm. The freebsd image size is 
already
# large enough to make VM startup slow, and even without llvm freebsd
-@@ .cirrus.yml: task:
+@@ .cirrus.tasks.yml: task:
  --buildtype=debug \
  -Dcassert=true -Duuid=bsd -Dtcl_version=tcl86 -Ddtrace=auto \
  -DPG_TEST_EXTRA="$PG_TEST_EXTRA" \
@@ .cirrus.yml: task:
  -Dextra_lib_dirs=/usr/local/lib 
-Dextra_include_dirs=/usr/local/include/ \
  build
  EOF
-@@ .cirrus.yml: LINUX_CONFIGURE_FEATURES: _CONFIGURE_FEATURES >-
+@@ .cirrus.tasks.yml: LINUX_CONFIGURE_FEATURES: _CONFIGURE_FEATURES 
>-
--with-libxslt
--with-llvm
--with-lz4
@@ .cirrus.yml: LINUX_CONFIGURE_FEATURES: _CONFIGURE_FEATURES >-
--with-pam
--with-perl
--with-python
-@@ .cirrus.yml: LINUX_CONFIGURE_FEATURES: _CONFIGURE_FEATURES >-
+@@ .cirrus.tasks.yml: LINUX_CONFIGURE_FEATURES: _CONFIGURE_FEATURES 
>-
  
  LINUX_MESON_FEATURES: _MESON_FEATURES >-
-Dllvm=enabled
@@ .cirrus.yml: LINUX_CONFIGURE_FEATURES: _CONFIGURE_FEATURES >-
-Duuid=e2fs
  
  
-@@ .cirrus.yml: task:
+@@ .cirrus.tasks.yml: task:
  EOF
  
setup_additional_packages_script: |
@@ .cirrus.yml: task:
  
matrix:
  - name: Linux - Debian Bullseye - Autoconf
-@@ .cirrus.yml: task:
+@@ .cirrus.tasks.yml: task:
  folder: $CCACHE_DIR
  
setup_additional_packages_script: |
@@ src/test/python/requirements.txt
  construct~=2.10.61
  isort~=5.6
  # TODO: update to psycopg[c] 3.1
+-psycopg2~=2.9.6
++psycopg2~=2.9.7
+ pytest~=7.3
+ pytest-asyncio~=0.21.0
 
  ## src/test/python/server/conftest.py ##
 @@
6:  8f36b5c124 < -:  -- XXX work around psycopg2 build failures


v11-0001-common-jsonapi-support-FRONTEND-clients.patch.gz
Description: application/gzip


v11-0002-libpq-add-OAUTHBEARER-SASL-mechanism.patch.gz
Description: application/gzip


v11-0003-backend-add-OAUTHBEARER-SASL-mechanism.patch.gz
Description: application/gzip


v11-0004-Add-pytest-suite-for-OAuth.patch.gz
Description: application/gzip


v11-0005-squash-Add-pytest-suite-for-OAuth.patch.gz
Description: application/gzip

Re: Query execution in Perl TAP tests needs work

[Andrew Dunstan]

On 2023-08-28 Mo 09:23, Andrew Dunstan wrote:



On 2023-08-28 Mo 01:29, Thomas Munro wrote:

Hi,

Every time we run a SQL query, we fork a new psql process and a new
cold backend process.  It's not free on Unix, and quite a lot worse on
Windows, at around 70ms per query.  Take amcheck/001_verify_heapam for
example.  It runs 272 subtests firing off a stream of queries, and
completes in ~51s on Windows (!), and ~6-9s on the various Unixen, on
CI.

Here are some timestamps I captured from CI by instrumenting various
Perl and C bits:

0.000s: IPC::Run starts
0.023s:   postmaster socket sees connection
0.025s:   postmaster has created child process
0.033s: backend starts running main()
0.039s: backend has reattached to shared memory
0.043s: backend connection authorized message
0.046s: backend has executed and logged query
0.070s: IPC::Run returns

I expected process creation to be slow on that OS, but it seems like
something happening at the end is even slower.  CI shows Windows
consuming 4 CPUs at 100% for a full 10 minutes to run a test suite
that finishes in 2-3 minutes everywhere else with the same number of
CPUs.  Could there be an event handling snafu in IPC::Run or elsewhere
nearby?  It seems like there must be either a busy loop or a busted
sleep/wakeup... somewhere?  But even if there's a weird bug here
waiting to be discovered and fixed, I guess it'll always be too slow
at ~10ms per process spawned, with two processes to spawn, and it's
bad enough on Unix.

As an experiment, I hacked up a not-good-enough-to-share experiment
where $node->safe_psql() would automatically cache a BackgroundPsql
object and reuse it, and the times for that test dropped ~51 -> ~9s on
Windows, and ~7 -> ~2s on the Unixen.  But even that seems non-ideal
(well it's certainly non-ideal the way I hacked it up anyway...).  I
suppose there are quite a few ways we could do better:

1.  Don't fork anything at all: open (and cache) a connection directly
from Perl.
1a.  Write xsub or ffi bindings for libpq.  Or vendor (parts) of the
popular Perl xsub library?
1b.  Write our own mini pure-perl pq client module.  Or vendor (parts)
of some existing one.
2.  Use long-lived psql sessions.
2a.  Something building on BackgroundPsql.
2b.  Maybe give psql or a new libpq-wrapper a new low level stdio/pipe
protocol that is more fun to talk to from Perl/machines?

In some other languages one can do FFI pretty easily so we could use
the in-tree libpq without extra dependencies:


import ctypes
libpq = ctypes.cdll.LoadLibrary("/path/to/libpq.so")
libpq.PQlibVersion()

17

... but it seems you can't do either static C bindings or runtime FFI
from Perl without adding a new library/package dependency.  I'm not
much of a Perl hacker so I don't have any particular feeling.  What
would be best?

This message brought to you by the Lorax.


Thanks for raising this. Windows test times have bothered me for ages.

The standard perl DBI library has a connect_cached method. Of course 
we don't want to be dependent on it, especially if we might have 
changed libpq in what we're testing, and it would place a substantial 
new burden on testers like buildfarm owners.


I like the idea of using a pure perl pq implementation, not least 
because it could expand our ability to test things at the protocol 
level. Not sure how much work it would be. I'm willing to help if we 
want to go that way.


Yes you need an external library to use FFI in perl, but there's one 
that's pretty tiny. See . There 
is also FFI::Platypus, but it involves building a library. OTOH, 
that's the one that's available standard on my Fedora and Ubuntu 
systems. I haven't tried using either Maybe we could use some logic 
that would use the FFI interface if it's available, and fall back on 
current usage.






I had a brief play with this. Here's how easy it was to wrap libpq in perl:


#!/usr/bin/perl

use strict; use warnings;

use FFI::Platypus;

my $ffi = FFI::Platypus->new(api=>1);
$ffi->lib("inst/lib/libpq.so");


$ffi->type('opaque' => 'PGconn');
$ffi->attach(PQconnectdb => [ 'string' ] => 'PGconn');
$ffi->attach(PQfinish => [ 'PGconn' ] => 'void');

$ffi->type('opaque' => 'PGresult');
$ffi->attach(PQexec => [ 'PGconn', 'string' ] => 'PGresult');
$ffi->attach(PQgetvalue => [ 'PGresult', 'int', 'int' ] => 'string');

my $pgconn = PQconnectdb("dbname=postgres host=/tmp");
my $res = PQexec($pgconn, "select count(*) from pg_class");
my $count = PQgetvalue( $res, 0, 0);

print "count: $count\n";

PQfinish($pgconn);


cheers


andrew

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

Re: Query execution in Perl TAP tests needs work

[Andres Freund]

Hi,

On 2023-08-28 17:29:56 +1200, Thomas Munro wrote:
> Every time we run a SQL query, we fork a new psql process and a new
> cold backend process.  It's not free on Unix, and quite a lot worse on
> Windows, at around 70ms per query.  Take amcheck/001_verify_heapam for
> example.  It runs 272 subtests firing off a stream of queries, and
> completes in ~51s on Windows (!), and ~6-9s on the various Unixen, on
> CI.

Whoa.

> Here are some timestamps I captured from CI by instrumenting various
> Perl and C bits:
> 
> 0.000s: IPC::Run starts
> 0.023s:   postmaster socket sees connection
> 0.025s:   postmaster has created child process
> 0.033s: backend starts running main()
> 0.039s: backend has reattached to shared memory
> 0.043s: backend connection authorized message
> 0.046s: backend has executed and logged query
> 0.070s: IPC::Run returns
> 
> I expected process creation to be slow on that OS, but it seems like
> something happening at the end is even slower.  CI shows Windows
> consuming 4 CPUs at 100% for a full 10 minutes to run a test suite
> that finishes in 2-3 minutes everywhere else with the same number of
> CPUs.

It finishes in that time on linux, even with sanitizers enabled...


> As an experiment, I hacked up a not-good-enough-to-share experiment
> where $node->safe_psql() would automatically cache a BackgroundPsql
> object and reuse it, and the times for that test dropped ~51 -> ~9s on
> Windows, and ~7 -> ~2s on the Unixen.  But even that seems non-ideal
> (well it's certainly non-ideal the way I hacked it up anyway...).  I
> suppose there are quite a few ways we could do better:

That's a really impressive win.

Even if we "just" converted some of the safe_psql() cases and converted
poll_query_until() to this, we'd win a lot.


> 1.  Don't fork anything at all: open (and cache) a connection directly
> from Perl.

One advantage of that is that the socket is entirely controlled by perl, so
waiting for IO should be easy...


> 2b.  Maybe give psql or a new libpq-wrapper a new low level stdio/pipe
> protocol that is more fun to talk to from Perl/machines?

That does also seem promising - a good chunk of the complexity around some of
the IPC::Run uses is that we end up parsing psql input/output...

Greetings,

Andres Freund

Re: Use virtual tuple slot for Unique node

[David Rowley]

On Thu, 31 Aug 2023 at 05:37, Денис Смирнов  wrote:
> I have inspected the performance of the GROUP BY and DISTINCT queries for the 
> sorted data streams and found out, that Group node (produced by GROUP BY) 
> works faster then the Unique node (produced by DISTINCT).  The flame graph 
> should out the reason - Unique palloc`s tuples for the result slot while the 
> Group node doesn’t.
>
> I wonder, why do we use minimal tuples for the Unique node instead of the 
> virtual ones? It looks like there is no actual reason for that as Unique 
> doesn’t make any materialization.

It would be good to see example queries and a demonstration of the
performance increase. I'm not disputing your claims, but showing some
performance numbers might catch the eye of a reviewer more quickly.

You should also add this to the September commitfest at
https://commitfest.postgresql.org/44/

David

Re: pg_stat_get_backend_subxact() and backend IDs?

[Nathan Bossart]

On Wed, Aug 30, 2023 at 10:56:22AM -0400, Robert Haas wrote:
> On Wed, Aug 30, 2023 at 10:27 AM Nathan Bossart
>  wrote:
>> I'm about to spend way too much time writing the commit message for 0002,
>> but I plan to commit both patches sometime today.
> 
> Thanks! I'm glad your committing the patches, and I approve of you
> spending way too much time on the commit message. :-)

Committed.

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

Re: Sync scan & regression tests

[David Rowley]

On Tue, 29 Aug 2023 at 22:35, Heikki Linnakangas  wrote:
> Looking the new heapgettup_advance_block() function and the code that it
> replaced, it's now skipping this ss_report_location() on the last call,
> when it has reached the end of the scan:
>
> >
> >   /*
> >* Report our new scan position for synchronization purposes. We
> >* don't do that when moving backwards, however. That would just
> >* mess up any other forward-moving scanners.
> >*
> >* Note: we do this before checking for end of scan so that the
> >* final state of the position hint is back at the start of the
> >* rel.  That's not strictly necessary, but otherwise when you run
> >* the same query multiple times the starting position would shift
> >* a little bit backwards on every invocation, which is confusing.
> >* We don't guarantee any specific ordering in general, though.
> >*/
> >   if (scan->rs_base.rs_flags & SO_ALLOW_SYNC)
> >   ss_report_location(scan->rs_base.rs_rd, block);
>
> The comment explicitly says that we do that before checking for
> end-of-scan, but that commit moved it to after. I propose the attached
> to move it back to before the end-of-scan checks.
>
> Melanie, David, any thoughts?

I just looked at v15's code and I agree that the ss_report_location()
would be called even when the scan is finished.  It wasn't intentional
that that was changed in v16, so I'm happy for your patch to be
applied and backpatched to 16.  Thanks for digging into that.

David

Re: [PATCH] Support SK_SEARCHNULL / SK_SEARCHNOTNULL for heap-only scans

[Jacob Champion]

On 7/19/23 16:44, Jacob Champion wrote:
> This patch pushes down any
> forced-null and not-null Vars as ScanKeys. It doesn't remove the
> redundant quals after turning them into ScanKeys, so it's needlessly
> inefficient, but there's still a decent speedup for some of the basic
> benchmarks in 0003.
> 
> Plans look something like this:
> 
> # EXPLAIN SELECT * FROM t WHERE i IS NULL;
>  QUERY PLAN
> 
>  Seq Scan on t  (cost=0.00..1393.00 rows=49530 width=4)
>Scan Cond: (i IS NULL)
>Filter: (i IS NULL)
> (3 rows)

Redundant clauses are now filtered out in v3. So the new plans look more
like what you'd expect:

  =# EXPLAIN SELECT * FROM table1 WHERE a IS NOT NULL AND b = 2;
 QUERY PLAN
  -
   Seq Scan on table1  (cost=0.00..3344.00 rows=1 width=4)
 Scan Cond: (a IS NOT NULL)
 Filter: (b = 2)
  (3 rows)

> The non-nullable case worries me a bit because so many things imply IS
> NOT NULL. I think I need to do some sort of cost analysis using the
> null_frac statistics -- it probably only makes sense to push an
> implicit SK_SEARCHNOTNULL down to the AM layer if some fraction of
> rows would actually be filtered out -- but I'm not really sure how to
> choose a threshold.

v3 also uses the nullfrac and the expected cost of the filter clauses to
decide whether to promote a derived IS NOT NULL condition to a ScanKey.
(Explicit IS [NOT] NULL clauses are always promoted.) I'm still not sure
how to fine-tune the expected cost of the ScanKey, but the number I've
chosen for now (`0.1 * cpu_operator_cost`) doesn't seem to regress my
benchmarks, for whatever that's worth.

I recorded several of the changes to the regression EXPLAIN output, but
I've left a few broken because I'm not sure if they're useful or if I
should just disable the optimization. For example:

   explain (analyze, costs off, summary off, timing off)
 select * from list_part where a = list_part_fn(1) + a;
   QUERY PLAN
   --
Append (actual rows=0 loops=1)
  ->  Seq Scan on list_part1 list_part_1 (actual rows=0 loops=1)
  + Scan Cond: (a IS NOT NULL)
Filter: (a = (list_part_fn(1) + a))
Rows Removed by Filter: 1
  ->  Seq Scan on list_part2 list_part_2 (actual rows=0 loops=1)
  + Scan Cond: (a IS NOT NULL)
Filter: (a = (list_part_fn(1) + a))
Rows Removed by Filter: 1
  ->  Seq Scan on list_part3 list_part_3 (actual rows=0 loops=1)
  + Scan Cond: (a IS NOT NULL)
Filter: (a = (list_part_fn(1) + a))
Rows Removed by Filter: 1
  ->  Seq Scan on list_part4 list_part_4 (actual rows=0 loops=1)
  + Scan Cond: (a IS NOT NULL)
Filter: (a = (list_part_fn(1) + a))
Rows Removed by Filter: 1

These new conditions are due to a lack of statistics for the tiny
partitions; the filters are considered expensive enough that the savings
against a DEFAULT_UNK_SEL proportion of NULLs would hypothetically be
worth it. Best I can tell, this is a non-issue, since autovacuum will
fix the problem by the time the table grows to the point where the
pointless ScanKey would cause a slowdown. But it sure _looks_ like a
mistake, especially since these particular partitions can't even contain
NULL. Do I need to do something about this short-lived case?

There's still other work to do -- for instance, I think my modifications
to the filter clauses have probably broken some isolation levels until I
fix up SeqRecheck(), and better benchmarks would be good -- but I think
this is ready for early CF feedback.

Thanks,
--JacobFrom 12794631ee790d637a664f81679c9cdd7d00e2c4 Mon Sep 17 00:00:00 2001
From: Aleksander Alekseev 
Date: Mon, 13 Feb 2023 16:15:45 +0300
Subject: [PATCH v3 1/3] Support SK_SEARCHNULL / SK_SEARCHNOTNULL for heap-only
 scans

Previously it was not supported which could be of inconvenience for the
extension authors.

Author: Aleksander Alekseev
Reviewed-by: Andres Freund
Discussion: https://postgr.es/m/caj7c6tpkeh7uwen9orqp_dmr8uxifhxt8pecq01zw1hkptb...@mail.gmail.com
---
 src/include/access/skey.h|  7 +--
 src/include/access/valid.h   | 41 ++-
 src/test/regress/expected/heapscan.out   | 33 
 src/test/regress/expected/test_setup.out | 13 +
 src/test/regress/parallel_schedule   |  2 +-
 src/test/regress/regress.c   | 67 
 src/test/regress/sql/heapscan.sql| 12 +
 src/test/regress/sql/test_setup.sql  | 16 ++
 8 files changed, 175 insertions(+), 16 deletions(-)
 create mode 100644 src/test/regress/expected/heapscan.out
 create mode 100644 src/test/regress/sql/heapscan.sql

diff --git a/src/include/access/skey.h b/src/include/access/skey.h
index 

Re: Allow specifying a dbname in pg_basebackup connection string

[Jim Jones]

On 30.08.23 14:11, Jelte Fennema wrote:

Oops it indeed seemed like I made an unintended change when handling
database names that did not exist in pgbouncer.conf when you used
auth_type=hba. I pushed a fix for that now to the replication-support
branch. Feel free to try again. But as you said it's unrelated to the
postgres patch.


Nice! Now it seems to work as expected :)

$ /usr/local/postgres-dev/bin/pg_basebackup --dbname "host=127.0.0.1 
port=6432 user=jim  dbname=foo" -X fetch -v -l testlabel -D /tmp/dump

pg_basebackup: initiating base backup, waiting for checkpoint to complete
pg_basebackup: checkpoint completed
pg_basebackup: write-ahead log start point: 0/1228 on timeline 1
pg_basebackup: write-ahead log end point: 0/12000100
pg_basebackup: syncing data to disk ...
pg_basebackup: renaming backup_manifest.tmp to backup_manifest
pg_basebackup: base backup completed

pgbouncer log:

2023-08-30 21:04:30.225 CEST [785989] LOG C-0x55fbee0f50b0: 
foo/jim@127.0.0.1:49764 login attempt: db=foo user=jim tls=no 
replication=yes
2023-08-30 21:04:30.225 CEST [785989] LOG S-0x55fbee0fc560: 
foo/jim@127.0.0.1:5432 new connection to server (from 127.0.0.1:34400)
2023-08-30 21:04:30.226 CEST [785989] LOG S-0x55fbee0fc560: 
foo/jim@127.0.0.1:5432 closing because: replication client was closed 
(age=0s)
2023-08-30 21:04:30.226 CEST [785989] LOG S-0x55fbee0fc560: 
foo/jim@127.0.0.1:5432 new connection to server (from 127.0.0.1:34408)
2023-08-30 21:04:30.227 CEST [785989] LOG S-0x55fbee0fc560: 
foo/jim@127.0.0.1:5432 closing because: replication client was closed 
(age=0s)
2023-08-30 21:04:30.227 CEST [785989] LOG S-0x55fbee0fc560: 
foo/jim@127.0.0.1:5432 new connection to server (from 127.0.0.1:34410)
2023-08-30 21:04:30.228 CEST [785989] LOG S-0x55fbee0fc560: 
foo/jim@127.0.0.1:5432 closing because: replication client was closed 
(age=0s)
2023-08-30 21:04:30.228 CEST [785989] LOG S-0x55fbee0fc560: 
foo/jim@127.0.0.1:5432 new connection to server (from 127.0.0.1:34418)
2023-08-30 21:04:30.309 CEST [785989] LOG S-0x55fbee0fc560: 
foo/jim@127.0.0.1:5432 closing because: replication client was closed 
(age=0s)
2023-08-30 21:04:30.309 CEST [785989] LOG C-0x55fbee0f50b0: 
foo/jim@127.0.0.1:49764 closing because: client close request (age=0s)


Jim

Re: UUID v7

[Andrey M. Borodin]

> On 21 Aug 2023, at 13:42, Andrey M. Borodin  wrote:
> 
> 

FPA attached next version.
Changes:
- implemented protection from time leap backwards when series is generated on 
the same backend
- counter overflow is now translated into ms step forward


Best regards, Andrey Borodin.


v6-0001-Implement-UUID-v7-as-per-IETF-draft.patch
Description: Binary data


v6-0003-Use-cached-random-numbers-in-gen_random_uuid-too.patch
Description: Binary data


v6-0002-Buffer-random-numbers.patch
Description: Binary data

Replace some cstring_to_text to cstring_to_text_with_len

[Ranier Vilela]

Hi,

cstring_to_text has a small overhead, because call strlen for
pointer to char parameter.

Is it worth the effort to avoid this, where do we know the size of the
parameter?

best regards,
Ranier Vilela


0003-Avoid-unecessary-calls-to-strlen-function.patch
Description: Binary data

Re: More new SQL/JSON item methods

[Chapman Flack]

On 2023-08-30 12:28, Alvaro Herrera wrote:

Yeah, I think the experience of the SQL committee with XML was pretty
bad, as you carefully documented.  I hope they don't make such a mess
with JSON.


I guess the SQL committee was taken by surprise after basing something
on Infoset and XPath 1.0 for 2003, and then w3 deciding those things
needed to be scrapped and redone with the lessons learned. So the
SQL committee had to come out with a rather different SQL/XML for 2006,
but I'd say the 2003-2006 difference is the only real 'mess', and other
than going back in time to unpublish 2003, I'm not sure how they'd have
done better.


b) Otherwise, the result of JAE is the SQL/JSON sequence V_1,
   ..., V_n.


This has my Spidey sense tingling, as it seems very parallel to SQL/XML
where the result of XMLQUERY is to have type XML(SEQUENCE), which is a
type we do not have, and I'm not sure we have a type for "JSON sequence"
either, unless SQL/JSON makes it equivalent to a JSON array (which
I guess is conceivable, more easily than with XML). What does SQL/JSON
say about this SQL/JSON sequence type and how it should behave?

Regards,
-Chap

Re: EBCDIC sorting as a use case for ICU rules

[Daniel Verite]

Peter Eisentraut wrote:

> Committed with some editing.  I moved the existing rules example from 
> the CREATE COLLATION page into the new section you created, so we have a 
> simple example followed by the complex example.

OK, thanks for pushing this!


Best regards,
-- 
Daniel Vérité
https://postgresql.verite.pro/
Twitter: @DanielVerite

Use virtual tuple slot for Unique node

[Денис Смирнов]

Hi,

I have inspected the performance of the GROUP BY and DISTINCT queries for the 
sorted data streams and found out, that Group node (produced by GROUP BY) works 
faster then the Unique node (produced by DISTINCT).  The flame graph should out 
the reason - Unique palloc`s tuples for the result slot while the Group node 
doesn’t.

I wonder, why do we use minimal tuples for the Unique node instead of the 
virtual ones? It looks like there is no actual reason for that as Unique 
doesn’t make any materialization.
diff --git a/src/backend/executor/nodeUnique.c 
b/src/backend/executor/nodeUnique.c
index 45035d74fa..c859add6e0 100644
--- a/src/backend/executor/nodeUnique.c
+++ b/src/backend/executor/nodeUnique.c
@@ -141,7 +141,7 @@ ExecInitUnique(Unique *node, EState *estate, int eflags)
 * Initialize result slot and type. Unique nodes do no projections, so
 * initialize projection info for this node appropriately.
 */
-   ExecInitResultTupleSlotTL(>ps, );
+   ExecInitResultTupleSlotTL(>ps, );
uniquestate->ps.ps_ProjInfo = NULL;
 
/*

Re: More new SQL/JSON item methods

[Alvaro Herrera]

On 2023-Aug-30, Chapman Flack wrote:

> Hi,
> 
> On 2023-08-29 03:05, Jeevan Chalke wrote:
> > This commit implements jsonpath .bigint(), .integer(), and .number()
> > ---
> > This commit implements jsonpath .date(), .time(), .time_tz(),
> > .timestamp(), .timestamp_tz() methods.
> > ---
> > This commit implements jsonpath .boolean() and .string() methods.
> 
> Writing as an interested outsider to the jsonpath spec, my first
> question would be, is there a published jsonpath spec independent
> of PostgreSQL, and are these methods in it, and are the semantics
> identical?

Looking at the SQL standard itself, in the 2023 edition section 9.46
"SQL/JSON path language: syntax and semantics", it shows this:

 ::=
type  
| size  
| double  
| ceiling  
| floor  
| abs  
| datetime  [  ] 
| keyvalue  
| bigint  
| boolean  
| date  
| decimal  [  [   ] ] 
| integer  
| number  
| string  
| time  [  ] 
| time_tz  [  ] 
| timestamp  [  ] 
| timestamp_tz  [  ] 

and then details, for each of those, rules like

III) If JM specifies , then:
 1) For all j, 1 (one) ≤ j ≤ n,
Case:
a) If I_j is not a number or character string, then let ST be data
   exception — non-numeric SQL/JSON item (22036).
b) Otherwise, let X be an SQL variable whose value is I_j.
   Let V_j be the result of
   CAST (X AS DOUBLE PRECISION)
   If this conversion results in an exception condition, then
   let ST be that exception condition.
 2) Case:
a) If ST is not successful completion, then the result of JAE
   is ST.
b) Otherwise, the result of JAE is the SQL/JSON sequence V_1,
   ..., V_n.

so at least superficially our implementation is constrained by what the
SQL standard says to do, and we should verify that this implementation
matches those rules.  We don't necessarily need to watch what do other
specs such as jsonpath itself.

> The question comes out of my experience on a PostgreSQL integration
> of XQuery/XPath, which was nontrivial because the w3 specs for those
> languages give rigorous definitions of their data types, independently
> of SQL, and a good bit of the work was squinting at those types and at
> the corresponding PostgreSQL types to see in what ways they were
> different, and what the constraints on converting them were.

Yeah, I think the experience of the SQL committee with XML was pretty
bad, as you carefully documented.  I hope they don't make such a mess
with JSON.

-- 
Álvaro Herrera   48°01'N 7°57'E  —  https://www.EnterpriseDB.com/

Re: Disabling Heap-Only Tuples

[Matthias van de Meent]

On Wed, 30 Aug 2023 at 15:31, Robert Haas  wrote:
>
> On Wed, Aug 30, 2023 at 9:01 AM Matthias van de Meent
>  wrote:
> > I've reworked the patch a bit to remove the "excessive bloat with low
> > fillfactors when local space is available" issue that this parameter
> > could cause - local updates are now done if the selected page we would
> > be inserting into is after the old tuple's page and the old tuple's
> > page still (or: now) has space available.
> >
> > Does that alleviate your concerns?
>
> That seems like a good chance, but my core concern is around people
> having to micromanage local_update_limit, and probably either not
> knowing how to do it properly, or not being able or willing to keep
> updating it as things change.

Assuming you do want to provide a way to users to solve the issue of
"there is a lot of free space in the table, but I don't want to take
an access exclusive lock or wait for new inserts to fix the issue",
how would you suggest we do that then?

Alternative approaches that I can think of are:

- A %-based parameter.
  This does scale with the table, but doesn't stop being a performance
hog once you've reached the optimal table size, and thus also needs to
be disabled.

- Measure the parameter from the end of the table, instead of from the
front; i.e. "try to empty the last X=50 MBs of the table".
  Scales with the table, but same issue as above - once the table has
an optimal size, it doesn't stop.

- Install one more dynamic system to move the tuples to a better page,
one the users don't directly control (yet to be designed).
  I don't know if or when this will be implemented and what benefits
it will have, but we don't have access to a lot of state in
table_tuple_update or heap_update, so any data needs special lookup.

- Let users keep using VACUUM FULL and CLUSTER instead.
  I don't think this is a reasonable solution.

> In a way, this parameter is a lot like work_mem, which is notoriously
> very difficult to tune. If you set it too high, you run out of memory.
> If you set it too low, you get bad plans. You can switch from having
> one of those problems to having the other very quickly as load changs,
> and sometimes you can have both at the same time. If an omniscient
> oracle could set work_mem properly for every query based not only on
> what the query does but the state of the system at that moment, it
> would still be a very crude parameter, and since omniscient oracles
> are rare in practice, problems are reasonably common. I think that if
> we add this parameter, it's going to end up in the same category. A
> lot of people will ignore it, and they'll be OK, but 30% of the people
> who do try to use it will shoot themselves in the foot, or something
> like that.

The "shoot yourself in the foot" in this case is limited to "your
UPDATE statement's performance is potentially Y times worse due to
forced FSM lookups for every update at the end of the table". I'll
admit that this is not great, but I'd say it is also not the end of
the world, and still much better than the performance differences that
you can see when the plan changes due to an updated work_mem.

I'd love to have more contextual information available on the table's
free space distribution so that this decision could be made by the
system, but that info just isn't available right now. We don't really
have infrastructure in place that would handle such information
either, and table_tuple_update does not get to use reuse state across
tuples, so any use of information will add cost for every update. With
this patch, the FSM cost is gated behind the storage parameter, and
thus only limited, but I don't think we can store much more than
storage parameters in the Relation data.

VACUUM / ANALYZE could probably create and store sketches about the
free space distribution in the relation, but that would widen the
scope significantly, and I have only limited bandwidth available for
this.
So, while I do plan to implement any small changes or fixes required
to get this in, a major change in direction for this patch won't put
it anywhere high on my active items list.


Kind regards,

Matthias van de Meent

Re: [17] CREATE SUBSCRIPTION ... SERVER

[Jeff Davis]

On Wed, 2023-08-30 at 09:49 -0400, Tom Lane wrote:
> This seems like it requires a whole lot of new mechanism (parser
> and catalog infrastructure) that could be done far more easily
> in other ways.  In particular, how about inventing a built-in
> dummy FDW to serve the purpose?

That was my initial approach, but it was getting a bit messy.

FDWs don't have a schema, so we can't put it in pg_catalog, and names
beginning with "pg_" aren't restricted now. Should I retroactively
restrict FDW names that begin with "pg_"? Or just use special cases in
pg_dump and elsewhere? Also I didn't see a great place to document it.

Admittedly, I didn't complete the dummy-FDW approach, so perhaps it
works out better overall. I can give it a try.

Regards,
Jeff Davis

Re: New compiler warning

[Bruce Momjian]

Peter Eisentraut has applied a patch to fix this.

---

On Wed, Aug 30, 2023 at 10:07:24AM -0400, David Steele wrote:
> On 8/30/23 08:10, Aleksander Alekseev wrote:
> > 
> > > I am seeing a new gcc 12.2.0 compiler warning from
> > > src/backend/commands/sequence.c:
> > 
> > Yep, the compiler is just not smart enough to derive that this
> > actually is not going to happen.
> > 
> > Here is a proposed fix.
> 
> Here's an alternate way to deal with this which is a bit more efficient
> (code not tested):
> 
> - case SEQ_COL_CALLED:
> - coldef = makeColumnDef("is_called", BOOLOID, -1, 
> InvalidOid);
> - value[i - 1] = BoolGetDatum(false);
> - break;
> + default:
> +   Assert(i == SEQ_COL_CALLED);
> + coldef = makeColumnDef("is_called", BOOLOID, -1, 
> InvalidOid);
> + value[i - 1] = BoolGetDatum(false);
> + break;
> 
> The downside is that any garbage in i will lead to processing as
> SEQ_COL_CALLED. But things are already pretty bad in that case, ISTM, even
> with the proposed patch (or the original code for that matter).
> 
> Regards,
> -David

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

  Only you can decide what is important to you.

Re: Debian 12 gcc warning

[Bruce Momjian]

On Wed, Aug 30, 2023 at 11:16:48AM -0400, Bruce Momjian wrote:
> On Tue, Aug 29, 2023 at 11:30:06PM -0400, Tom Lane wrote:
> > Bruce Momjian  writes:
> > > On Tue, Aug 29, 2023 at 10:18:36AM -0400, Tom Lane wrote:
> > >> That seems like a pretty clear compiler bug, particularly since it just
> > >> appears in this one version.  Rather than contorting our code, I'd
> > >> suggest filing a gcc bug.
> > 
> > > I assume I have to create a test case to report this to the gcc team.  I
> > > tried to create such a test case on gcc 12 but it doesn't generate the
> > > warning.  Attached is my attempt.  Any ideas?  I assume we can't just
> > > tell them to download our software and compile it.
> > 
> > IIRC, they'll accept preprocessed compiler input for the specific file;
> > you don't need to provide a complete source tree.  Per
> > https://gcc.gnu.org/bugs/
> > 
> > Please include all of the following items, the first three of which can 
> > be obtained from the output of gcc -v:
> > 
> > the exact version of GCC;
> > the system type;
> > the options given when GCC was configured/built;
> > the complete command line that triggers the bug;
> > the compiler output (error messages, warnings, etc.); and
> > the preprocessed file (*.i*) that triggers the bug, generated by adding 
> > -save-temps to the complete compilation command, or, in the case of a bug 
> > report for the GNAT front end, a complete set of source files (see below).
> > 
> > Obviously, if you can trim the input it's good, but it doesn't
> > have to be a minimal reproducer.
> 
> Bug submitted, thanks for th preprocessed file tip.

Good news, I was able to prevent the bug by causing compiling of
clauses.c to use -O2 by adding this to src/Makefile.custom:

clauses.o : CFLAGS+=-O2

Here is my submitted bug report:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111240

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

  Only you can decide what is important to you.

Re: [17] CREATE SUBSCRIPTION ... SERVER

[Jeff Davis]

On Wed, 2023-08-30 at 19:11 +0530, Ashutosh Bapat wrote:
> Are you suggesting that SERVERs created with FDW can not be used as
> publishers?

Correct. Without that, how would the subscription know that the FDW
contains valid postgres connection information? I suppose it could
create a connection string out of the options itself and do another
round of validation, is that what you had in mind?

> We can push down a join
> between a replicated table and foreign table down to the foreign
> server.

Interesting idea.

Regards,
Jeff Davis

Re: More new SQL/JSON item methods

[Chapman Flack]

On 2023-08-30 11:18, Chapman Flack wrote:

If I look in [1], am I looking in the right place for the most
current jsonpath draft?


My bad, I see that it is not. Um if I look in [1'], am I then looking
at the same spec you are?

[1'] https://www.ietf.org/archive/id/draft-ietf-jsonpath-base-20.html

Regards,
-Chap

Re: More new SQL/JSON item methods

[Chapman Flack]

Hi,

On 2023-08-29 03:05, Jeevan Chalke wrote:

This commit implements jsonpath .bigint(), .integer(), and .number()
---
This commit implements jsonpath .date(), .time(), .time_tz(),
.timestamp(), .timestamp_tz() methods.
---
This commit implements jsonpath .boolean() and .string() methods.


Writing as an interested outsider to the jsonpath spec, my first
question would be, is there a published jsonpath spec independent
of PostgreSQL, and are these methods in it, and are the semantics
identical?

The question comes out of my experience on a PostgreSQL integration
of XQuery/XPath, which was nontrivial because the w3 specs for those
languages give rigorous definitions of their data types, independently
of SQL, and a good bit of the work was squinting at those types and at
the corresponding PostgreSQL types to see in what ways they were
different, and what the constraints on converting them were. (Some of
that squinting was already done by the SQL committee in the SQL/XML
spec, which has plural pages on how those conversions have to happen,
especially for the date/time types.)

If I look in [1], am I looking in the right place for the most
current jsonpath draft?

(I'm a little squeamish reading as a goal "cover only essential
parts of XPath 1.0", given that XPath 1.0 is the one w3 threw away
so XPath 2.0 wouldn't have the same problems.)

On details of the patch itself, I only have quick first impressions,
like:

- surely there's a more direct way to make boolean from numeric
  than to serialize the numeric and parse an int?

- I notice that .bigint() and .integer() finish up by casting the
  value to numeric so the existing jbv->val.numeric can hold it.
  That may leave some opportunity on the table: there is another
  patch under way [2] that concerns quickly getting such result
  values from json operations to the surrounding SQL query. That
  could avoid the trip through numeric completely if the query
  wants a bigint, if there were a val.bigint in JsonbValue.

  But of course that would complicate everything else that
  touches JsonbValue. Is there a way for a jsonpath operator to
  determine that it's the terminal operation in the path, and
  leave a value in val.bigint if it is, or build a numeric if
  it's not? Then most other jsonpath code could go on expecting
  a numeric value is always in val.numeric, and the only code
  checking for a val.bigint would be code involved with
  getting the result value out to the SQL caller.

Regards,
-Chap


[1] 
https://www.ietf.org/archive/id/draft-goessner-dispatch-jsonpath-00.html

[2] https://commitfest.postgresql.org/44/4476/

Re: Debian 12 gcc warning

[Bruce Momjian]

On Tue, Aug 29, 2023 at 11:30:06PM -0400, Tom Lane wrote:
> Bruce Momjian  writes:
> > On Tue, Aug 29, 2023 at 10:18:36AM -0400, Tom Lane wrote:
> >> That seems like a pretty clear compiler bug, particularly since it just
> >> appears in this one version.  Rather than contorting our code, I'd
> >> suggest filing a gcc bug.
> 
> > I assume I have to create a test case to report this to the gcc team.  I
> > tried to create such a test case on gcc 12 but it doesn't generate the
> > warning.  Attached is my attempt.  Any ideas?  I assume we can't just
> > tell them to download our software and compile it.
> 
> IIRC, they'll accept preprocessed compiler input for the specific file;
> you don't need to provide a complete source tree.  Per
> https://gcc.gnu.org/bugs/
> 
> Please include all of the following items, the first three of which can 
> be obtained from the output of gcc -v:
> 
> the exact version of GCC;
> the system type;
> the options given when GCC was configured/built;
> the complete command line that triggers the bug;
> the compiler output (error messages, warnings, etc.); and
> the preprocessed file (*.i*) that triggers the bug, generated by adding 
> -save-temps to the complete compilation command, or, in the case of a bug 
> report for the GNAT front end, a complete set of source files (see below).
> 
> Obviously, if you can trim the input it's good, but it doesn't
> have to be a minimal reproducer.

Bug submitted, thanks for th preprocessed file tip.

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

  Only you can decide what is important to you.

Re: [PATCH] Add native windows on arm64 support

[Anthony Roberts]

Hi,

Just a follow-up: having spoken to the relevant people, sadly, right now,
we do not have the capacity to be pulling machines out of our day-to-day CI
tasks to dedicate to specific projects.

I can, however, offer a few alternatives:

1. If you have capacity on your linux x64 machines, it is possible to
emulate WoA, and run tests that way:
https://www.linaro.org/blog/emulate-windows-on-arm/
This is something that works well for projects that we have contributed it
to (ie, sse2neon uses it via github actions) - if you run into any issues
with this, we are able to try and fix them on our side, as likely more than
postgres would benefit.

2. If there is any scope for it on your side at all, a Samsung Galaxy Book
Go 14" can be had from ebay for ~£140.
Understandably, this might not be an option for you.

If it helps, if this is merged, we do have the possibility of running our
own downstream CI builds nightly for the tip of your main branch here:
https://gitlab.com/Linaro/windowsonarm/nightly
This is something we do for a few projects that do not have full upstream
CI.

Thanks,
Anthony


On Fri, 25 Aug 2023 at 11:34, Anthony Roberts 
wrote:

> Thanks for the link - that looks basically the same as what we do for
> CMake (option 2), so should be relatively easy.
>
> I will have a chat to relevant people about setting a machine up
> properly for it.
>
> Thanks,
> Anthony
>
> On 25/08/2023 11:17, Michael Paquier wrote:
> > On Fri, Aug 25, 2023 at 10:40:39AM +0100, Anthony Roberts wrote:
> >> Which of these are you looking for us to provide:
> >>
> >> * A machine for you to directly access (via a VPN)
> >> * A machine we just run a worker script on that automatically picks up
> the
> >> builds as required
> >> * Us to do downstream CI
> >>
> >> All are possible, but preferably not option 1, as it would mean
> straight up
> >> pulling out a machine from our build farm, and it has to go through all
> >> sorts of approvals internally. If it's the only way forward I can kick
> it up
> >> the chain though.
> >>
> >> Option 2 and 3 are ones we do for various other projects (ie. 2 -
> CMake, 3 -
> >> OpenSSL)
> > The community has its own CI facility.  Here is a link of how to set
> > up a machine:
> > https://wiki.postgresql.org/wiki/PostgreSQL_Buildfarm_Howto
> >
> > And all the results are published by the maintainers of the machines
> > on a periodic basis where the buildfarm client code is set:
> > https://buildfarm.postgresql.org/cgi-bin/show_status.pl
> >
> > The final result would be for you to maintain the machine so as we are
> > able to see if anything breaks with it.  Adding extra dependencies to
> > the build like OpenSSL would be nice, but based on the contents of the
> > patch to add this port that does not seem mandatory to me, either.
> >
> > Once the machine is ready, you will need to request a buildfarm
> > machine name and a key to be able to begin publishing the reports of
> > the builds to the community buildfarm.  Once the machine is ready to
> > go, just let me know and I'd be OK to merge the patch (I still want to
> > do a final review of it in case I've missed something, but I can move
> > on with that before the buildfarm machine is set up).
> > --
> > Michael
>

Re: pg_stat_get_backend_subxact() and backend IDs?

[Robert Haas]

On Wed, Aug 30, 2023 at 10:27 AM Nathan Bossart
 wrote:
> On Wed, Aug 30, 2023 at 09:50:41AM -0400, Robert Haas wrote:
> > Sorry, I'm only just noticing this thread. Thanks, Nathan, Ian, and
> > others, for your work on this. Apart from hoping that the 0002 patch
> > will get a more detailed commit message spelling out the problem very
> > explicitly, I don't have any comments on the proposed patches.
>
> I'm about to spend way too much time writing the commit message for 0002,
> but I plan to commit both patches sometime today.

Thanks! I'm glad your committing the patches, and I approve of you
spending way too much time on the commit message. :-)

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

Re: tablecmds.c/MergeAttributes() cleanup

[Alvaro Herrera]

On 2023-Aug-29, Nathan Bossart wrote:

> On Tue, Aug 29, 2023 at 08:44:02PM +0200, Alvaro Herrera wrote:

> > Makes sense.  However, maybe we should replace those ugly defines and
> > their hardcoded values in DefineSequence with a proper array with their
> > names and datatypes.
> 
> That might be an improvement, but IIUC you'd still need to enumerate all of
> the columns or data types to make sure you use the right get-Datum
> function.  It doesn't help that last_value uses Int64GetDatumFast and
> log_cnt uses Int64GetDatum.  I could be missing something, though.

Well, for sure I meant to enumerate everything that was needed,
including the initializer for the value.  Like in the attached patch.

However, now that I've actually written it, I don't find it so pretty
anymore, but maybe that's just because I don't know how to write the
array assignment as a single statement instead of a separate statement
for each column.

But this should silence the warnings, anyway.

-- 
Álvaro HerreraBreisgau, Deutschland  —  https://www.EnterpriseDB.com/
commit 50ac445d1e1ad84282d59c09a3937c221d94b968
Author: Alvaro Herrera  [Álvaro Herrera ]
AuthorDate: Wed Aug 30 16:09:52 2023 +0200
CommitDate: Wed Aug 30 16:41:06 2023 +0200

seq-warnings

diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index 0b0003fcc8..4798c9cd20 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -129,11 +129,16 @@ DefineSequence(ParseState *pstate, CreateSeqStmt *seq)
 	Relation	rel;
 	HeapTuple	tuple;
 	TupleDesc	tupDesc;
-	Datum		value[SEQ_COL_LASTCOL];
-	bool		null[SEQ_COL_LASTCOL];
 	Datum		pgs_values[Natts_pg_sequence];
 	bool		pgs_nulls[Natts_pg_sequence];
-	int			i;
+	struct SequenceColumn
+	{
+		const char *name;
+		Oid			type;
+		Datum		init_value;
+	}			sequence_cols[3];
+	Datum		value[3];
+	bool		null[3];
 
 	/*
 	 * If if_not_exists was given and a relation with the same name already
@@ -166,32 +171,39 @@ DefineSequence(ParseState *pstate, CreateSeqStmt *seq)
 , ,
 _seq_rewrite, _by);
 
+	sequence_cols[0] = (struct SequenceColumn)
+	{
+		.name = "last_value",
+			.type = INT8OID,
+			.init_value = Int64GetDatumFast(seqdataform.last_value)
+	};
+	sequence_cols[1] = (struct SequenceColumn)
+	{
+		.name = "log_cnt",
+			.type = INT8OID,
+			.init_value = Int64GetDatum((int64) 0)
+	};
+	sequence_cols[2] = (struct SequenceColumn)
+	{
+		.name = "is_called",
+			.type = BOOLOID,
+			.init_value = BoolGetDatum(false)
+	};
+
 	/*
 	 * Create relation (and fill value[] and null[] for the tuple)
 	 */
 	stmt->tableElts = NIL;
-	for (i = SEQ_COL_FIRSTCOL; i <= SEQ_COL_LASTCOL; i++)
+	for (int i = 0; i < lengthof(sequence_cols); i++)
 	{
 		ColumnDef  *coldef;
 
-		switch (i)
-		{
-			case SEQ_COL_LASTVAL:
-coldef = makeColumnDef("last_value", INT8OID, -1, InvalidOid);
-value[i - 1] = Int64GetDatumFast(seqdataform.last_value);
-break;
-			case SEQ_COL_LOG:
-coldef = makeColumnDef("log_cnt", INT8OID, -1, InvalidOid);
-value[i - 1] = Int64GetDatum((int64) 0);
-break;
-			case SEQ_COL_CALLED:
-coldef = makeColumnDef("is_called", BOOLOID, -1, InvalidOid);
-value[i - 1] = BoolGetDatum(false);
-break;
-		}
-
+		coldef = makeColumnDef(sequence_cols[i].name, sequence_cols[i].type, -1,
+			   InvalidOid);
 		coldef->is_not_null = true;
-		null[i - 1] = false;
+
+		value[i] = sequence_cols[i].init_value;
+		null[i] = false;
 
 		stmt->tableElts = lappend(stmt->tableElts, coldef);
 	}
diff --git a/src/include/commands/sequence.h b/src/include/commands/sequence.h
index 7db7b3da7b..784280b26d 100644
--- a/src/include/commands/sequence.h
+++ b/src/include/commands/sequence.h
@@ -31,17 +31,6 @@ typedef struct FormData_pg_sequence_data
 
 typedef FormData_pg_sequence_data *Form_pg_sequence_data;
 
-/*
- * Columns of a sequence relation
- */
-
-#define SEQ_COL_LASTVAL			1
-#define SEQ_COL_LOG2
-#define SEQ_COL_CALLED			3
-
-#define SEQ_COL_FIRSTCOL		SEQ_COL_LASTVAL
-#define SEQ_COL_LASTCOL			SEQ_COL_CALLED
-
 /* XLOG stuff */
 #define XLOG_SEQ_LOG			0x00
 

Re: pg_stat_get_backend_subxact() and backend IDs?

[Nathan Bossart]

On Wed, Aug 30, 2023 at 09:50:41AM -0400, Robert Haas wrote:
> Sorry, I'm only just noticing this thread. Thanks, Nathan, Ian, and
> others, for your work on this. Apart from hoping that the 0002 patch
> will get a more detailed commit message spelling out the problem very
> explicitly, I don't have any comments on the proposed patches.

I'm about to spend way too much time writing the commit message for 0002,
but I plan to commit both patches sometime today.

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

Re: tablecmds.c/MergeAttributes() cleanup

[Peter Eisentraut]

On 29.08.23 19:45, Nathan Bossart wrote:

On Tue, Aug 29, 2023 at 10:43:39AM +0200, Peter Eisentraut wrote:

I have committed a few more patches from this series that were already
agreed upon.  The remaining ones are rebased and reordered a bit, attached.


My compiler is complaining about 1fa9241b:

../postgresql/src/backend/commands/sequence.c: In function ‘DefineSequence’:
../postgresql/src/backend/commands/sequence.c:196:21: error: ‘coldef’ may be 
used uninitialized in this function [-Werror=maybe-uninitialized]
   196 |   stmt->tableElts = lappend(stmt->tableElts, coldef);
   | ^~~~

This went away when I added a default case that ERROR'd or initialized
coldef to NULL.


fixed

Re: New compiler warning

[David Steele]

On 8/30/23 08:10, Aleksander Alekseev wrote:



I am seeing a new gcc 12.2.0 compiler warning from
src/backend/commands/sequence.c:


Yep, the compiler is just not smart enough to derive that this
actually is not going to happen.

Here is a proposed fix.


Here's an alternate way to deal with this which is a bit more efficient 
(code not tested):


-   case SEQ_COL_CALLED:
-   coldef = makeColumnDef("is_called", BOOLOID, -1, 
InvalidOid);
-   value[i - 1] = BoolGetDatum(false);
-   break;
+   default:
+   Assert(i == SEQ_COL_CALLED);
+   coldef = makeColumnDef("is_called", BOOLOID, -1, 
InvalidOid);
+   value[i - 1] = BoolGetDatum(false);
+   break;

The downside is that any garbage in i will lead to processing as 
SEQ_COL_CALLED. But things are already pretty bad in that case, ISTM, 
even with the proposed patch (or the original code for that matter).


Regards,
-David

Re: pg_stat_get_backend_subxact() and backend IDs?

[Robert Haas]

On Wed, Aug 30, 2023 at 12:13 AM Michael Paquier  wrote:
> Yep, that looks more consistent, at quick glance.

Sorry, I'm only just noticing this thread. Thanks, Nathan, Ian, and
others, for your work on this. Apart from hoping that the 0002 patch
will get a more detailed commit message spelling out the problem very
explicitly, I don't have any comments on the proposed patches.

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

Re: [17] CREATE SUBSCRIPTION ... SERVER

[Tom Lane]

Jeff Davis  writes:
> The server "myserver" must have been created with the new syntax:
>CREATE SERVER myserver FOR CONNECTION ONLY
> instead of specifying FOREIGN DATA WRAPPER. In other words, a server
> FOR CONNECTION ONLY doesn't have a real FDW, it's a special server just
> used for the postgres connection options.

This seems like it requires a whole lot of new mechanism (parser
and catalog infrastructure) that could be done far more easily
in other ways.  In particular, how about inventing a built-in
dummy FDW to serve the purpose?  That could have some use for
other testing as well.

regards, tom lane

Re: Extract numeric filed in JSONB more effectively

[Chapman Flack]

On 2023-08-30 00:47, Andy Fan wrote:

see what it is.  Suppose the original query is:

numeric(jsonb_object_field(v_jsonb, text)) -> numeric.
...
However the declared type of jsonb_object_field_type is:

jsonb_object_field_type(internal, jsonb, text) -> internal.

So the situation is: b).  We return a NUMERIC type which matches
the original query ...
 case b) shouldn't be treat as an issue.


*We* may know we are returning a NUMERIC type which matches the
original query, but nothing else knows that. Anything that
examined the complete tree after our rewriting would see some
expression that wants a numeric type, but supplied with a
subexpression that returns internal. Without a relabel node
there to promise that we know this internal is really numeric,
any type checker would reject the tree.

The fact that it even works at all without a relabel node there
seems to indicate that all of PostgreSQL's type checking was
done before calling the support function, and that there is not
much sanity checking of what the support function returns,
which I guess is efficient, if a little scary. Seems like
writing a support function is a bit like trapeze performing
without a net.


So your new method is:
1. jsonb_{op}_start() ->  internal  (internal actually JsonbValue).
2. jsonb_finish_{type}(internal, ..) -> type.   (internal is JsonbValue 
).


This avoids the case a) at the very beginning.  I'd like to provides
patches for both solutions for comparison.


I think, unavoidably, there is still a case a) at the very beginning,
just because of the rule that if json_{op}_start is going to have an
internal return type, it needs to have at least one internal parameter
to prevent casual calls from SQL, even if that parameter is not used
for anything.

It would be ok to write in a Const for that parameter, just zero or
42 or anything besides null (in case the function is strict), but
again if the Const has type internal then EXPLAIN will be sad, so
it has to be some type that makes EXPLAIN cheerful, and relabeled
internal.

But with this approach there is no longer a type mismatch of the
end result.


fcinfo initialization in DirectFunctionCall1 is an interesting point!
so  I am persuaded the extra steps in  ExprState may not be
worse than the current way due to the "every-time-through
fcinfo initialization" (in which case the memory is allocated
once in heap rather than every time in stack).


Stack allocation is super cheap, just by emitting the function
entry to reserve n+m bytes instead of just m, so it there's any
measurable cost to the DirectFunctionCall I would think it more
likely to be in the initialization after allocation ... but I
haven't looked at that code closely to see how much there is.
I just wanted to make the point that another step or two in
ExprState might not be a priori worse. We might be talking
about negligible effects in either direction.


I can do a
comparison at last to see if we can find some other interesting
findings.


That would be the way to find out. I think I would still lean
toward the approach with less code duplication, unless there
is a strong timing benefit the other way.


True, reusing the casting system should be better than hard-code
the casting function manually.  I'd apply this on both methods.


I noticed there is another patch registered in this CF: [1]
It adds new operations within jsonpath like .bigint .time
and so on.

I was wondering whether that work would be conflicting or
complementary with this. It looks to be complementary. The
operations being added there are within jsonpath evaluation.
Here we are working on faster ways to get those results out.

It does not seem that [1] will add any new choices in
JsonbValue. All of its (.bigint .integer .number) seem to
verify the requested form and then put the result as a
numeric in ->val.numeric. So that doesn't add any new
cases for this patch to handle. (Too bad, in a way: if that
other patch added ->val.bigint, this patch could add a case
to retrieve that value without going through the work of
making a numeric. But that would complicate other things
touching JsonbValue, and be a matter for that other patch.)

It may be expanding the choices for what we might one day
find in ->val.datetime though.

Regards,
-Chap


[1] https://commitfest.postgresql.org/44/4526/

Re: [17] CREATE SUBSCRIPTION ... SERVER

[Ashutosh Bapat]

Hi Jeff,

On Wed, Aug 30, 2023 at 2:12 PM Jeff Davis  wrote:
>
> The server "myserver" must have been created with the new syntax:
>
>CREATE SERVER myserver FOR CONNECTION ONLY
>
> instead of specifying FOREIGN DATA WRAPPER. In other words, a server
> FOR CONNECTION ONLY doesn't have a real FDW, it's a special server just
> used for the postgres connection options. To create a server FOR
> CONNECTION ONLY, the user must be a member of the new predefined role
> pg_create_connection. A server FOR CONNECTION ONLY still uses ACLs and
> user mappings the same way as other foreign servers, but cannot be used
> to create foreign tables.

Are you suggesting that SERVERs created with FDW can not be used as
publishers? I think there's value in knowing that the publisher which
contains a replica of a table is the same as the foreign server which
is referenced by another foreign table. We can push down a join
between a replicated table and foreign table down to the foreign
server. A basic need for sharding with replicated tables. Of course
there's a lot work that we have to do in order to actually achieve
such a push down but by restricting this feature to only CONNECTION
ONLY, we are restricting the possibility of such a push down.

-- 
Best Wishes,
Ashutosh Bapat

Re: Disabling Heap-Only Tuples

[Robert Haas]

On Wed, Aug 30, 2023 at 9:31 AM Robert Haas  wrote:
> That seems like a good chance, but

*change

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

Re: Disabling Heap-Only Tuples

[Robert Haas]

On Wed, Aug 30, 2023 at 9:01 AM Matthias van de Meent
 wrote:
> I've reworked the patch a bit to remove the "excessive bloat with low
> fillfactors when local space is available" issue that this parameter
> could cause - local updates are now done if the selected page we would
> be inserting into is after the old tuple's page and the old tuple's
> page still (or: now) has space available.
>
> Does that alleviate your concerns?

That seems like a good chance, but my core concern is around people
having to micromanage local_update_limit, and probably either not
knowing how to do it properly, or not being able or willing to keep
updating it as things change.

In a way, this parameter is a lot like work_mem, which is notoriously
very difficult to tune. If you set it too high, you run out of memory.
If you set it too low, you get bad plans. You can switch from having
one of those problems to having the other very quickly as load changs,
and sometimes you can have both at the same time. If an omniscient
oracle could set work_mem properly for every query based not only on
what the query does but the state of the system at that moment, it
would still be a very crude parameter, and since omniscient oracles
are rare in practice, problems are reasonably common. I think that if
we add this parameter, it's going to end up in the same category. A
lot of people will ignore it, and they'll be OK, but 30% of the people
who do try to use it will shoot themselves in the foot, or something
like that.

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

Re: persist logical slots to disk during shutdown checkpoint

[Ashutosh Bapat]

On Tue, Aug 29, 2023 at 5:40 PM Ashutosh Bapat
 wrote:
>
> I am looking at it. If you can wait till the end of the week, that
> will be great.

 /*
  * Successfully wrote, unset dirty bit, unless somebody dirtied again
- * already.
+ * already and remember the confirmed_flush LSN value.
  */
 SpinLockAcquire(>mutex);
 if (!slot->just_dirtied)
 slot->dirty = false;
+slot->last_saved_confirmed_flush = slot->data.confirmed_flush;

If the slot->data.confirmed_flush gets updated concurrently between copying it
to be written to the disk and when it's written to last_saved_confirmed_flush,
we will miss one update. I think we need to update last_saved_confirmed_flush
based on the cp.slotdata.confirmed_flush rather than
slot->data.confirmed_flush.

We are setting last_saved_confirmed_flush for all types of slots but using it
only when the slot is logical. Should we also set it only for logical slots?

 /* first check whether there's something to write out */
 SpinLockAcquire(>mutex);
 was_dirty = slot->dirty;
 slot->just_dirtied = false;
+confirmed_flush_has_changed = (slot->data.confirmed_flush !=
slot->last_saved_confirmed_flush);

The confirmed_flush LSN should always move forward, otherwise there may not be
enough WAL retained for the slot to work. I am wondering whether we should take
an opportunity to make sure
Assert(slot->data.confirmed_flush <= slot->last_saved_confirmed_flush)

-/* and don't do anything if there's nothing to write */
-if (!was_dirty)
+/* Don't do anything if there's nothing to write. See ReplicationSlot. */
+if (!was_dirty &&
+!(is_shutdown && SlotIsLogical(slot) && confirmed_flush_has_changed))

Rather than complicating this condition, I wonder whether it's better to just
set was_dirty = true when is_shutdown && SlotIsLogical(slot) &&
confirmed_flush_has_changed) or even slot->dirty = true. See also the note at
the end of the email.

+
+/*
+ * We won't ensure that the slot is persisted after the confirmed_flush
+ * LSN is updated as that could lead to frequent writes.  However, we need
+ * to ensure that we do persist the slots at the time of shutdown whose
+ * confirmed_flush LSN is changed since we last saved the slot to disk.
+ * This will help in avoiding retreat of the confirmed_flush LSN after
+ * restart.  This variable is used to track the last saved confirmed_flush
+ * LSN value.
+ */

This comment makes more sense in SaveSlotToPath() than here. We may decide to
use last_saved_confirmed_flush for something else in future.
+
+sub compare_confirmed_flush
+{
+my ($node, $confirmed_flush_from_log) = @_;
+
+# Fetch Latest checkpoint location from the control file
+my ($stdout, $stderr) =
+  run_command([ 'pg_controldata', $node->data_dir ]);
+my @control_data  = split("\n", $stdout);
+my $latest_checkpoint = undef;
+foreach (@control_data)
+{
+if ($_ =~ /^Latest checkpoint location:\s*(.*)$/mg)
+{
+$latest_checkpoint = $1;
+last;
+}
+}
+die "Latest checkpoint location not found in control file\n"
+  unless defined($latest_checkpoint);
+
+# Is it same as the value read from log?
+ok( $latest_checkpoint eq $confirmed_flush_from_log,
+"Check that the slot's confirmed_flush LSN is the same as the
latest_checkpoint location"
+);

This function assumes that the subscriber will receive and confirm WAL upto
checkpoint location and publisher's WAL sender will update it in the slot.
Where is the code to ensure that? Does the WAL sender process wait for
checkpoint
LSN to be confirmed when shutting down?
+
+# Restart the publisher to ensure that the slot will be persisted if required
+$node_publisher->restart();

Can we add this test comparing LSNs after publisher restart, to an existing
test itself - like basic replication. That's the only extra thing that this
test does beyond usual replication stuff.

+
+# Wait until the walsender creates decoding context
+$node_publisher->wait_for_log(
+qr/Streaming transactions committing after
([A-F0-9]+\/[A-F0-9]+), reading WAL from ([A-F0-9]+\/[A-F0-9]+)./,
+$offset);
+
+# Extract confirmed_flush from the logfile
+my $log_contents = slurp_file($node_publisher->logfile, $offset);
+$log_contents =~
+  qr/Streaming transactions committing after ([A-F0-9]+\/[A-F0-9]+),
reading WAL from ([A-F0-9]+\/[A-F0-9]+)./
+  or die "could not get confirmed_flush_lsn";

Why are we figuring out the LSN from the log file? Is it not available from
pg_replication_slots view? If we do so, I think this test will fail is the slot
gets written after the restart because of concurrent activity on the publisher
(like autovacuum, or other things that cause empty transaction to be
replicated) and subscriber. A very rare chance but not 0 probability one. I
think we should shut down subscriber, restart publisher and then make this
check based on 

Re: Disabling Heap-Only Tuples

[Matthias van de Meent]

On Mon, 28 Aug 2023 at 17:57, Robert Haas  wrote:
>
> On Mon, Aug 28, 2023 at 11:50 AM Matthias van de Meent
>  wrote:
> > Agreed on all points. But isn't that true for most most tools on bloat
> > prevention and/or detection? E.g. fillfactor, autovacuum_*, ...
>
> Not nearly to the same extent, IMHO. A lot of those parameters can be
> left alone forever and you lose nothing. That's not so here.

I've reworked the patch a bit to remove the "excessive bloat with low
fillfactors when local space is available" issue that this parameter
could cause - local updates are now done if the selected page we would
be inserting into is after the old tuple's page and the old tuple's
page still (or: now) has space available.

Does that alleviate your concerns?

Kind regards,

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


v2-0001-Add-heap-reloption-local_update_limit.patch
Description: Binary data

Re: Commitfest manager for September

[Magnus Hagander]

On Wed, Aug 30, 2023 at 2:50 PM Peter Eisentraut  wrote:
>
> On 28.08.23 15:46, Peter Eisentraut wrote:
> > I would like to be the commitfest manager for CF 2023-09.
>
> I think my community account needs to have some privilege change to be
> able to act as CF manager in the web interface.  Could someone make that
> happen?

Done!

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

Re: Commitfest manager for September

[Peter Eisentraut]

On 28.08.23 15:46, Peter Eisentraut wrote:

I would like to be the commitfest manager for CF 2023-09.


I think my community account needs to have some privilege change to be 
able to act as CF manager in the web interface.  Could someone make that 
happen?

pg_resetwal tests, logging, and docs update

[Peter Eisentraut]

I noticed that pg_resetwal has poor test coverage.  There are some TAP 
tests, but they all run with -n, so they don't actually test the full 
functionality.  (There is a non-dry-run call of pg_resetwal in the 
recovery test suite, but that is incidental.)


So I added a bunch of more tests to test all the different options and 
scenarios.  That also led to adding more documentation about more 
details how some of the options behave, and some tweaks in the program 
output.


It's attached as one big patch, but it could be split into smaller 
pieces, depending what gets agreement.From a8f1f7b84d38d5130f3bd8426be39cbc2c6866c3 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Wed, 30 Aug 2023 14:00:01 +0200
Subject: [PATCH v1] pg_resetwal: More tests, and some output improvements

- More tests and test coverage
- Update an obsolete comment
- Regroup --help output
- Improve documentation about pg_resetwal -f option
- Improve error output
---
 doc/src/sgml/ref/pg_resetwal.sgml  |  70 ---
 src/bin/pg_resetwal/pg_resetwal.c  |  58 +++--
 src/bin/pg_resetwal/t/001_basic.pl | 114 +
 src/bin/pg_resetwal/t/002_corrupted.pl |   4 +
 4 files changed, 209 insertions(+), 37 deletions(-)

diff --git a/doc/src/sgml/ref/pg_resetwal.sgml 
b/doc/src/sgml/ref/pg_resetwal.sgml
index fd539f5604..d47f9552b8 100644
--- a/doc/src/sgml/ref/pg_resetwal.sgml
+++ b/doc/src/sgml/ref/pg_resetwal.sgml
@@ -52,21 +52,33 @@ Description
   
 
   
-   After running this command, it should be possible to start the server,
+   Some options, such as --wal-segsize (see below), can also
+   be used to modify certain global settings of a database cluster without the
+   need to rerun initdb.  This can be done safely on an
+   otherwise sound database cluster, if none of the dangerous modes mentioned
+   below are used.
+  
+
+  
+   If pg_resetwal is used on a data directory where the
+   server has been cleanly shut down and the control file is sound, then it
+   will have no effect on the contents of the database system, except that no
+   longer used WAL files are cleared away.  Any other use is potentially
+   dangerous and must be done with great care.  pg_resetwal
+   will require the -f (force) option to be specified before
+   working on a data directory in an unclean shutdown state or with a corrupt
+   control file.
+  
+
+  
+   After running this command on a data directory with corrupted WAL or a
+   corrupt control file, it should be possible to start the server,
but bear in mind that the database might contain inconsistent data due to
partially-committed transactions.  You should immediately dump your data,
run initdb, and restore.  After restore, check for
inconsistencies and repair as needed.
   
 
-  
-   This utility can only be run by the user who installed the server, because
-   it requires read/write access to the data directory.
-   For safety reasons, you must specify the data directory on the command line.
-   pg_resetwal does not use the environment variable
-   PGDATA.
-  
-
   
If pg_resetwal complains that it cannot determine
valid data for pg_control, you can force it to proceed 
anyway
@@ -82,19 +94,41 @@ Description
execute any data-modifying operations in the database before you dump,
as any such action is likely to make the corruption worse.
   
+
+  
+   This utility can only be run by the user who installed the server, because
+   it requires read/write access to the data directory.
+  
  
 
  
   Options
 
   
+   
+datadir
+-D datadir
+--pgdata=datadir
+
+ 
+  Specifies the location of the database directory.
+  For safety reasons, you must specify the data directory on the command
+  line.  pg_resetwal does not use the environment
+  variable PGDATA.
+ 
+
+   
+

 -f
 --force
 
  
-  Force pg_resetwal to proceed even if it cannot 
determine
-  valid data for pg_control, as explained above.
+  Force pg_resetwal to proceed even in situations where
+  it could be dangerous, as explained above.  Specifically, this option is
+  required to proceed if the server had not been cleanly shut down, or if
+  pg_resetwal cannot determine valid data for
+  pg_control.
  
 

@@ -132,7 +166,8 @@ Options
pg_resetwal is unable to determine appropriate values
by reading pg_control.  Safe values can be determined 
as
described below.  For values that take numeric arguments, hexadecimal
-   values can be specified by using the prefix 0x.
+   values can be specified by using the prefix 0x. Note
+   that these instructions only apply with the standard block size of 8 kB.
   
 
   
@@ -155,6 +190,7 @@ Options
   greatest file name in the same directory.  The file names are in
   hexadecimal.
  
+  
 

 
@@ -238,6 +274,7 @@ Options
   names are in hexadecimal, so the easiest way to do this is to 

Re: PATCH: Add REINDEX tag to event triggers

[Jim Jones]

Greetings

On 27.07.23 06:43, Garrett Thornburg wrote:
I added a v2 patch for adding REINDEX to event triggers. The following 
has changed:


1. I fixed the docs to note that ddl_command_start is supported for 
REINDEX. Thanks, Michael!
2. I added Jian He's excellent partition table test and updated the 
expectations to include that regression test.


What is still up for debate:

Michael pointed out that REINDEX probably isn't DDL because the docs 
only specify commands in the standard like CREATE, ALTER, etc. It 
might be worth creating a new event to trigger on (similar to what was 
done for table_rewrite) to make a REINDEX trigger fit in a little 
nicer. Let me know what you think here. Until then, I left the code as 
`lev = LOGSTMT_DDL` for `T_ReindexStmt`.


Thanks,
Garrett


Thanks for the patch, Garrett!

I was unable to apply it in 7ef5f5f 



$ git apply -v v2-0001-Add-REINDEX-tag-to-event-triggers.patch
Checking patch doc/src/sgml/event-trigger.sgml...
Checking patch src/backend/commands/indexcmds.c...
error: while searching for:
static List *ChooseIndexColumnNames(List *indexElems);
static void ReindexIndex(RangeVar *indexRelation, ReindexParams *params,
 bool isTopLevel);
static void RangeVarCallbackForReindexIndex(const RangeVar *relation,
Oid relId, Oid oldRelId, void *arg);
static Oid  ReindexTable(RangeVar *relation, ReindexParams *params,

error: patch failed: src/backend/commands/indexcmds.c:94
error: src/backend/commands/indexcmds.c: patch does not apply
Checking patch src/backend/tcop/utility.c...
Checking patch src/include/tcop/cmdtaglist.h...
Checking patch src/test/regress/expected/event_trigger.out...
Hunk #1 succeeded at 556 (offset 2 lines).
Checking patch src/test/regress/sql/event_trigger.sql...

Am I missing something?

Jim

logfmt and application_context

[Étienne BERSAC]

Hi everyone,

I just release a logfmt log collector for PostgreSQL :
https://pgxn.org/dist/logfmt/1.0.0/ . This works quite well but I have
a few issues I would like to share with hackers.

First, what do you think of having logfmt output along json and CSV ?
PostgreSQL internal syslogger has builtin support for the different
LOG_DESTINATION_*. Thus logfmt does not send log collector headers
using write_syslogger_file or write_pipe_chunks but plain log line with
write_console. Do you have some hint about this ? The consequences ?
How much is it a good bet to write a custom log collector in a shared
preload library ?

Second issue, logfmt provides a guc called
`logfmt.application_context`. The purpose of application_context is the
same as `application_name` but for a more varying value like request
UUID, task ID, etc. What do you think of this ? Would it be cool to
have this GUC in PostgreSQL and available in log_line_prefix ?

Anyway, it's my first attempt at writing C code for PostgreSQL, with
the help of Guillaume LELARGE and Jehan-Guillaume de RORTHAIS and it's
a pleasure ! PostgreSQL C code is very readable. Thanks everyone for
this !

Regards,
Étienne BERSAC
Developer at Dalibo

Re: lockup in parallel hash join on dikkop (freebsd 14.0-current)

[Tomas Vondra]

Hi,

I have another case of this on dikkop (on 11 again). Is there anything
else we'd want to try? Or maybe someone would want access to the machine
and do some investigation directly?

regards

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

Re: Allow specifying a dbname in pg_basebackup connection string

[Jelte Fennema]

On Wed, 30 Aug 2023 at 01:01, Jim Jones  wrote:
> However, pgbouncer closes with a segmentation fault, so I couldn't test the 
> result of pg_basebackup itself - but I guess it isn't the issue here.

Oops it indeed seemed like I made an unintended change when handling
database names that did not exist in pgbouncer.conf when you used
auth_type=hba. I pushed a fix for that now to the replication-support
branch. Feel free to try again. But as you said it's unrelated to the
postgres patch.

Re: New compiler warning

[Aleksander Alekseev]

Hi,

> I am seeing a new gcc 12.2.0 compiler warning from
> src/backend/commands/sequence.c:

Yep, the compiler is just not smart enough to derive that this
actually is not going to happen.

Here is a proposed fix.

-- 
Best regards,
Aleksander Alekseev


v1-0001-Silence-GCC-12-warning.patch
Description: Binary data

New compiler warning

[Bruce Momjian]

I am seeing a new gcc 12.2.0 compiler warning from
src/backend/commands/sequence.c:

sequence.c: In function ‘DefineSequence’:
sequence.c:196:35: warning: ‘coldef’ may be used uninitialized 
[-Wmaybe-uninitialized]
  196 | stmt->tableElts = lappend(stmt->tableElts, 
coldef);
  |   
^~~~
sequence.c:175:29: note: ‘coldef’ was declared here
  175 | ColumnDef  *coldef;
  | ^~

The code is:

for (i = SEQ_COL_FIRSTCOL; i <= SEQ_COL_LASTCOL; i++)
{
--> ColumnDef  *coldef;

switch (i)
{
case SEQ_COL_LASTVAL:
coldef = makeColumnDef("last_value", INT8OID, -1, 
InvalidOid);
value[i - 1] = Int64GetDatumFast(seqdataform.last_value);
break;
case SEQ_COL_LOG:
coldef = makeColumnDef("log_cnt", INT8OID, -1, InvalidOid);
value[i - 1] = Int64GetDatum((int64) 0);
break;
case SEQ_COL_CALLED:
coldef = makeColumnDef("is_called", BOOLOID, -1, 
InvalidOid);
value[i - 1] = BoolGetDatum(false);
break;
}

coldef->is_not_null = true;
null[i - 1] = false;

--> stmt->tableElts = lappend(stmt->tableElts, coldef);
}

and I think it is caused by this commit:

commit 1fa9241bdd
Author: Peter Eisentraut 
Date:   Tue Aug 29 08:41:04 2023 +0200

Make more use of makeColumnDef()

Since we already have it, we might as well make full use of it,
instead of assembling ColumnDef by hand in several places.

Reviewed-by: Alvaro Herrera 
Discussion: 
https://www.postgresql.org/message-id/flat/52a125e4-ff9a-95f5-9f61-b87cf447e...@eisentraut.org

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

  Only you can decide what is important to you.

Re: New WAL record to detect the checkpoint redo location

[Dilip Kumar]

On Wed, Aug 30, 2023 at 1:03 PM Michael Paquier  wrote:
>
> On Mon, Aug 28, 2023 at 01:47:18PM +0530, Dilip Kumar wrote:
> > I removed this mainly because now in other comments[1] where we are
> > introducing this new CHECKPOINT_REDO record we are explaining the
> > problem
> > that the redo location and the actual checkpoint records are not at
> > the same place and that is because of the concurrent xlog insertion.
> > I think we are explaining in more
> > detail by also stating that in case of a shutdown checkpoint, there
> > would not be any concurrent insertion so the shutdown checkpoint redo
> > will be at the same place.  So I feel keeping old comments is not
> > required.
> > And we are explaining it when we are setting this for
> > non-shutdown checkpoint because for shutdown checkpoint this statement
> > is anyway not correct because for the shutdown checkpoint the
> > checkpoint record will be at the same location and there will be no
> > concurrent wal insertion, what do you think?
>
> +* Insert a dummy CHECKPOINT_REDO record and set start LSN of this record
> +* as checkpoint.redo.
>
> I would add a "for a non-shutdown checkpoint" at the end of this
> sentence.
>
> +* record.  So when processing the wal, we cannot rely on the checkpoint
> +* record if we want to stop at the checkpoint-redo LSN.
>
> The term "checkpoint-redo" is also a bit confusing, I guess, because
> you just mean to refer to the "redo" LSN here?  Maybe rework the last
> sentence as:
> "So, when processing WAL, we cannot rely on the checkpoint record if
> we want to stop at the same position as the redo LSN".
>
> +* This special record, however, is not required when we are doing a
> +* shutdown checkpoint, as there will be no concurrent wal insertions
> +* during that time.  So, the shutdown checkpoint LSN will be the same as
> +* checkpoint-redo LSN.
>
> Perhaps the last sentence could be merged with the first one, if we
> are tweaking things, say:
> "This special record is not required when doing a shutdown checkpoint;
> the redo LSN is the same LSN as the checkpoint record as there cannot
> be any WAL activity in a shutdown sequence."

Your suggestions LGTM so modified accordingly

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


v5-0001-New-WAL-record-for-checkpoint-redo-location.patch
Description: Binary data

pg_upgrade bug: pg_upgrade successes even if the slots are defined, but they becomes unusable

[Hayato Kuroda (Fujitsu)]

Dear hackers,

While testing pg_upgrade for [1], I found a bug related with logical replication
slots. 

# Found bug

Status of logical replication slots are still "reserved", but they are not 
usable.

```
tmp=# SELECT slot_name, slot_type, restart_lsn, confirmed_flush_lsn, wal_status 
FROM pg_replication_slots;
 slot_name  | slot_type | restart_lsn | confirmed_flush_lsn | wal_status 
+---+-+-+
 new_on_tmp | logical   | 0/196C7B0   | 0/196C7E8   | reserved
(1 row)

tmp=# SELECT * FROM pg_logical_slot_get_changes('new_on_tmp', NULL, NULL);
ERROR:  requested WAL segment pg_wal/00010001 has already been 
removed
```

I did not check about physical slots, but it may also similar problem.

# Condition

This happens when logical slots exist on new cluster before doing pg_upgrade.
It happened for HEAD and REL_16_STABLE branches, but I think it would happen
all supported versions.

## How to reproduce

You can get same ERROR with below steps. Also I attached the script for
reproducing the bug, 

1. do initdb for old and new cluster
2. create logical replication slots only on new cluster. Note that it must be
   done aother database than "postgres".
3. do pg_upgrade.
4. boot new cluster and executed pg_logical_slot_get_changes()

# My analysis

The immediate cause is that pg_resetwal removes WALs required by logical
replication slots, it cannot be skipped.
Therefore, I think it is better not to allow upgrade when replication slots are
defined on the new cluster. I was not sure the case for physical replication,
so I want to hear your opinion.

I will create a patch if it is real problem. Any comments for that are very
welcome.

[1]: 
https://www.postgresql.org/message-id/flat/tyapr01mb58664c81887b3af2eb6b16e3f5...@tyapr01mb5866.jpnprd01.prod.outlook.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED



slots_error.sh
Description: slots_error.sh

Re: Avoid a possible overflow (src/backend/utils/sort/logtape.c)

[Ranier Vilela]

Em ter., 29 de ago. de 2023 às 20:06, Michael Paquier 
escreveu:

> On Thu, Aug 24, 2023 at 07:02:40PM -0700, Peter Geoghegan wrote:
> > FWIW I'm pretty sure that it's impossible to run into problems here in
> > practice -- the minheap is allocated by palloc(), and the high
> > watermark number of free pages is pretty small. Even still, I agree
> > with your conclusion. There is really no reason to not be consistent
> > here.
>
> Postgres 16 RC1 is now tagged, so applied down to 13.
>
Thank you, Michael.

best regards,
Ranier Vilela

Re: [PATCH v1] PQputCopyEnd never returns 0, fix the inaccurate comment

[Aleksander Alekseev]

Hi,

> > On Mon, Aug 28, 2023 at 09:46:07PM +0800, Junwang Zhao wrote:
> > > Yeah, it makes sense to me, or maybe just `PQputCopyEnd(...) == -1`,
> > > let's wait for some other opinions.
> >
> > One can argue that PQputCopyEnd() returning 0 could be possible in an
> > older version of libpq these callers are linking to, but this has
> > never existed from what I can see (just checked down to 8.2 now).
> > Anyway, changing these callers may create some backpatching conflicts,
> > so I'd let them as they are, and just fix the comment.
>
> sure, thanks for the explanation.

The patch was applied in 8bf7db02 [1] and I assume it's safe to close
the corresponding CF entry [2].

Thanks, everyone.

[1]: 
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=8bf7db0285dfbc4b505c8be4c34ab7386eb6297f
[2]: https://commitfest.postgresql.org/44/4521/

-- 
Best regards,
Aleksander Alekseev

Re: Missing comments/docs about custom scan path

[Etsuro Fujita]

Hi Richard,

On Wed, Aug 30, 2023 at 11:05 AM Richard Guo  wrote:
> On Tue, Aug 29, 2023 at 5:08 PM Etsuro Fujita  wrote:
>> Another thing I would like to propose is minor adjustments to the docs
>> related to parallel query:
>>
>> A custom scan provider will typically add paths for a base relation by
>> setting the following hook, which is called after the core code has
>> generated all the access paths it can for the relation (except for
>> Gather paths, which are made after this call so that they can use
>> partial paths added by the hook):
>>
>> For clarity, I think "except for Gather paths" should be "except for
>> Gather and Gather Merge paths".
>>
>> Although this hook function can be used to examine, modify, or remove
>> paths generated by the core system, a custom scan provider will
>> typically confine itself to generating CustomPath objects and adding
>> them to rel using add_path.
>>
>> For clarity, I think "adding them to rel using add_path" should be eg,
>> "adding them to rel using add_path, or using add_partial_path if they
>> are partial paths".

> +1.  I can see that this change makes the doc more consistent with the
> comments in set_rel_pathlist.

Agreed.

I have committed the patch.  Thanks for taking a look!

Best regards,
Etsuro Fujita

Re: Is pg_regress --use-existing used by anyone or is it broken?

[Daniel Gustafsson]

> On 30 Aug 2023, at 00:55, Peter Geoghegan  wrote:
> 
> On Tue, Aug 29, 2023 at 3:37 PM Daniel Gustafsson  wrote:
>>> It's handy when using pg_regress with a custom test suite, where I
>>> don't want to be nagged about disconnecting from the database every
>>> time.
>> 
>> I'm curious about your workflow around it, it seems to me that it's kind of
>> broken so I wonder if we instead then should make it an equal citizen with 
>> temp
>> instance?
> 
> I'm confused. You seem to think that it's a problem that
> --use-existing doesn't create databases and roles. But that's the
> whole point, at least for me.

Well, I think it's problematic that it doesn't handle database and role
creation due to it being buggy.  I'll have another look at fixing the issues to
see if there is more than what I posted upthread, while at the same time making
sure it will still support your use-case.

--
Daniel Gustafsson

Re: Cirrus-ci is lowering free CI cycles - what to do with cfbot, etc?

[Daniel Gustafsson]

> On 28 Aug 2023, at 14:32, Daniel Gustafsson  wrote:

> Attached is a patch with a quick PoC for using PQPing instead of using psql 
> for
> connection checks in pg_regress.

The attached v2 fixes a silly mistake which led to a compiler warning.

--
Daniel Gustafsson



v2-0001-Speed-up-pg_regress-server-testing.patch
Description: Binary data

Re: Test case for parameterized remote path in postgres_fdw

[Etsuro Fujita]

On Wed, Aug 16, 2023 at 6:45 PM Etsuro Fujita  wrote:
> On Wed, Aug 16, 2023 at 9:41 AM Richard Guo  wrote:
> > On Tue, Aug 15, 2023 at 7:50 PM Etsuro Fujita  
> > wrote:
> >> So we should have modified the second one as well?  Attached is a
> >> small patch for that.
>
> > Agreed, nice catch!  +1 to the patch.
>
> Thanks for looking!

Pushed.

Best regards,
Etsuro Fujita

Re: New WAL record to detect the checkpoint redo location

[Michael Paquier]

On Mon, Aug 28, 2023 at 01:47:18PM +0530, Dilip Kumar wrote:
> I removed this mainly because now in other comments[1] where we are
> introducing this new CHECKPOINT_REDO record we are explaining the
> problem
> that the redo location and the actual checkpoint records are not at
> the same place and that is because of the concurrent xlog insertion.
> I think we are explaining in more
> detail by also stating that in case of a shutdown checkpoint, there
> would not be any concurrent insertion so the shutdown checkpoint redo
> will be at the same place.  So I feel keeping old comments is not
> required.
> And we are explaining it when we are setting this for
> non-shutdown checkpoint because for shutdown checkpoint this statement
> is anyway not correct because for the shutdown checkpoint the
> checkpoint record will be at the same location and there will be no
> concurrent wal insertion, what do you think?

+* Insert a dummy CHECKPOINT_REDO record and set start LSN of this record
+* as checkpoint.redo.

I would add a "for a non-shutdown checkpoint" at the end of this
sentence.

+* record.  So when processing the wal, we cannot rely on the checkpoint
+* record if we want to stop at the checkpoint-redo LSN.

The term "checkpoint-redo" is also a bit confusing, I guess, because
you just mean to refer to the "redo" LSN here?  Maybe rework the last
sentence as:
"So, when processing WAL, we cannot rely on the checkpoint record if
we want to stop at the same position as the redo LSN".

+* This special record, however, is not required when we are doing a
+* shutdown checkpoint, as there will be no concurrent wal insertions
+* during that time.  So, the shutdown checkpoint LSN will be the same as
+* checkpoint-redo LSN.

Perhaps the last sentence could be merged with the first one, if we
are tweaking things, say:
"This special record is not required when doing a shutdown checkpoint;
the redo LSN is the same LSN as the checkpoint record as there cannot
be any WAL activity in a shutdown sequence."
--
Michael


signature.asc
Description: PGP signature

[17] CREATE SUBSCRIPTION ... SERVER

[Jeff Davis]

Synopsis:

  Publisher:

CREATE TABLE x(i INT);
CREATE TABLE y(i INT);
INSERT INTO x VALUES(1);
INSERT INTO y VALUES(-1);
CREATE PUBLICATION pub1 FOR TABLE x;
CREATE PUBLICATION pub2 FOR TABLE y;

  Subscriber:

CREATE SERVER myserver FOR CONNECTION ONLY OPTIONS (
  host '...', dbname '...'
);
CREATE USER MAPPING FOR PUBLIC SERVER myserver OPTIONS (
  user '...', password '...'
);

CREATE TABLE x(i INT);
CREATE TABLE y(i INT);
CREATE SUBSCRIPTION sub1 SERVER myserver PUBLICATION pub1;
CREATE SUBSCRIPTION sub2 SERVER myserver PUBLICATION pub2;

Motivation:

  * Allow managing connections separately from managing the
subscriptions themselves. For instance, if you update an
authentication method or the location of the publisher, updating
the server alone will update all subscriptions at once.
  * Enable separating the privileges to create a subscription from the
privileges to create a connection string. (By default
pg_create_subscription has both privileges for compatibility with
v16, but the connection privilege can be revoked from
pg_create_subscription, see below.)
  * Enable changing of single connection parameters without pasting
the rest of the connection string as well. E.g. "ALTER SERVER
... OPTIONS (SET ... '...');".
  * Benefit from user mappings and ACLs on foreign server object if
you have multiple roles creating subscriptions.

Details:

The attached patch implements "CREATE SUBSCRIPTION ... SERVER myserver"
as an alternative to "CREATE SUBSCRIPTION ... CONNECTION '...'". The
user must be a member of pg_create_subscription and have USAGE
privileges on the server.

The server "myserver" must have been created with the new syntax:

   CREATE SERVER myserver FOR CONNECTION ONLY

instead of specifying FOREIGN DATA WRAPPER. In other words, a server
FOR CONNECTION ONLY doesn't have a real FDW, it's a special server just
used for the postgres connection options. To create a server FOR
CONNECTION ONLY, the user must be a member of the new predefined role
pg_create_connection. A server FOR CONNECTION ONLY still uses ACLs and
user mappings the same way as other foreign servers, but cannot be used
to create foreign tables.

The predefined role pg_create_subscription is also a member of the role
pg_create_connection, so that existing members of the
pg_create_subscription role may continue to create subscriptions using
CONNECTION just like in v16 without any additional grant.

Security:

One motivation of this patch is to enable separating the privileges to
create a subscription from the privileges to create a connection
string, because each have their own security implications and may be
done through separate processes. To separate the privileges, simply
revoke pg_create_connection from pg_create_subscription; then you can
grant each one independently as you see fit.

For instance, there may be an administrator that controls what
postgres instances are available, and what connections may be
reasonable between those instances. That admin will need the
pg_create_connection role, and can proactively create all the servers
(using FOR CONNECTION ONLY) and user mappings that may be useful, and
manage and update those as necessary without breaking
subscriptions. Another role may be used to manage the subscriptions
themselves, and they would need to be a member of
pg_create_subscription but do not need the privileges to create raw
connection strings.

Note: the ability to revoke pg_create_connection from
pg_create_subscription avoids some risks in some environments; but
creating a subcription should still be considered a highly privileged
operation whether using SERVER or CONNECTION.

Remaining work:

The code for options handling needs some work. It's similar to
postgres_fdw in behavior, but I didn't spend as much time on it because
I suspect we will want to refactor the various ways connection strings
are handled (in CREATE SUBSCRIPTION ... CONNECTION, postgres_fdw, and
dblink) to make them more consistent.

Also, there are some nuances in handling connection options that I
don't fully understand. postgres_fdw makes a lot of effort: it
overrides client_encoding, it does a
post-connection security check, and allows GSS instead of a password
option for non-superusers. But CREATE SUBSCRIPTION ... CONNECTION makes
little effort, only checking whether the password is specified or not.
I'd like to understand why they are different and what we can unify.

Also, right now dblink has it's own dblink_fdw, and perhaps a server
FOR CONNECTION ONLY should become the preferred method instead.


-- 
Jeff Davis
PostgreSQL Contributor Team - AWS


From 422114a0bc1d928d257505bf31e99397cb8a6a8c Mon Sep 17 00:00:00 2001
From: Jeff Davis 
Date: Wed, 23 Aug 2023 10:31:16 -0700
Subject: [PATCH v1] CREATE SUBSCRIPTION ... SERVER.

---
 contrib/dblink/dblink.c   |  17 +-
 contrib/dblink/expected/dblink.out