Re: archive_command / pg_stat_archiver & documentation

[Benoit Lobréau]

Le lun. 1 mars 2021 à 08:36, Michael Paquier  a écrit :

> On Fri, Feb 26, 2021 at 10:03:05AM +0100, Benoit Lobréau wrote:
> > Done here : https://commitfest.postgresql.org/32/3012/
>
> Documenting that properly for the archive command, as already done for
> restore_command, sounds good to me.  I am not sure that there is much
> point in doing a cross-reference to the archiving section for one
> specific field of pg_stat_archiver.
>

I wanted to add a warning that using pg_stat_archiver to monitor the good
health of the
archiver comes with a caveat in the view documentation itself. But couldn't
find a concise
way to do it. So I added a link.

If you think it's unnecessary, that's ok.


> For the second paragraph, I would recommend to move that to a
> different  to outline this special case, leading to the
> attached.
>

Good.

Re: A reloption for partitioned tables - parallel_workers

[Amit Langote]

On Tue, Feb 23, 2021 at 7:24 PM houzj.f...@fujitsu.com
 wrote:
> > > It seems the patch does not include the code that get the
> > > parallel_workers from new struct " PartitionedTableRdOptions ", Did I miss
> > something ?
> >
> > Aren't the following hunks in the v2 patch what you meant?
> >
> > diff --git a/src/backend/access/common/reloptions.c
> > b/src/backend/access/common/reloptions.c
> > index c687d3ee9e..f8443d2361 100644
> > --- a/src/backend/access/common/reloptions.c
> > +++ b/src/backend/access/common/reloptions.c
> > @@ -377,7 +377,7 @@ static relopt_int intRelOpts[] =
> >   {
> >   "parallel_workers",
> >   "Number of parallel processes that can be used per executor node for this
> > relation.",
> > - RELOPT_KIND_HEAP,
> > + RELOPT_KIND_HEAP | RELOPT_KIND_PARTITIONED,
> >   ShareUpdateExclusiveLock
> >   },
> >   -1, 0, 1024
> > @@ -1962,12 +1962,18 @@ bytea *
> >  partitioned_table_reloptions(Datum reloptions, bool validate)  {
> >   /*
> > - * There are no options for partitioned tables yet, but this is able to do
> > - * some validation.
> > + * Currently the only setting known to be useful for partitioned tables
> > + * is parallel_workers.
> >   */
> > + static const relopt_parse_elt tab[] = { {"parallel_workers",
> > + RELOPT_TYPE_INT, offsetof(PartitionedTableRdOptions,
> > + parallel_workers)}, };
> > +
> >   return (bytea *) build_reloptions(reloptions, validate,
> > RELOPT_KIND_PARTITIONED,
> > -   0, NULL, 0);
> > +   sizeof(PartitionedTableRdOptions),
> > +   tab, lengthof(tab));
> >  }
> >
> >  /*
> >
> > diff --git a/src/include/utils/rel.h b/src/include/utils/rel.h index
> > 10b63982c0..fe114e0856 100644
> > --- a/src/include/utils/rel.h
> > +++ b/src/include/utils/rel.h
> > @@ -308,6 +308,16 @@ typedef struct StdRdOptions
> >   bool vacuum_truncate; /* enables vacuum to truncate a relation */  }
> > StdRdOptions;
> >
> > +/*
> > + * PartitionedTableRdOptions
> > + * Contents of rd_options for partitioned tables  */ typedef struct
> > +PartitionedTableRdOptions {
> > + int32 vl_len_; /* varlena header (do not touch directly!) */  int
> > +parallel_workers; /* max number of parallel workers */ }
> > +PartitionedTableRdOptions;
> > +
> >  #define HEAP_MIN_FILLFACTOR 10
> >  #define HEAP_DEFAULT_FILLFACTOR 100
> Hi,
>
> I am not sure.
> IMO, for normal table, we use the following macro to get the parallel_workers:
> --
> /*
>  * RelationGetParallelWorkers
>  *  Returns the relation's parallel_workers reloption setting.
>  *  Note multiple eval of argument!
>  */
> #define RelationGetParallelWorkers(relation, defaultpw) \
> ((relation)->rd_options ? \
>  ((StdRdOptions *) (relation)->rd_options)->parallel_workers : 
> (defaultpw))
> --
>
> Since we add new struct " PartitionedTableRdOptions ", It seems we need to 
> get parallel_workers in different way.
> Do we need similar macro to get partitioned table's parallel_workers ?

Oh, you're right.  The parallel_workers setting of a relation is only
accessed through this macro, even for partitioned tables, and I can
see that it is actually wrong to access a partitioned table's
parallel_workers through this macro as-is.   Although I hadn't tested
that, so thanks for pointing that out.

> Like:
> #define PartitionedTableGetParallelWorkers(relation, defaultpw) \
> xxx
> (PartitionedTableRdOptions *) (relation)->rd_options)->parallel_workers : 
> (defaultpw))

I'm thinking it would be better to just modify the existing macro to
check relkind to decide which struct pointer type to cast the value in
rd_options to.

I will post an updated patch later.

-- 
Amit Langote
EDB: http://www.enterprisedb.com

Re: Asynchronous Append on postgres_fdw nodes.

[Etsuro Fujita]

On Sat, Feb 20, 2021 at 3:35 PM Etsuro Fujita  wrote:
> So I’ll update the patch using your patch without the
> postgresIterateForeignScan() change.

Here is an updated version of the patch.  Based on your idea of
completing an in-progress command (if any) before sending a new
command to the remote, I created a function for that
process_pending_request(), and added it where needed in
contrib/postgres_fdw.  I also adjusted the patch, and fixed some bugs
in the postgres_fdw part of the patch.

Best regards,
Etsuro Fujita


async-wip-2021-03-01.patch
Description: Binary data

Re: repeated decoding of prepared transactions

[Amit Kapila]

On Mon, Mar 1, 2021 at 7:23 AM Ajin Cherian  wrote:
>
Few minor comments on 0002 patch
=
1.
  ctx->streaming &= enable_streaming;
- ctx->twophase &= enable_twophase;
+
 }

Spurious line addition.

2.
-  proallargtypes =>
'{name,name,text,oid,bool,bool,int4,xid,xid,pg_lsn,pg_lsn,text,int8}',
-  proargmodes => '{o,o,o,o,o,o,o,o,o,o,o,o,o}',
-  proargnames =>
'{slot_name,plugin,slot_type,datoid,temporary,active,active_pid,xmin,catalog_xmin,restart_lsn,confirmed_flush_lsn,wal_status,safe_wal_size}',
+  proallargtypes =>
'{name,name,text,oid,bool,bool,int4,xid,xid,pg_lsn,pg_lsn,text,int8,bool}',
+  proargmodes => '{o,o,o,o,o,o,o,o,o,o,o,o,o,o}',
+  proargnames =>
'{slot_name,plugin,slot_type,datoid,temporary,active,active_pid,xmin,catalog_xmin,restart_lsn,confirmed_flush_lsn,wal_status,safe_wal_size,twophase}',
   prosrc => 'pg_get_replication_slots' },
 { oid => '3786', descr => 'set up a logical replication slot',
   proname => 'pg_create_logical_replication_slot', provolatile => 'v',
-  proparallel => 'u', prorettype => 'record', proargtypes => 'name name bool',
-  proallargtypes => '{name,name,bool,name,pg_lsn}',
-  proargmodes => '{i,i,i,o,o}',
-  proargnames => '{slot_name,plugin,temporary,slot_name,lsn}',
+  proparallel => 'u', prorettype => 'record', proargtypes => 'name
name bool bool',
+  proallargtypes => '{name,name,bool,bool,name,pg_lsn}',
+  proargmodes => '{i,i,i,i,o,o}',
+  proargnames => '{slot_name,plugin,temporary,twophase,slot_name,lsn}',

I think it is better to use two_phase here and at other places as well
to be consistent with similar parameters.

3.
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -894,7 +894,8 @@ CREATE VIEW pg_replication_slots AS
 L.restart_lsn,
 L.confirmed_flush_lsn,
 L.wal_status,
-L.safe_wal_size
+L.safe_wal_size,
+ L.twophase
 FROM pg_get_replication_slots() AS L

Indentation issue. Here, you need you spaces instead of tabs.

4.
@@ -533,6 +533,12 @@ CreateDecodingContext(XLogRecPtr start_lsn,

  ctx->reorder->output_rewrites = ctx->options.receive_rewrites;

+ /*
+ * If twophase is set on the slot at create time, then
+ * make sure the field in the context is also updated.
+ */
+ ctx->twophase &= MyReplicationSlot->data.twophase;
+

Why didn't you made similar change in CreateInitDecodingContext when I
already suggested the same in my previous email? If we don't make that
change then during slot initialization two_phase will always be true
even though user passed in as false. It looks inconsistent and even
though there is no direct problem due to that but it could be cause of
possible problem in future.

-- 
With Regards,
Amit Kapila.

Re: archive_command / pg_stat_archiver & documentation

[Julien Rouhaud]

On Mon, Mar 1, 2021 at 4:33 PM Benoit Lobréau  wrote:
>
> Le lun. 1 mars 2021 à 08:36, Michael Paquier  a écrit :
>>
>> On Fri, Feb 26, 2021 at 10:03:05AM +0100, Benoit Lobréau wrote:
>> > Done here : https://commitfest.postgresql.org/32/3012/
>>
>> Documenting that properly for the archive command, as already done for
>> restore_command, sounds good to me.  I am not sure that there is much
>> point in doing a cross-reference to the archiving section for one
>> specific field of pg_stat_archiver.
>
>
> I wanted to add a warning that using pg_stat_archiver to monitor the good 
> health of the
> archiver comes with a caveat in the view documentation itself. But couldn't 
> find a concise
> way to do it. So I added a link.
>
> If you think it's unnecessary, that's ok.

Maybe this can be better addressed than with a link in the
documentation.  The final outcome is that it can be difficult to
monitor the archiver state in such case.  That's orthogonal to this
patch but maybe we can add a new "archiver_start" timestamptz column
in pg_stat_archiver, so monitoring tools can detect a problem if it's
too far away from pg_postmaster_start_time() for instance?

Re: archive_command / pg_stat_archiver & documentation

[Benoit Lobréau]

I like the idea !

If it's not too complicated, I'd like to take a stab at it.

Le lun. 1 mars 2021 à 10:16, Julien Rouhaud  a écrit :

> On Mon, Mar 1, 2021 at 4:33 PM Benoit Lobréau 
> wrote:
> >
> > Le lun. 1 mars 2021 à 08:36, Michael Paquier  a
> écrit :
> >>
> >> On Fri, Feb 26, 2021 at 10:03:05AM +0100, Benoit Lobréau wrote:
> >> > Done here : https://commitfest.postgresql.org/32/3012/
> >>
> >> Documenting that properly for the archive command, as already done for
> >> restore_command, sounds good to me.  I am not sure that there is much
> >> point in doing a cross-reference to the archiving section for one
> >> specific field of pg_stat_archiver.
> >
> >
> > I wanted to add a warning that using pg_stat_archiver to monitor the
> good health of the
> > archiver comes with a caveat in the view documentation itself. But
> couldn't find a concise
> > way to do it. So I added a link.
> >
> > If you think it's unnecessary, that's ok.
>
> Maybe this can be better addressed than with a link in the
> documentation.  The final outcome is that it can be difficult to
> monitor the archiver state in such case.  That's orthogonal to this
> patch but maybe we can add a new "archiver_start" timestamptz column
> in pg_stat_archiver, so monitoring tools can detect a problem if it's
> too far away from pg_postmaster_start_time() for instance?
>

Re: archive_command / pg_stat_archiver & documentation

[Julien Rouhaud]

On Mon, Mar 1, 2021 at 5:24 PM Benoit Lobréau  wrote:
>
> I like the idea !
>
> If it's not too complicated, I'd like to take a stab at it.

Great!  And it shouldn't be too complicated.  Note that unfortunately
this will likely not be included in pg14 as the last commitfest should
begin today.

Re: Fix DROP TABLESPACE on Windows with ProcSignalBarrier?

[Daniel Gustafsson]

> On 1 Mar 2021, at 05:46, Thomas Munro  wrote:

>> The 0003 patch to achieve $SUBJECT needs
>> more discussion.
> 
> Rebased.
> 
> The more I think about it, the more I think that this approach is good
> enough for an initial solution to the problem.  It only affects
> Windows, dropping tablespaces is hopefully rare, and it's currently
> broken on that OS.  That said, it's complex enough, and I guess more
> to the point, enough of a compromise, that I'm hoping to get some
> explicit consensus about that.
> 
> A better solution would probably have to be based on the sinval queue,
> somehow.  Perhaps with a new theory or rule making it safe to process
> at every CFI(), or by deciding that we're prepared have the operation
> wait arbitrarily long until backends reach a point where it is known
> to be safe (probably near ProcessClientReadInterrupt()'s call to
> ProcessCatchupInterrupt()), or by inventing a new kind of lightweight
> "sinval peek" that is safe to run at every CFI() point, being based on
> reading (but not consuming!) the sinval queue and performing just
> vfd-close of referenced smgr relations in this case.  The more I think
> about all that complexity for a super rare event on only one OS, the
> more I want to just do it the stupid way and close all the fds.
> Robert opined similarly in an off-list chat about this problem.

I don't know Windows at all so I can't really comment on that portion, but from
my understanding of procsignalbarriers I think this seems right.  No tests
break when forcing the codepath to run on Linux and macOS.

Should this be performed in tblspc_redo as well for the similar case?

+#if defined(WIN32) || defined(USE_ASSERT_CHECKING)

Is the USE_ASSERT_CHECKING clause to exercise the code a more frequent than
just on Windows?  That could warrant a quick word in the comment if so IMO to
avoid confusion.

-ProcessBarrierPlaceholder(void)
+ProcessBarrierSmgrRelease(void)
 {
-   /*
-* XXX. This is just a placeholder until the first real user of this
-* machinery gets committed. Rename PROCSIGNAL_BARRIER_PLACEHOLDER to
-* PROCSIGNAL_BARRIER_SOMETHING_ELSE where SOMETHING_ELSE is something
-* appropriately descriptive. Get rid of this function and instead have
-* ProcessBarrierSomethingElse. Most likely, that function should live 
in
-* the file pertaining to that subsystem, rather than here.
-*
-* The return value should be 'true' if the barrier was successfully
-* absorbed and 'false' if not. Note that returning 'false' can lead to
-* very frequent retries, so try hard to make that an uncommon case.
-*/
+   smgrrelease();

Should this instead be in smgr.c to avoid setting a precedent for procsignal.c
to be littered with absorption functions?

--
Daniel Gustafsson   https://vmware.com/

Re: [HACKERS] logical decoding of two-phase transactions

[Amit Kapila]

On Fri, Feb 26, 2021 at 4:28 PM Amit Kapila  wrote:
>
> On Fri, Feb 26, 2021 at 9:56 AM Amit Kapila  wrote:
> >
> > On Thu, Feb 25, 2021 at 12:32 PM Peter Smith  wrote:
> > >
> >
> > 5. You need to write/sync the spool file at prepare time because after
> > restart between prepare and commit prepared the changes can be lost
> > and won't be resent by the publisher assuming there are commits of
> > other transactions between prepare and commit prepared. For the same
> > reason, I am not sure if we can just rely on the in-memory hash table
> > for it (prepare_spoolfile_exists). Sure, if it exists and there is no
> > restart then it would be cheap to check in the hash table but I don't
> > think it is guaranteed.
> >
>
> As we can't rely on the hash table, I think we can get rid of it and
> always check if the corresponding file exists.
>

Few more related points:

1. Currently, the patch will always clean up the files if there is an
error because SharedFileSetInit registers the cleanup function.
However, we want the files to be removed only if any error happens
before flushing prepare. Once prepare is flushed, we expect the file
will be cleaned up by commit prepared. So, we need to probably call
SharedFileSetUnregister after prepare has been flushed to file.

2. The other point is that I think we need to drop these files (if
any) on Drop Subscription. Investigate if any variant of Alter needs
similar handling.

-- 
With Regards,
Amit Kapila.

[PATCH] Add --create-only option to pg_dump/pg_dumpall

[Michael Banck]

Hi,

There is (to my knowledge) no direct way to get the `CREATE DATABASE`
and assorted `GRANT foo ON DATABASE` etc. commands out of a pg_dump
without having to edit the TOC or filter the SQL output with e.g. grep.

It is not part of pg_dumpall -g, and if one uses pg_dump / pg_dumpall -s
-C, one gets all definitions for all database objects.

So I propose a small additional option --create-only, which only dumps
the create-related commands, e.g.:

postgres=# CREATE DATABASE test;
CREATE DATABASE
postgres=# CREATE USER test;
CREATE ROLE
postgres=# GRANT CONNECT ON DATABASE test TO test;
GRANT
postgres=# \q
postgres@kohn:~$ pg_dump --create-only -p 65432 -d test -h /tmp | egrep -v 
'^($|--|SET)' 
SELECT pg_catalog.set_config('search_path', '', false);
CREATE DATABASE test WITH TEMPLATE = template0 ENCODING = 'UTF8' LOCALE = 
'de_DE.UTF-8';
ALTER DATABASE test OWNER TO postgres;
\connect test
SELECT pg_catalog.set_config('search_path', '', false);
GRANT CONNECT ON DATABASE test TO test;
postgres@kohn:~$


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer

Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz
From 7b924aed0db30f5e138ae0050d45159b2d675f6e Mon Sep 17 00:00:00 2001
From: Michael Banck 
Date: Thu, 31 Dec 2020 16:12:31 +0100
Subject: [PATCH] Add --create-only option to pg_dump/pg_dumpall.

This makes pg_dump only output the database creation and assorted commands
(notably also ALTER DATABASE [...] SET [...]). If only the database-specific
settings are desired, this makes dumping large databases or schemas much
easier.
---
 src/bin/pg_dump/pg_backup.h  |  1 +
 src/bin/pg_dump/pg_dump.c| 14 +++---
 src/bin/pg_dump/pg_dumpall.c |  8 +++-
 3 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/src/bin/pg_dump/pg_backup.h b/src/bin/pg_dump/pg_backup.h
index eea9f30a79..6560a611fc 100644
--- a/src/bin/pg_dump/pg_backup.h
+++ b/src/bin/pg_dump/pg_backup.h
@@ -172,6 +172,7 @@ typedef struct _dumpOptions
 
 	int			outputClean;
 	int			outputCreateDB;
+	int			outputCreateDBOnly;
 	bool		outputBlobs;
 	bool		dontOutputBlobs;
 	int			outputNoOwner;
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index eb988d7eb4..8b60f91ffe 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -363,6 +363,7 @@ main(int argc, char **argv)
 		 */
 		{"attribute-inserts", no_argument, &dopt.column_inserts, 1},
 		{"binary-upgrade", no_argument, &dopt.binary_upgrade, 1},
+		{"create-only", no_argument, &dopt.outputCreateDBOnly, 1},
 		{"column-inserts", no_argument, &dopt.column_inserts, 1},
 		{"disable-dollar-quoting", no_argument, &dopt.disable_dollar_quoting, 1},
 		{"disable-triggers", no_argument, &dopt.disable_triggers, 1},
@@ -703,6 +704,9 @@ main(int argc, char **argv)
 	if (!plainText)
 		dopt.outputCreateDB = 1;
 
+	if (dopt.outputCreateDBOnly)
+		dopt.outputCreateDB = 1;
+
 	/*
 	 * On Windows we can only have at most MAXIMUM_WAIT_OBJECTS (= 64 usually)
 	 * parallel jobs because that's the maximum limit for the
@@ -917,9 +921,12 @@ main(int argc, char **argv)
 	if (dopt.outputCreateDB)
 		dumpDatabase(fout);
 
-	/* Now the rearrangeable objects. */
-	for (i = 0; i < numObjs; i++)
-		dumpDumpableObject(fout, dobjs[i]);
+	if (!dopt.outputCreateDBOnly)
+	{
+		/* Now the rearrangeable objects. */
+		for (i = 0; i < numObjs; i++)
+			dumpDumpableObject(fout, dobjs[i]);
+	}
 
 	/*
 	 * Set up options info to ensure we dump what we want.
@@ -1019,6 +1026,7 @@ help(const char *progname)
 	printf(_("  -B, --no-blobs   exclude large objects in dump\n"));
 	printf(_("  -c, --clean  clean (drop) database objects before recreating\n"));
 	printf(_("  -C, --create include commands to create database in dump\n"));
+	printf(_("  --create-onlyonly dump commands to create database\n"));
 	printf(_("  -E, --encoding=ENCODING  dump the data in encoding ENCODING\n"));
 	printf(_("  -n, --schema=PATTERN dump the specified schema(s) only\n"));
 	printf(_("  -N, --exclude-schema=PATTERN do NOT dump the specified schema(s)\n"));
diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c
index 007a3d0f9a..7eaa4d1901 100644
--- a/src/bin/pg_dump/pg_dumpall.c
+++ b/src/bin/pg_dump/pg_dumpall.c
@@ -67,6 +67,7 @@ static bool dosync = true;
 
 static int	binary_upgrade = 0;
 static int	column_inserts = 0;
+static int	create_only = 0;
 static int	disable_dollar_quoting = 0;
 static int	disable_triggers = 0;
 static int	if_exists = 0;
@@ -126,6 +127,7 @@ main(int argc, char *argv[])
 		{"attribute-inserts", no_argument, &column_inserts, 1},
 		{"binary-upgrade", no_argument, &binary_upgrad

Re: PostmasterIsAlive() in recovery (non-USE_POST_MASTER_DEATH_SIGNAL builds)

[Thomas Munro]

On Mon, Nov 16, 2020 at 8:56 PM Michael Paquier  wrote:
> On Thu, Sep 24, 2020 at 05:55:17PM +1200, Thomas Munro wrote:
> > Right, RestoreArchivedFile() uses system(), so I guess it can hang
> > around for a long time after unexpected postmaster exit on every OS if
> > the command waits.  To respond to various kinds of important
> > interrupts, I suppose that'd ideally use something like
> > OpenPipeStream() and a typical WaitLatch() loop with CFI().  I'm not
> > sure what our policy is or should be for exiting while we have running
> > subprocesses.  I guess that is a separate issue.
>
> -   if (IsUnderPostmaster && !PostmasterIsAlive())
> +   if (IsUnderPostmaster &&
> +#ifndef USE_POSTMASTER_DEATH_SIGNAL
> +   count++ % 1024 == 0 &&
> +#endif
> +   !PostmasterIsAlive())
> That's pretty hack-ish, still efficient.  Could we consider a
> different approach like something relying on
> PostmasterIsAliveInternal() with repetitive call handling?  This may
> not be the only place where we care about that, particularly for
> non-core code.

As far as I know there aren't any other places that do polling of
PostmasterIsAlive() in a loop like this.  All others have been removed
from core code: either they already had a WaitLatch() or similar so it
we just had to add WL_EXIT_ON_PM_DEATH, or they do pure CPU-bound and
don't actively try to detect postmaster death.  That's why it seems
utterly insane that here we try to detect it X million times per
second.

> No objections with the two changes from pg_usleep() to WaitLatch() so
> they could be applied separately first.

I thought about committing that first part, and got as far as
splitting the patch into two (see attached), but then I re-read
Fujii-san's message about the speed of promotion and realised that we
really should have something like a condition variable for walRcvState
changes.  I'll look into that.
From f90c4b58989168a1fe5a2dd48181444a18351bfb Mon Sep 17 00:00:00 2001
From: Thomas Munro 
Date: Mon, 1 Mar 2021 23:08:55 +1300
Subject: [PATCH v5 1/2] Replace some sleep/poll loops with WaitLatch().

Although the replaced sleeps are fairly short, a proposed patch to
reduce the frequency of reads of the postmaster pipe could cause the
loops to take a very long time to react to postmaster exit.  Instead,
use the standard waiting infrastructure (with no latch), which can exit
automatically based on readiness.

Reviewed-by: Heikki Linnakangas 
Reviewed-by: Fujii Masao 
Reviewed-by: Michael Paquier 
Discussion: https://postgr.es/m/CA%2BhUKGK1607VmtrDUHQXrsooU%3Dap4g4R2yaoByWOOA3m8xevUQ%40mail.gmail.com
---
 doc/src/sgml/monitoring.sgml   | 4 
 src/backend/access/transam/xlog.c  | 7 +++
 src/backend/postmaster/pgstat.c| 3 +++
 src/backend/replication/walreceiverfuncs.c | 6 --
 src/include/pgstat.h   | 1 +
 5 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 3513e127b7..cf00210cb3 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -1757,6 +1757,10 @@ postgres   27093  0.0  0.0  30096  2752 ?Ss   11:34   0:00 postgres: ser
   Waiting for confirmation from a remote server during synchronous
replication.
  
+ 
+  WalrcvExit
+  Waiting for the walreceiver to exit.
+ 
  
   XactGroupUpdate
   Waiting for the group leader to update transaction status at
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 377afb8732..e63e19eed3 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -6017,7 +6017,7 @@ recoveryStopsAfter(XLogReaderState *record)
  * the paused state starts at the end of recovery because of
  * recovery_target_action=pause, and false otherwise.
  *
- * XXX Could also be done with shared latch, avoiding the pg_usleep loop.
+ * XXX Could also be done with shared latch, avoiding the WL_TIMEOUT loop.
  * Probably not worth the trouble though.  This state shouldn't be one that
  * anyone cares about server power consumption in.
  */
@@ -6046,9 +6046,8 @@ recoveryPausesHere(bool endOfRecovery)
 		HandleStartupProcInterrupts();
 		if (CheckForStandbyTrigger())
 			return;
-		pgstat_report_wait_start(WAIT_EVENT_RECOVERY_PAUSE);
-		pg_usleep(100L);	/* 1000 ms */
-		pgstat_report_wait_end();
+		(void) WaitLatch(NULL, WL_TIMEOUT | WL_EXIT_ON_PM_DEATH, 1000,
+		 WAIT_EVENT_RECOVERY_PAUSE);
 	}
 }
 
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index f75b52719d..2dbfb81e40 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -4122,6 +4122,9 @@ pgstat_get_wait_ipc(WaitEventIPC w)
 		case WAIT_EVENT_SYNC_REP:
 			event_name = "SyncRep";
 			break;
+		case WAIT_EVENT_WALRCV_EXIT:
+			event_name = "WalrcvExit";
+			break;
 		case WAIT_EVENT_XACT_GROUP_UPDATE:
 			event_name = "XactGroupUpdate";
 			break;

Re: Regex back-reference semantics and performance

[Joel Jacobson]

On Mon, Mar 1, 2021, at 01:53, Tom Lane wrote:
>0001-fix-backref-semantics.patch
>0002-backref-performance-hack.patch

I've successfully tested both patches.

On HEAD the trouble-query took forever, I cancelled it after 23 minutes.

HEAD (f5a5773a9dc4185414fe538525e20d8512c2ba35):
SELECT regexp_matches(subject, pattern, 'g') FROM trouble;
^CCancel request sent
ERROR:  canceling statement due to user request
Time: 1387398.764 ms (23:07.399)

HEAD + 0001 + 0002:
SELECT regexp_matches(subject, pattern, 'g') FROM trouble;
Time: 24.943 ms
Time: 22.217 ms
Time: 20.250 ms

Very nice!

I also verified the patches gave the same result for the performance_test:

SELECT
  is_match <> (subject ~ pattern) AS is_match_diff,
  captured IS DISTINCT FROM regexp_match(subject, pattern, flags) AS 
captured_diff,
  COUNT(*)
FROM performance_test
GROUP BY 1,2
ORDER BY 1,2
;

is_match_diff | captured_diff |  count
---+---+-
f | f | 3360068
(1 row)

No notable timing differences:

HEAD (f5a5773a9dc4185414fe538525e20d8512c2ba35)
Time: 97016.668 ms (01:37.017)
Time: 96945.567 ms (01:36.946)
Time: 95261.263 ms (01:35.261)

HEAD + 0001:
Time: 97165.302 ms (01:37.165)
Time: 96327.836 ms (01:36.328)
Time: 96295.643 ms (01:36.296)

HEAD + 0001 + 0002:
Time: 96447.527 ms (01:36.448)
Time: 94262.288 ms (01:34.262)
Time: 95331.483 ms (01:35.331)

/Joel

Re: Fix DROP TABLESPACE on Windows with ProcSignalBarrier?

[Thomas Munro]

On Mon, Mar 1, 2021 at 11:07 PM Daniel Gustafsson  wrote:
> I don't know Windows at all so I can't really comment on that portion, but 
> from
> my understanding of procsignalbarriers I think this seems right.  No tests
> break when forcing the codepath to run on Linux and macOS.

Hey Daniel,

Thanks for looking!

> Should this be performed in tblspc_redo as well for the similar case?

Ah.  Yes.  Added (not tested yet).

> +#if defined(WIN32) || defined(USE_ASSERT_CHECKING)
>
> Is the USE_ASSERT_CHECKING clause to exercise the code a more frequent than
> just on Windows?  That could warrant a quick word in the comment if so IMO to
> avoid confusion.

Note added.

> -ProcessBarrierPlaceholder(void)
> +ProcessBarrierSmgrRelease(void)
>  {
> -   /*
> -* XXX. This is just a placeholder until the first real user of this
> -* machinery gets committed. Rename PROCSIGNAL_BARRIER_PLACEHOLDER to
> -* PROCSIGNAL_BARRIER_SOMETHING_ELSE where SOMETHING_ELSE is something
> -* appropriately descriptive. Get rid of this function and instead 
> have
> -* ProcessBarrierSomethingElse. Most likely, that function should 
> live in
> -* the file pertaining to that subsystem, rather than here.
> -*
> -* The return value should be 'true' if the barrier was successfully
> -* absorbed and 'false' if not. Note that returning 'false' can lead 
> to
> -* very frequent retries, so try hard to make that an uncommon case.
> -*/
> +   smgrrelease();
>
> Should this instead be in smgr.c to avoid setting a precedent for procsignal.c
> to be littered with absorption functions?

Done.
From 5d5103716cbd1ac2ce5f62674ea7eb263ffcba71 Mon Sep 17 00:00:00 2001
From: Thomas Munro 
Date: Mon, 1 Mar 2021 17:14:11 +1300
Subject: [PATCH v6] Use a global barrier to fix DROP TABLESPACE on Windows.

Previously, it was possible for DROP TABLESPACE to fail because a table
had been dropped, but other backends still had open file handles.
Windows won't allow a directory containing unlinked-but-still-open files
to be unlinked.  Tackle this problem by forcing all backends to close
all fds.  No change for Unix systems, which didn't suffer from the
problem, but simulate the need to do it in assertion builds just to
exercise the code.

Reviewed-by: Andres Freund 
Reviewed-by: Daniel Gustafsson 
Discussion: https://postgr.es/m/ca+hukgldemy2gbm80kz20gte6hnvwoere8kwcjk6-u56ost...@mail.gmail.com
---
 src/backend/commands/tablespace.c| 32 ++--
 src/backend/storage/ipc/procsignal.c | 24 +++--
 src/backend/storage/smgr/md.c|  6 ++
 src/backend/storage/smgr/smgr.c  | 18 
 src/include/storage/md.h |  1 +
 src/include/storage/procsignal.h |  7 +-
 src/include/storage/smgr.h   |  1 +
 7 files changed, 55 insertions(+), 34 deletions(-)

diff --git a/src/backend/commands/tablespace.c b/src/backend/commands/tablespace.c
index 69ea155d50..38894c5123 100644
--- a/src/backend/commands/tablespace.c
+++ b/src/backend/commands/tablespace.c
@@ -520,15 +520,25 @@ DropTableSpace(DropTableSpaceStmt *stmt)
 		 * but we can't tell them apart from important data files that we
 		 * mustn't delete.  So instead, we force a checkpoint which will clean
 		 * out any lingering files, and try again.
-		 *
-		 * XXX On Windows, an unlinked file persists in the directory listing
-		 * until no process retains an open handle for the file.  The DDL
-		 * commands that schedule files for unlink send invalidation messages
-		 * directing other PostgreSQL processes to close the files.  DROP
-		 * TABLESPACE should not give up on the tablespace becoming empty
-		 * until all relevant invalidation processing is complete.
 		 */
 		RequestCheckpoint(CHECKPOINT_IMMEDIATE | CHECKPOINT_FORCE | CHECKPOINT_WAIT);
+		/*
+		 * On Windows, an unlinked file persists in the directory listing until
+		 * no process retains an open handle for the file.  The DDL
+		 * commands that schedule files for unlink send invalidation messages
+		 * directing other PostgreSQL processes to close the files, but nothing
+		 * guarantees they'll be processed in time.  So, we'll also use a
+		 * global barrier to ask all backends to close all files, and wait
+		 * until they're finished.
+		 *
+		 * For test coverage, also do this on Unix systems in assert builds.
+		 */
+#if defined(WIN32) || defined(USE_ASSERT_CHECKING)
+		LWLockRelease(TablespaceCreateLock);
+		WaitForProcSignalBarrier(EmitProcSignalBarrier(PROCSIGNAL_BARRIER_SMGRRELEASE));
+		LWLockAcquire(TablespaceCreateLock, LW_EXCLUSIVE);
+#endif
+		/* And now try again. */
 		if (!destroy_tablespace_directories(tablespaceoid, false))
 		{
 			/* Still not empty, the files must be important then */
@@ -1550,6 +1560,14 @@ tblspc_redo(XLogReaderState *record)
 		 */
 		if (!destroy_tablespace_directories(xlrec->ts_id, true))
 		{
+#if defined(WIN32) || defined(USE_ASSERT_CHECKING)
+

Re: [PATCH] Note effect of max_replication_slots on subscriber side in documentation.

[Amit Kapila]

On Sat, Feb 27, 2021 at 2:35 PM Amit Kapila  wrote:
>
> On Sat, Feb 27, 2021 at 2:47 AM Paul Martinez  wrote:
> >
> > On Fri, Feb 26, 2021 at 5:22 AM Amit Kapila  wrote:
> > >
> > > https://www.postgresql.org/docs/devel/logical-replication-config.html
> > >
> >
> > Ah, yep. I added a clause to the end of the sentence to clarify why we're
> > using max_replication_slots here:
> >
> > - The subscriber also requires the max_replication_slots to be set.
> >
> > + The subscriber also requires that max_replication_slots be set to
> > + configure how many replication origins can be tracked.
> >
>
> LGTM.
>

The rebased version attached. As mentioned earlier, I think we can
backpatch this patch as this clarifies the already existing behavior.
Do let me know if you or others think otherwise?

-- 
With Regards,
Amit Kapila.


max_replication_slots_subscriber_doc_v02.patch
Description: Binary data

Re: [HACKERS] Custom compression methods

[Dilip Kumar]

On Mon, Mar 1, 2021 at 11:06 AM Justin Pryzby  wrote:

> Thanks.  It seems like that explains it.
> I think if that's a problem with recent versions, then you'll have to
> conditionally disable slicing.
> https://packages.debian.org/liblz4-dev
>
> Slicing isn't generally usable if it sometimes makes people's data 
> inaccessible
> and gives errors about corruption.
>
> I guess you could make it a compile time test on these constants (I don't know
> the necessary version, though)
>
> #define LZ4_VERSION_MAJOR1/* for breaking interface changes  */
> #define LZ4_VERSION_MINOR7/* for new (non-breaking) interface 
> capabilities */
> #define LZ4_VERSION_RELEASE  1/* for tweaks, bug-fixes, or development */
> #define LZ4_VERSION_NUMBER (LZ4_VERSION_MAJOR *100*100 + LZ4_VERSION_MINOR 
> *100 + LZ4_VERSION_RELEASE)
>
> If the version is too low, either make it #error, or disable slicing.
> The OS usual library version infrastructure will make sure the runtime version
> is at least the MAJOR+MINOR of the compile time version.

I think we can check the version and if it too low i.e.  below1.8.3 (
in this release the slicing issue was fixed) then we can call the full
decompression routine from the slicing function.


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

Re: Improving connection scalability: GetSnapshotData()

[AJG]

Hi,

Greatly appreciate if you could please reply to the following questions as
time allows.

I have seen previous discussion/patches on a built-in connection pooler. How
does this scalability improvement, particularly idle connection improvements
etc, affect that built-in pooler need, if any?


Same general question about an external connection pooler in general in
Production? Still required to route to different servers but no longer
needed for the pooling part. as an example.


Many Thanks!





--
Sent from: https://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html

Re: Improving connection scalability: GetSnapshotData()

[luis . roberto]

- Mensagem original -
> De: "AJG" 
> Para: "Pg Hackers" 
> Enviadas: Sábado, 27 de fevereiro de 2021 14:40:58
> Assunto: Re: Improving connection scalability: GetSnapshotData()

> Hi,

> Greatly appreciate if you could please reply to the following questions as
> time allows.

> I have seen previous discussion/patches on a built-in connection pooler. How
> does this scalability improvement, particularly idle connection improvements
> etc, affect that built-in pooler need, if any?

> Same general question about an external connection pooler in general in
> Production? Still required to route to different servers but no longer
> needed for the pooling part. as an example.

> Many Thanks!

> --
> Sent from: https://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html


As I understand it, the improvements made to GetSnapShotData() mean having 
higher connection count does not incur as much a penalty to performance as 
before. 
I am not sure it solves the connection stablishment side of things, but I may 
be wrong.

Luis R. Weck 

Re: proposal: psql –help reflecting service or URI usage

[Paul Förster]

Hi Mark,

I revisited my first admittedly naive and faulty attempt.

> On 28. Feb, 2021, at 19:10, Paul Förster  wrote:
> 
>> but of course being careful to still fit in the line length limit.
> I agree to all, thanks. What is the line length limit?

still, what is the line length limit? Where do I find it?

I'd be very happy if you could take a look at this version. Thanks in advance 
and thanks very much for the input.

Cheers,
Paul



psql-help.c.patch
Description: Binary data

Re: [PATCH] Bug fix in initdb output

[Juan José Santamaría Flecha]

On Mon, Mar 1, 2021 at 5:52 AM Nitin Jadhav 
wrote:

>
>> Please share your thoughts on this. If we go ahead with this change,
> then we need to back-patch. I would be happy to create those patches.
>

A full path works, even with the slashes. The commiter will take care of
back-patching, if needed. As for HEAD at least, this LGTM.

Regards,

Juan José Santamaría Flecha

Improvements in prepared statements

[Alejandro Sánchez]

Hello, some improvements in the prepared statements would facilitate
their use from applications:

- Use of table and column names in prepared statements.

Example: select # from # where # = ?;

- Use of arrays in prepared statements.

Example: select # from article where id in (?);

# = author,title
? = 10,24,45

Best regards.
Alejandro Sánchez.

SSL negotiation error on massive connect/disconnect

[Maxim Orlov]

Hi!

I have primary server on port 55942 and two standbys on 55943 and 55944. 
Then use connection string like 
"postgresql://127.0.0.1:55942,127.0.0.1:55943,127.0.0.1:55944/postgres" 
to connect to the servers via psql.


Everything works perfectly no matter how many attempts to connect I do.
But when I stop primary server, very rarely I do get an error "received 
invalid response to SSL negotiation" from psql. I got this error when I 
tried to make massive connects/disconnects test and it's unlikely to 
reproduce manually without thousands of connections sequentially with no 
intentional delay in between.


The problem present only on Linux, MacOS works fine.

As far as I understand this particular problem is because of postgresql 
gets "zero" (i.e. 0) byte in SSLok in 
PQconnectPoll@src/interfaces/libpq/fe-connect.c. This lead to select 
"else" branch with described error message. This may be fixed by 
handling zero byte as 'E' byte. But I'm not sure if it's good solution, 
since I don't know why fe-connect gets an zero byte at all.


I consider it's worth to correct this. This error is rare but it's 
really odd to notice this unexpected and wrong behavior.


---
Best regards,
Maxim Orlov.From 860f14c99ec70fcea58897c97e218b8a54286a39 Mon Sep 17 00:00:00 2001
From: Maxim Orlov 
Date: Mon, 1 Mar 2021 16:47:41 +0300
Subject: [PATCH] WIP

---
 src/interfaces/libpq/Makefile   |   3 +
 src/interfaces/libpq/fe-connect.c   |   2 +-
 src/interfaces/libpq/t/004-multihost.pl | 143 
 3 files changed, 147 insertions(+), 1 deletion(-)
 create mode 100644 src/interfaces/libpq/t/004-multihost.pl

diff --git a/src/interfaces/libpq/Makefile b/src/interfaces/libpq/Makefile
index f74677eaf9b..98fd1d694ef 100644
--- a/src/interfaces/libpq/Makefile
+++ b/src/interfaces/libpq/Makefile
@@ -120,6 +120,9 @@ install: all installdirs install-lib
 installcheck:
 	$(MAKE) -C test $@
 
+check:
+	$(prove_check)
+
 installdirs: installdirs-lib
 	$(MKDIR_P) '$(DESTDIR)$(includedir)' '$(DESTDIR)$(includedir_internal)' '$(DESTDIR)$(datadir)'
 
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index db71fea169c..43c14a4433a 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -3023,7 +3023,7 @@ keep_going:		/* We will come back to here until there is
 		conn->status = CONNECTION_MADE;
 		return PGRES_POLLING_WRITING;
 	}
-	else if (SSLok == 'E')
+	else if (SSLok == 'E' || SSLok == '\0')
 	{
 		/*
 		 * Server failure of some sort, such as failure to
diff --git a/src/interfaces/libpq/t/004-multihost.pl b/src/interfaces/libpq/t/004-multihost.pl
new file mode 100644
index 000..116f4a82635
--- /dev/null
+++ b/src/interfaces/libpq/t/004-multihost.pl
@@ -0,0 +1,143 @@
+# target_server_type
+use strict;
+use warnings;
+use PostgresNode;
+use TestLib;
+use Test::More tests => 1;
+use Scalar::Util qw(blessed);
+
+# Initialize master node
+my $node_master = get_new_node('master');
+$node_master->init(allows_streaming => 1);
+$node_master->append_conf('postgresql.conf', "listen_addresses = '$PostgresNode::test_localhost'");
+$node_master->start;
+
+# Take backup
+my $backup_name = 'my_backup';
+$node_master->backup($backup_name);
+
+# Create streaming standby 1 linking to master
+my $node_standby_1 = get_new_node('standby_1');
+$node_standby_1->init_from_backup($node_master, $backup_name, has_streaming => 1);
+$node_standby_1->append_conf('postgresql.conf', "listen_addresses = '$PostgresNode::test_localhost'");
+$node_standby_1->start;
+
+# Create streaming standby 2 linking to master
+my $node_standby_2 = get_new_node('standby_2');
+$node_standby_2->init_from_backup($node_master, $backup_name, has_streaming => 1);
+$node_standby_2->append_conf('postgresql.conf', "listen_addresses = '$PostgresNode::test_localhost'");
+$node_standby_2->start;
+
+sub get_host_port
+{
+	my $node = shift;
+	return "$PostgresNode::test_localhost:" . $node->port;
+}
+
+sub multiconnstring
+{
+	my $nodes= shift;
+	my $database = shift || "postgres";
+	my $params   = shift;
+	my $extra= "";
+	if ($params)
+	{
+		my @cs;
+		while (my ($key, $val) = each %$params)
+		{
+			push @cs, $key . "=" . $val;
+		}
+		$extra = "?" . join("&", @cs);
+	}
+	my $str =
+		"postgresql://"
+		. join(",", map({ get_host_port($_) } @$nodes))
+		. "/$database$extra";
+
+	return $str;
+}
+
+#
+# Copied from PosgresNode.pm passing explicit connect-string instead of
+# constructed from object
+#
+
+sub psql2
+{
+	# We expect dbname to be part of connstr
+	my ($connstr, $sql, %params) = @_;
+
+	my $stdout= $params{stdout};
+	my $stderr= $params{stderr};
+	my @psql_params   = ('psql', '-XAtq', '-d', $connstr, '-f', '-');
+
+	# Allocate temporary ones to capture them so we
+	# can return them. Otherwise we won't redirect them at all.
+	if (!defined($stdout))
+	{
+		my $temp_stdout = "";
+		$stdout = \$temp_stdout;

Re: Improvements in prepared statements

[Pavel Stehule]

Hi

po 1. 3. 2021 v 15:20 odesílatel Alejandro Sánchez 
napsal:

> Hello, some improvements in the prepared statements would facilitate
> their use from applications:
>
> - Use of table and column names in prepared statements.
>
> Example: select # from # where # = ?;
>
> - Use of arrays in prepared statements.
>
> Example: select # from article where id in (?);
>
> # = author,title
> ? = 10,24,45
>

The server side prepared statements are based on reusing execution plans.
You cannot reuse execution plans if you change table, or column. This is
the reason why SQL identifiers are immutable in prepared statements. There
are client side prepared statements - JDBC does it. There it is possible.
But it is impossible on the server side. Prepared statements are like a
compiled program. You can change parameters, variables - but you cannot
change the program.

Regards

Pavel




>
> Best regards.
> Alejandro Sánchez.
>
>
>
>

Re: proposal: psql –help reflecting service or URI usage

[Euler Taveira]

On Mon, Mar 1, 2021, at 10:32 AM, Paul Förster wrote:
> still, what is the line length limit? Where do I find it?
We try to limit it to 80 characters but it is not a hard limit. Long
descriptions should certainly be split into multiple lines.

The question is: how popular is service and connection URIs? We cannot  
  
certainly include all possibilities in the --help that's why we have the
  
documentation. IMO we could probably include "connection string" that accepts 2
formats: (i) multiple keyword - value string and URIs ("service" is included in 
the (i)).   
  

   
Usage:  
  
   psql [OPTION]... [DBNAME [USERNAME]|CONNINFO]
   

   
or even 
  

   
Usage:  
  
   psql [OPTION]... [DBNAME [USERNAME]] 
   
   psql [OPTION]... [CONNINFO]  
   

   

   
Connection options: 
  
   -h, --host=HOSTNAME  database server host or socket directory (default: 
"local socket")
   -p, --port=PORT  database server port (default: "")  
   
   -U, --username=USERNAME  database user name (default: "euler")   
   
   -w, --no-passwordnever prompt for password   
   
   -W, --password   force password prompt (should happen automatically) 

   
   CONNINFO connection string to connect to (key = value strings
or connection URI)  
   

I don't like the CONNINFO in the connection options. It seems a natural place  
but DBNAME and USERNAME aren't included in it. Should we include it too?
  
Someone can argue that they are self-explanatory, hence, a description is not  
necessary.  
  

   
It might be a different topic but since we are talking about --help 
improvements, I have some suggestions:  
  

* an Example section could help newbies to Postgres command-line   
tools to figure out how to inform the connection parameters. In this case, we   
  
could include at least 3 examples: (i) -h, -p, -U parameters, (ii) key/value
  
connection string and (iii) connection URI.
* Connection options could be moved to the top (maybe after General options) if
we consider that it is more important than the other sections (you cannot  
probably execute psql without using a connection parameter in production).


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

Re: macOS SIP, next try

[Tom Lane]

Peter Eisentraut  writes:
> I have since learned that there is a way to disable only the part of SIP 
> that is relevant for us.  This seems like a useful compromise, and it 
> appears that a number of other open-source projects are following the 
> same route.  I suggest the attached documentation patch and then close 
> this issue.

Hmm, interesting.  Where is it documented what this does?

regards, tom lane

Re: A reloption for partitioned tables - parallel_workers

[Laurenz Albe]

On Mon, 2021-03-01 at 17:39 +0900, Amit Langote wrote:
> > I am not sure.
> > IMO, for normal table, we use the following macro to get the 
> > parallel_workers:
> > --
> > /*
> >   * RelationGetParallelWorkers
> >   *  Returns the relation's parallel_workers reloption setting.
> >   *  Note multiple eval of argument!
> >   */
> > #define RelationGetParallelWorkers(relation, defaultpw) \
> >  ((relation)->rd_options ? \
> >   ((StdRdOptions *) (relation)->rd_options)->parallel_workers : 
> > (defaultpw))
> > --
> > Since we add new struct " PartitionedTableRdOptions ", It seems we need to 
> > get parallel_workers in different way.
> > Do we need similar macro to get partitioned table's parallel_workers ?
> 
> Oh, you're right.  The parallel_workers setting of a relation is only
> accessed through this macro, even for partitioned tables, and I can
> see that it is actually wrong to access a partitioned table's
> parallel_workers through this macro as-is.   Although I hadn't tested
> that, so thanks for pointing that out.
> 
> > Like:
> > #define PartitionedTableGetParallelWorkers(relation, defaultpw) \
> > xxx
> > (PartitionedTableRdOptions *) (relation)->rd_options)->parallel_workers : 
> > (defaultpw))
> 
> I'm thinking it would be better to just modify the existing macro to
> check relkind to decide which struct pointer type to cast the value in
> rd_options to.

Here is an updated patch with this fix.

I added regression tests and adapted the documentation a bit.

I also added support for lowering the number of parallel workers.

Yours,
Laurenz Albe
From d48874a61ac698dc284b83b7e3b147a71158d09d Mon Sep 17 00:00:00 2001
From: Laurenz Albe 
Date: Mon, 1 Mar 2021 16:06:49 +0100
Subject: [PATCH] Allow setting parallel_workers on partitioned tables.

Sometimes it's beneficial to do

ALTER TABLE my_part_tbl SET (parallel_workers = 32);

when the query planner is planning too few workers to read from your
partitions. For example, if you have a partitioned table where each
partition is a foreign table that cannot itself have this setting on it.
Shown to give a 100%+ performance increase (in my case, 700%) when used
with cstore_fdw column store partitions.

This is also useful to lower the number of parallel workers, for
example when the executor is expected to prune most partitions.

Authors: Seamus Abshere, Amit Langote, Laurenz Albe
Discussion: https://postgr.es/m/95b1dd96-8634-4545-b1de-e2ac779beb44%40www.fastmail.com
---
 doc/src/sgml/ref/create_table.sgml|  15 +-
 src/backend/access/common/reloptions.c|  14 +-
 src/backend/optimizer/path/allpaths.c | 155 ++
 src/include/utils/rel.h   |  15 +-
 .../regress/expected/partition_aggregate.out  |  51 +-
 src/test/regress/sql/partition_aggregate.sql  |  17 +-
 6 files changed, 188 insertions(+), 79 deletions(-)

diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml
index 3b2b227683..c8c14850c1 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -1337,8 +1337,9 @@ WITH ( MODULUS numeric_literal, REM
 If a table parameter value is set and the
 equivalent toast. parameter is not, the TOAST table
 will use the table's parameter value.
-Specifying these parameters for partitioned tables is not supported,
-but you may specify them for individual leaf partitions.
+These parameters, with the exception of parallel_workers,
+are not supported on partitioned tables, but you may specify them for
+individual leaf partitions.

 

@@ -1401,9 +1402,13 @@ WITH ( MODULUS numeric_literal, REM
  
   This sets the number of workers that should be used to assist a parallel
   scan of this table.  If not set, the system will determine a value based
-  on the relation size.  The actual number of workers chosen by the planner
-  or by utility statements that use parallel scans may be less, for example
-  due to the setting of .
+  on the relation size and the number of scanned partitions.
+  When set on a partitioned table, the specified number of workers will
+  work on distinct partitions, so the number of partitions affected by the
+  parallel operation should be taken into account.
+  The actual number of workers chosen by the planner or by utility
+  statements that use parallel scans may be less, for example due
+  to the setting of .
  
 

diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index c687d3ee9e..f8443d2361 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -377,7 +377,7 @@ static relopt_int intRelOpts[] =
 		{
 			"parallel_workers",
 			"Number of parallel processes that can be used per executor node for this relation.",
-			RELOPT_KIND_HEAP,
+			RELOPT_KIND_HE

2019-03 CF now in progress

[David Steele]

Hackers,

The 2019-03 commitfest is now in progress. It's a big one as usual.

Needs review: 213.
Waiting on Author: 21.
Ready for Committer: 28.
Committed: 29.
Withdrawn: 3.
Total: 294.

If you are a patch author please check http://commitfest.cputube.org to 
be sure your patch still applies, compiles, and passes tests.


Please be sure to review patches of equal or greater complexity to the 
patches you submit. This only works well if everyone participates.


Happy coding!

Regards,
--
-David
da...@pgmasters.net

We should stop telling users to "vacuum that database in single-user mode"

[Hannu Krosing]

It looks like we are unnecessarily instructing our usiers to vacuum their
databases in single-user mode when just vacuuming would be enough.

We should fix the error message to be less misleading.

== The story

I think most of us have at some time seen the following message, if not in their
own database, then at some client.

ERROR: database is not accepting commands to avoid wraparound data
loss in database ""
HINT: Stop the postmaster and vacuum that database in single-user mode.
You might also need to commit or roll back old prepared transactions.

But "vacuum that database in single-user mode" is the very last thing
one wants to
do, because

*   it is single-user mode, so nothing else works ...
*   CHECKPOINTs are not running, so all the WAL segments can not be rotated and
reused
*   Replication does not work, so after vacuum is done and database is started
in normal mode, there is huge backlog to replicate
*   pg_stat_progress_vacuum is not available so you have no idea when the
command is going to complete
*   VACUUM VERBOSE isn't - there is absolutely no output from single-user mode
vacuum with or without VERBOSE, so you *really* do not know what is going on
and how much progress is made (if you are locky you can guess something from
IO and CPU monitoring, but it is inexact at best )

When I started looking at improving the situation I discovered, that there
already is no need to run VACUUM in single user mode in any currently supported
PostgreSQL version as you can run VACUUM perfectly well when the wraparound
protection is active.

It worked in all PG versions from v9.6 to v13.

I also tested v 8.3 as this is where we added virtual transactions, but there
VACUUM really fails to run successfully without single-user mode..

So my proposal is to change the error message [*]  to something that does not
suggest the single-user mode as the requirement for running VACUUM.
Also COMMIT PREPARED still works ok in this situation.

Single-user mode still may be needed in case one needs to drop a
replication slot
or something similar.

[*] The message is in src/backend/access/transam/varsup.c around line 120

=== How to test

The following instructions let you run into wraparound in about an hour,
depending on your setup (was 1.2 hours on my laptop)

 First, set some flags

To allow PREPARE TRANSACTION to block VACUUM cleanup

```
alter system set max_prepared_transactions = 10;
```

Also set *_min_messages to errors, unless you want to get 10M of
WARNINGs (~4GB) to
logs and the same amount sent to client, slowing down the last 10M transactions
significantly.

```
alter system set log_min_messages = error;
alter system set client_min_messages = error;
```

 Restart the system to activate the settings

 Block Vacuum from cleaning up transactions

Create a database `wraptest` and connect to it, then

```
create table t (i int);

BEGIN;
insert into t values(1);
PREPARE TRANSACTION 'trx_id_pin';
```

Now you have a prepared transaction, which makes sure that even well-tuned
autovacuum does not prevent running into the wraparound protection.

```
[local]:5096 hannu@wraptest=# SELECT * FROM pg_prepared_xacts;
 transaction |gid |   prepared| owner | database
-++---+---+--
 593 | trx_id_pin | 2021-03-01 08:57:27.024777+01 | hannu | wraptest
(1 row)
```

 Create a function to consume transaction ids as fast as possible:

```
CREATE OR REPLACE FUNCTION trx_eater(n int) RETURNS void
LANGUAGE plpgsql
AS $plpgsql$
BEGIN
 FOR i IN 0..n LOOP
   BEGIN
 INSERT INTO t values(i);
   EXCEPTION WHEN OTHERS THEN
   RAISE; -- raise it again, so that we actually err out on wraparound
   END;
 END LOOP;
END;
$plpgsql$;
```

 Use pgbench to drive this function

Make a pgbench command file

$ echo 'select trx_eater(10);' > trx_eater.pgbench

and start pgbench to run this function in a few backends in parallel

$ pgbench -c 16 -T 2 -P 60 -n wraptest -f trx_eater.pgbench

 Wait 1-2 hours

In about an hour or two this should error out with

ERROR: database is not accepting commands to avoid wraparound data
loss in database "postgres"
HINT: Stop the postmaster and vacuum that database in single-user mode.
You might also need to commit or roll back old prepared transactions.

After this just do

COMMIT PREPARED 'trx_id_pin';

 Verify that VACUUM still works

to release the blocket 2PC transaction and you can verify yourself that

*   you can run VACUUM on any table, and
*   Autovacuum is working, and will eventually clear up the situation

If you have not tuned autovacuum_vacuum_cost_* at all, especially in earlier
versions where it is 20ms by default the autovacuum-started vacuum is
running really
slowly, and it will take about 8 hours to clean up the table, but this
can be sped up
if you set autovacuum_vacuum_cost_delay=0 and then either restart the database
or just kill th

Re: Improvements in prepared statements

[Alejandro Sánchez]

Hello, as far as I know it is not done in JDBC, in many frameworks it
is.Although the execution plans cannot be reused it would be
somethingvery useful. It is included in a lot of frameworks and is a
recurrentquestion in database forums. It would be nice if it was
included in plain SQL.
Best regards.Alejandro Sánchez.
El lun, 01-03-2021 a las 15:31 +0100, Pavel Stehule escribió:
> Hi
> 
> po 1. 3. 2021 v 15:20 odesílatel Alejandro Sánchez <
> a...@nexttypes.com> napsal:
> > Hello, some improvements in the prepared statements would
> > facilitate
> > 
> > their use from applications:
> > 
> > 
> > 
> > - Use of table and column names in prepared statements.
> > 
> > 
> > 
> > Example: select # from # where # = ?;
> > 
> > 
> > 
> > - Use of arrays in prepared statements.
> > 
> > 
> > 
> > Example: select # from article where id in (?);
> > 
> > 
> > 
> > # = author,title
> > 
> > ? = 10,24,45
> 
> The server side prepared statements are based on reusing execution
> plans. You cannot reuse execution plans if you change table, or
> column. This is the reason why SQL identifiers are immutable in
> prepared statements. There are client side prepared statements - JDBC
> does it. There it is possible. But it is impossible on the server
> side. Prepared statements are like a compiled program. You can change
> parameters, variables - but you cannot change the program.
> 
> Regards
> Pavel
> 
> 
>  
> > 
> > Best regards.
> > 
> > Alejandro Sánchez.
> > 
> > 
> > 
> > 
> > 
> > 
> > 

Re: Improving connection scalability: GetSnapshotData()

[Konstantin Knizhnik]

On 27.02.2021 20:40, AJG wrote:

Hi,

Greatly appreciate if you could please reply to the following questions as
time allows.

I have seen previous discussion/patches on a built-in connection pooler. How
does this scalability improvement, particularly idle connection improvements
etc, affect that built-in pooler need, if any?


Same general question about an external connection pooler in general in
Production? Still required to route to different servers but no longer
needed for the pooling part. as an example.


Many Thanks!



Connection pooler is still needed.
The patch for GetSnapshotData() mostly improves scalability of read-only 
queries and reduce contention for procarray lock.
But read-write upload cause contention for many other resources: 
relation extension lock, buffer locks, tuple locks and so on.


If you run pgbench at NUMA machine with hundreds of cores, then you will 
still observe significant degradation of performance with increasing 
number of connection.
And this degradation will be dramatic if you replace some non-uniform 
distribution of keys, for example Zipfian distribution.









--
Sent from: https://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html

Re: Improvements in prepared statements

[Pavel Stehule]

po 1. 3. 2021 v 16:39 odesílatel Alejandro Sánchez 
napsal:

> Hello, as far as I know it is not done in JDBC, in many frameworks it is.
>
> Although the execution plans cannot be reused it would be something
>
> very useful. It is included in a lot of frameworks and is a recurrent 
> 
>
> question in database forums 
> .
>  It would be nice if it was included in plain
>
> SQL.
>
>
I am very sceptical about it. What benefit do you expect? When you cannot
reuse an execution plan, then there is not any benefit of this. Then you
don't need prepared statements, and all this API is useless. So some
questions are frequent and don't mean necessity to redesign. The developers
just miss the fundamental knowledge of database technology.

Regards

Pavel


> Best regards.
>
> Alejandro Sánchez.
>
>
> El lun, 01-03-2021 a las 15:31 +0100, Pavel Stehule escribió:
>
> Hi
>
> po 1. 3. 2021 v 15:20 odesílatel Alejandro Sánchez 
> napsal:
>
> Hello, some improvements in the prepared statements would facilitate
> their use from applications:
>
> - Use of table and column names in prepared statements.
>
> Example: select # from # where # = ?;
>
> - Use of arrays in prepared statements.
>
> Example: select # from article where id in (?);
>
> # = author,title
> ? = 10,24,45
>
>
> The server side prepared statements are based on reusing execution plans.
> You cannot reuse execution plans if you change table, or column. This is
> the reason why SQL identifiers are immutable in prepared statements. There
> are client side prepared statements - JDBC does it. There it is possible.
> But it is impossible on the server side. Prepared statements are like a
> compiled program. You can change parameters, variables - but you cannot
> change the program.
>
> Regards
>
> Pavel
>
>
>
>
>
> Best regards.
> Alejandro Sánchez.
>
>
>
>

Re: proposal: psql –help reflecting service or URI usage

[Mark Dilger]

> On Feb 28, 2021, at 10:10 AM, Paul Förster  wrote:
> 
> Hi Mark,
> 
>> On 28. Feb, 2021, at 17:54, Mark Dilger  wrote:
>> 
>> "definited" is a typo.
> 
> yes, definitely a typo, sorry. Thanks for pointing this out.
> 
>> Should this say "as defined in pg_service.conf"?  That's the default, but 
>> the user might have $PGSERVICEFILE set to something else.  Perhaps you could 
>> borrow the wording of other options and use "(default: as defined in 
>> pg_service.conf)", or something like that, but of course being careful to 
>> still fit in the line length limit.
> 
> I agree to all, thanks. What is the line length limit?

The output from --help should fit in a terminal window with only 80 characters 
width.  For example, in src/bin/scripts/createuser.c the line about 
--interactive is wrapped:

> printf(_("  -S, --no-superuserrole will not be superuser 
> (default)\n"));
> printf(_("  -V, --version output version information, then 
> exit\n"));
> printf(_("  --interactive prompt for missing role name and 
> attributes rather\n"
>  "than using defaults\n"));
> printf(_("  --replication role can initiate replication\n"));

You can find counter-examples where applications do not follow this rule.  
pg_isready is one of them.


>> Other client applications follow the same pattern as psql, so if this change 
>> were adopted, it should apply to all of them.
> 
> well, psql is central and IMHO the best place to start. I'd have to try out 
> all of them then. What I do know, though, is that pg_isready does not 
> understand a URI (why is that?), which is very unfortunate. So, I'd have to 
> try them all out and supply patches for them all?

There is a pattern in the client applications that the --help output is less 
verbose than the docs (see, for example, 
https://www.postgresql.org/docs/13/reference-client.html).  Your proposed patch 
makes psql's --help output a bit more verbose about this issue while leaving 
the other applications less so, without any obvious reason for the difference.

> Still, supporting a feature and not documenting it in its help is IMHO not a 
> good idea.

Ok.

>> Your proposal seems like something that would have been posted to the list 
>> before, possibly multiple times.  Any chance you could dig up past 
>> conversations on this subject and post links here for context?
> 
> I don't know any past discussions here. I only subscribed today to 
> psql-hackers. It might have been mentioned on psql-general, though. But I'm 
> not sure. This idea popped into my mind just yesterday when I was playing 
> around with psql and URIs and noticed that psql –help doesn't show them.

I mentioned looking at the mailing list archives, but neglected to give you a 
link:  https://www.postgresql.org/list/

Over the years, many proposals get made and debated, with some accepted and 
some rejected.  The rejected proposals often have some merit, and get suggested 
again, only to get rejected for the same reasons as the previous times they 
were suggested.  So searching the archives before posting a patch can sometimes 
be enlightening.  The difficulty in my experience is knowing what words and 
phrases to search for.  It can be a bit time consuming trying to find a prior 
discussion on a topic.

I don't know of any specific discussion which rejected your patch idea.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Improvements in prepared statements

[Alejandro Sánchez]

The benefit is ease of use. One of the great advantages of prepared
statements is nothaving to concatenate strings. The use of arrays would
also be very useful. 
query("select " + column1 + "," + column2 from " " + table + " where id
in (?), ids);
VS
query("select # from # where id in (?)", columns, table, ids);
And it doesn't have to be done with prepared statements, it can just be
another SQL syntax.
El lun, 01-03-2021 a las 16:46 +0100, Pavel Stehule escribió:
> po 1. 3. 2021 v 16:39 odesílatel Alejandro Sánchez <
> a...@nexttypes.com> napsal:
> > Hello, as far as I know it is not done in JDBC, in many frameworks
> > it is.Although the execution plans cannot be reused it would be
> > somethingvery useful. It is included in a lot of frameworks and is
> > a recurrentquestion in database forums. It would be nice if it was
> > included in plain SQL.
> 
> I am very sceptical about it. What benefit do you expect? When you
> cannot reuse an execution plan, then there is not any benefit of
> this. Then you don't need prepared statements, and all this API is
> useless. So some questions are frequent and don't mean necessity to
> redesign. The developers just miss the fundamental knowledge of
> database technology. 
> 
> Regards
> Pavel
> 
> > Best regards.Alejandro Sánchez.
> > El lun, 01-03-2021 a las 15:31 +0100, Pavel Stehule escribió:
> > > Hi
> > > 
> > > po 1. 3. 2021 v 15:20 odesílatel Alejandro Sánchez <
> > > a...@nexttypes.com> napsal:
> > > > Hello, some improvements in the prepared statements would
> > > > facilitate
> > > > 
> > > > their use from applications:
> > > > 
> > > > 
> > > > 
> > > > - Use of table and column names in prepared statements.
> > > > 
> > > > 
> > > > 
> > > > Example: select # from # where # = ?;
> > > > 
> > > > 
> > > > 
> > > > - Use of arrays in prepared statements.
> > > > 
> > > > 
> > > > 
> > > > Example: select # from article where id in (?);
> > > > 
> > > > 
> > > > 
> > > > # = author,title
> > > > 
> > > > ? = 10,24,45
> > > 
> > > The server side prepared statements are based on reusing
> > > execution plans. You cannot reuse execution plans if you change
> > > table, or column. This is the reason why SQL identifiers are
> > > immutable in prepared statements. There are client side prepared
> > > statements - JDBC does it. There it is possible. But it is
> > > impossible on the server side. Prepared statements are like a
> > > compiled program. You can change parameters, variables - but you
> > > cannot change the program.
> > > 
> > > Regards
> > > Pavel
> > > 
> > > 
> > >  
> > > > 
> > > > Best regards.
> > > > 
> > > > Alejandro Sánchez.
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 

Re: Improvements in prepared statements

[Pavel Stehule]

po 1. 3. 2021 v 17:08 odesílatel Alejandro Sánchez 
napsal:

> The benefit is ease of use. One of the great advantages of prepared 
> statements is not
>
> having to concatenate strings. The use of arrays would also be very useful.
>
>
> query("select " + column1 + "," + column2 from " " + table + " where id in 
> (?), ids);
>
>
> VS
>
>
> query("select # from # where id in (?)", columns, table, ids);
>
>
> And it doesn't have to be done with prepared statements, it can just be 
> another SQL syntax.
>
>
This is not too strong an argument - any language (and Database API) has
necessary functionality now. Just you should use it.

You can use fprintf in php, format in plpgsql, String.Format in C#, Java,
...

Regards

Pavel





> El lun, 01-03-2021 a las 16:46 +0100, Pavel Stehule escribió:
>
>
>
> po 1. 3. 2021 v 16:39 odesílatel Alejandro Sánchez 
> napsal:
>
> Hello, as far as I know it is not done in JDBC, in many frameworks it is.
>
> Although the execution plans cannot be reused it would be something
>
> very useful.
>
> It is included in a lot of frameworks and is
>
> a recurrent
>
>
> 
>
> question in database forums
>
>
> 
>
> . I
>
> t
>
>  would be nice if it was included in plain
>
> SQL.
>
>
>
> I am very sceptical about it. What benefit do you expect? When you cannot
> reuse an execution plan, then there is not any benefit of this. Then you
> don't need prepared statements, and all this API is useless. So some
> questions are frequent and don't mean necessity to redesign. The developers
> just miss the fundamental knowledge of database technology.
>
> Regards
>
> Pavel
>
>
> Best regards.
>
> Alejandro Sánchez.
>
>
> El lun, 01-03-2021 a las 15:31 +0100, Pavel Stehule escribió:
>
> Hi
>
> po 1. 3. 2021 v 15:20 odesílatel Alejandro Sánchez 
> napsal:
>
> Hello, some improvements in the prepared statements would facilitate
> their use from applications:
>
> - Use of table and column names in prepared statements.
>
> Example: select # from # where # = ?;
>
> - Use of arrays in prepared statements.
>
> Example: select # from article where id in (?);
>
> # = author,title
> ? = 10,24,45
>
>
> The server side prepared statements are based on reusing execution plans.
> You cannot reuse execution plans if you change table, or column. This is
> the reason why SQL identifiers are immutable in prepared statements. There
> are client side prepared statements - JDBC does it. There it is possible.
> But it is impossible on the server side. Prepared statements are like a
> compiled program. You can change parameters, variables - but you cannot
> change the program.
>
> Regards
>
> Pavel
>
>
>
>
>
> Best regards.
> Alejandro Sánchez.
>
>
>
>

Re: Improvements in prepared statements

[Pavel Stehule]

po 1. 3. 2021 v 17:15 odesílatel Pavel Stehule 
napsal:

>
>
> po 1. 3. 2021 v 17:08 odesílatel Alejandro Sánchez 
> napsal:
>
>> The benefit is ease of use. One of the great advantages of prepared 
>> statements is not
>>
>> having to concatenate strings. The use of arrays would also be very useful.
>>
>>
>> query("select " + column1 + "," + column2 from " " + table + " where id in 
>> (?), ids);
>>
>>
>>
The argument with arrays is not good.  You can work with arrays just on
binary level, that is more effective. But just you should use operator =
ANY() instead IN.

Regards

Pavel



> VS
>>
>>
>> query("select # from # where id in (?)", columns, table, ids);
>>
>>
>> And it doesn't have to be done with prepared statements, it can just be 
>> another SQL syntax.
>>
>>
> This is not too strong an argument - any language (and Database API) has
> necessary functionality now. Just you should use it.
>
> You can use fprintf in php, format in plpgsql, String.Format in C#, Java,
> ...
>
> Regards
>
> Pavel
>
>
>
>
>
>> El lun, 01-03-2021 a las 16:46 +0100, Pavel Stehule escribió:
>>
>>
>>
>> po 1. 3. 2021 v 16:39 odesílatel Alejandro Sánchez 
>> napsal:
>>
>> Hello, as far as I know it is not done in JDBC, in many frameworks it is.
>>
>> Although the execution plans cannot be reused it would be something
>>
>> very useful.
>>
>> It is included in a lot of frameworks and is
>>
>> a recurrent
>>
>>
>> 
>>
>> question in database forums
>>
>>
>> 
>>
>> . I
>>
>> t
>>
>>  would be nice if it was included in plain
>>
>> SQL.
>>
>>
>>
>> I am very sceptical about it. What benefit do you expect? When you cannot
>> reuse an execution plan, then there is not any benefit of this. Then you
>> don't need prepared statements, and all this API is useless. So some
>> questions are frequent and don't mean necessity to redesign. The developers
>> just miss the fundamental knowledge of database technology.
>>
>> Regards
>>
>> Pavel
>>
>>
>> Best regards.
>>
>> Alejandro Sánchez.
>>
>>
>> El lun, 01-03-2021 a las 15:31 +0100, Pavel Stehule escribió:
>>
>> Hi
>>
>> po 1. 3. 2021 v 15:20 odesílatel Alejandro Sánchez 
>> napsal:
>>
>> Hello, some improvements in the prepared statements would facilitate
>> their use from applications:
>>
>> - Use of table and column names in prepared statements.
>>
>> Example: select # from # where # = ?;
>>
>> - Use of arrays in prepared statements.
>>
>> Example: select # from article where id in (?);
>>
>> # = author,title
>> ? = 10,24,45
>>
>>
>> The server side prepared statements are based on reusing execution plans.
>> You cannot reuse execution plans if you change table, or column. This is
>> the reason why SQL identifiers are immutable in prepared statements. There
>> are client side prepared statements - JDBC does it. There it is possible.
>> But it is impossible on the server side. Prepared statements are like a
>> compiled program. You can change parameters, variables - but you cannot
>> change the program.
>>
>> Regards
>>
>> Pavel
>>
>>
>>
>>
>>
>> Best regards.
>> Alejandro Sánchez.
>>
>>
>>
>>

Re: Improvements in prepared statements

[Alejandro Sánchez]

It is a matter of taste. I think this functionality would be better in
SQLand be the same for all languages without the need to use string
functions.
El lun, 01-03-2021 a las 17:15 +0100, Pavel Stehule escribió:
> po 1. 3. 2021 v 17:08 odesílatel Alejandro Sánchez <
> a...@nexttypes.com> napsal:
> > The benefit is ease of use. One of the great advantages of prepared
> > statements is nothaving to concatenate strings. The use of arrays
> > would also be very useful. 
> > query("select " + column1 + "," + column2 from " " + table + "
> > where id in (?), ids);
> > VS
> > query("select # from # where id in (?)", columns, table, ids);
> > 
> > And it doesn't have to be done with prepared statements, it can
> > just be another SQL syntax.
> 
> This is not too strong an argument - any language (and Database API)
> has necessary functionality now. Just you should use it.
> 
> You can use fprintf in php, format in plpgsql, String.Format in C#,
> Java, ...
> 
> Regards
> 
> Pavel
> 
> 
> 
> 
> > El lun, 01-03-2021 a las 16:46 +0100, Pavel Stehule escribió:
> > > po 1. 3. 2021 v 16:39 odesílatel Alejandro Sánchez <
> > > a...@nexttypes.com> napsal:
> > > > Hello, as far as I know it is not done in JDBC, in many
> > > > frameworks it is.Although the execution plans cannot be reused
> > > > it would be somethingvery useful. It is included in a lot of
> > > > frameworks and is a recurrentquestion in database forums. It
> > > > would be nice if it was included in plain SQL.
> > > 
> > > I am very sceptical about it. What benefit do you expect? When
> > > you cannot reuse an execution plan, then there is not any benefit
> > > of this. Then you don't need prepared statements, and all this
> > > API is useless. So some questions are frequent and don't mean
> > > necessity to redesign. The developers just miss the fundamental
> > > knowledge of database technology. 
> > > 
> > > Regards
> > > Pavel
> > > 
> > > > Best regards.Alejandro Sánchez.
> > > > El lun, 01-03-2021 a las 15:31 +0100, Pavel Stehule escribió:
> > > > > Hi
> > > > > 
> > > > > po 1. 3. 2021 v 15:20 odesílatel Alejandro Sánchez <
> > > > > a...@nexttypes.com> napsal:
> > > > > > Hello, some improvements in the prepared statements would
> > > > > > facilitate
> > > > > > 
> > > > > > their use from applications:
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > - Use of table and column names in prepared statements.
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > Example: select # from # where # = ?;
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > - Use of arrays in prepared statements.
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > Example: select # from article where id in (?);
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > # = author,title
> > > > > > 
> > > > > > ? = 10,24,45
> > > > > 
> > > > > The server side prepared statements are based on reusing
> > > > > execution plans. You cannot reuse execution plans if you
> > > > > change table, or column. This is the reason why SQL
> > > > > identifiers are immutable in prepared statements. There are
> > > > > client side prepared statements - JDBC does it. There it is
> > > > > possible. But it is impossible on the server side. Prepared
> > > > > statements are like a compiled program. You can change
> > > > > parameters, variables - but you cannot change the program.
> > > > > 
> > > > > Regards
> > > > > Pavel
> > > > > 
> > > > > 
> > > > >  
> > > > > > 
> > > > > > Best regards.
> > > > > > 
> > > > > > Alejandro Sánchez.
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > 

Re: Improvements in prepared statements

[Pavel Stehule]

Hi

po 1. 3. 2021 v 17:26 odesílatel Alejandro Sánchez 
napsal:

> It is a matter of taste. I think this functionality would be better in SQL
>
> and be the same for all languages without the need to use string functions.
>
>
You can try to implement it, and send a patch. But I think a) it will be
just string concatenation, and then it is surely useless, or b) you should
introduce a new parser, because current parser need to know SQL identifiers
immediately.  But anyway - anybody can write your opinion here. From me - I
don't like this idea.

Regards

Pavel




>
> El lun, 01-03-2021 a las 17:15 +0100, Pavel Stehule escribió:
>
>
>
> po 1. 3. 2021 v 17:08 odesílatel Alejandro Sánchez 
> napsal:
>
> The benefit is ease of use. O
>
> ne of the great advantages of prepared statements is not
>
> having to concatenate strings. The use of arrays would also be very useful.
>
>
> query("select " + column1 + "," + column2 from " " + table + " where id in 
> (?), ids);
>
>
> VS
>
>
> query("select # from # where id in (?)", columns, table, ids);
>
>
> And it doesn't have to be done with prepared statements, it can just be 
> another SQL syntax.
>
>
> This is not too strong an argument - any language (and Database API) has
> necessary functionality now. Just you should use it.
>
> You can use fprintf in php, format in plpgsql, String.Format in C#, Java,
> ...
>
> Regards
>
> Pavel
>
>
>
>
>
> El lun, 01-03-2021 a las 16:46 +0100, Pavel Stehule escribió:
>
>
>
> po 1. 3. 2021 v 16:39 odesílatel Alejandro Sánchez 
> napsal:
>
> Hello, as far as I know it is not done in JDBC, in many frameworks it is.
>
> Although the execution plans cannot be reused it would be something
>
> very useful.
>
> It is included in a lot of frameworks and is
>
> a recurrent
>
>
> 
>
> question in database forums
>
>
> 
>
> . I
>
> t
>
>  would be nice if it was included in plain
>
> SQL.
>
>
>
> I am very sceptical about it. What benefit do you expect? When you cannot
> reuse an execution plan, then there is not any benefit of this. Then you
> don't need prepared statements, and all this API is useless. So some
> questions are frequent and don't mean necessity to redesign. The developers
> just miss the fundamental knowledge of database technology.
>
> Regards
>
> Pavel
>
>
> Best regards.
>
> Alejandro Sánchez.
>
>
> El lun, 01-03-2021 a las 15:31 +0100, Pavel Stehule escribió:
>
> Hi
>
> po 1. 3. 2021 v 15:20 odesílatel Alejandro Sánchez 
> napsal:
>
> Hello, some improvements in the prepared statements would facilitate
> their use from applications:
>
> - Use of table and column names in prepared statements.
>
> Example: select # from # where # = ?;
>
> - Use of arrays in prepared statements.
>
> Example: select # from article where id in (?);
>
> # = author,title
> ? = 10,24,45
>
>
> The server side prepared statements are based on reusing execution plans.
> You cannot reuse execution plans if you change table, or column. This is
> the reason why SQL identifiers are immutable in prepared statements. There
> are client side prepared statements - JDBC does it. There it is possible.
> But it is impossible on the server side. Prepared statements are like a
> compiled program. You can change parameters, variables - but you cannot
> change the program.
>
> Regards
>
> Pavel
>
>
>
>
>
> Best regards.
> Alejandro Sánchez.
>
>
>
>

Re: Improvements in prepared statements

[Alejandro Sánchez]

I have already implemented this in my Java project with some kind of
SQL preprocessor. I leave the idea here in case more people are
interested and PostgreSQL developers findit convenient to include it.
It is just string concatenation but it is a sintactic sugar very useful
in any SQL application. 
El lun, 01-03-2021 a las 17:35 +0100, Pavel Stehule escribió:
> Hi
> 
> po 1. 3. 2021 v 17:26 odesílatel Alejandro Sánchez <
> a...@nexttypes.com> napsal:
> > It is a matter of taste. I think this functionality would be better
> > in SQLand be the same for all languages without the need to use
> > string functions.
> 
> You can try to implement it, and send a patch. But I think a) it will
> be just string concatenation, and then it is surely useless, or b)
> you should introduce a new parser, because current parser need to
> know SQL identifiers immediately.  But anyway - anybody can write
> your opinion here. From me - I don't like this idea. 
> 
> Regards
> Pavel

Re: Add --tablespace option to reindexdb

[Mark Dilger]

> On Feb 25, 2021, at 10:49 PM, Michael Paquier  wrote:
> 
> Hi all,

Hi Michael,

> Since c5b2860, it is possible to specify a tablespace for a REINDEX,
> but the equivalent option has not been added to reindexdb.  Attached
> is a patch to take care of that.
> 
> This includes documentation and tests.

The documentation of the TABLESPACE option for REINDEX says:

+  Specifies that indexes will be rebuilt on a new tablespace.

but in your patch for reindexdb, it is less clear:

+Specifies the tablespace to reindex on. (This name is processed as
+a double-quoted identifier.)

I think the language "to reindex on" could lead users to think that indexes 
already existent in the given tablespace will be reindexed.  In other words, 
the option might be misconstrued as a way to specify all the indexes you want 
reindexed.  Whatever you do here, beware that you are using similar language in 
the --help, so consider changing that, too.


—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

[PATCH] regexp_positions ( string text, pattern text, flags text ) → setof int4range[]

[Joel Jacobson]

Hi,

I suggest adding a new function, regexp_positions(),
which works exactly like regexp_matches(),
except it returns int4range[] start/end positions for *where* the matches 
occurs.

I first thought I could live without this function,
and just get the positions using strpos(),
but as Andreas Karlsson kindly helped me understand,
that naive idea doesn't always work.

Andreas provided this pedagogic example
to demonstrate the problem:

SELECT regexp_matches('caaabaaa', '(?<=b)(a+)', 'g');
regexp_matches

{aaa}
(1 row)

If we would try to use strpos() to find the position,
based on the returned matched substring,
we would get the wrong answer:

SELECT strpos('caaabaaa','aaa');
strpos

  2
(1 row)

Sure, there is "aaa" at position 2,
but that's not where the match occurred,
since the (?<=b) means "positive lookbehind of the character b",
so the match actually occurred at position 6,
where there is a "b" before the "aaa".

Using regexp_positions(), we can now get the correct answer:

SELECT regexp_positions('caaabaaa', '(?<=b)(a+)', 'g');
regexp_positions
--
{"[6,9)"}
(1 row)

Some more examples from the regress/sql/strings.sql,
showing both regexp_matches() and regexp_positions()
for the same examples, as they both return the same structure,
but with different types:

SELECT regexp_matches('foobarbequebazilbarfbonk', $re$(b[^b]+)(b[^b]+)$re$, 
'g');
regexp_matches

{bar,beque}
{bazil,barf}
(2 rows)

SELECT regexp_positions('foobarbequebazilbarfbonk', $re$(b[^b]+)(b[^b]+)$re$, 
'g');
   regexp_positions
---
{"[4,7)","[7,12)"}
{"[12,17)","[17,21)"}
(2 rows)

I've added documentation and tests.

Forgive me for just sending a patch without much discussion on the list,
but it was so easy to implement, so I thought an implementation can
help the discussion on if this is something we want or not.

A few words on the implementation:
I copied build_regexp_match_result() to a new function 
build_regexp_positions_result(),
and removed the string parts, replacing it with code to make ranges instead.
Maybe there are common parts that could be put into some helper-function,
but I think not, since the functions are two small for it to be worth it.

Thanks to David Fetter for the idea on using ranges.

Based on HEAD (f5a5773a9dc4185414fe538525e20d8512c2ba35).

/Joel

0001-regexp-positions.patch
Description: Binary data

Re: Add --tablespace option to reindexdb

[Mark Dilger]

> On Feb 25, 2021, at 10:49 PM, Michael Paquier  wrote:
> 
> While on it, I have added tests for toast tables and indexes with a
> tablespace move during a REINDEX. 


In t/090_reindexdb.pl, you add:

+# Create a tablespace for testing.
+my $ts = $node->basedir . '/regress_reindex_tbspace';
+mkdir $ts or die "cannot create directory $ts";
+# this takes care of WIN-specific path issues
+my $ets = TestLib::perl2host($ts);
+my $tbspace = 'reindex_tbspace';
+$node->safe_psql('postgres', "CREATE TABLESPACE $tbspace LOCATION '$ets';");

I recognize that you are borrowing some of that from 
src/bin/pgbench/t/001_pgbench_with_server.pl, but I don't find anything 
intuitive about the name "ets" and would rather not see that repeated.  There 
is nothing in TestLib::perl2host to explain this name choice, nor in pgbench's 
test, nor yours.

You also change a test:

 $node->issues_sql_like(
-   [ 'reindexdb', '-v', '-t', 'test1', 'postgres' ],
-   qr/statement: REINDEX \(VERBOSE\) TABLE public\.test1;/,
-   'reindex with verbose output');
+   [ 'reindexdb', '--concurrently', '-v', '-t', 'test1', 'postgres' ],
+   qr/statement: REINDEX \(VERBOSE\) TABLE CONCURRENTLY public\.test1;/,
+   'reindex concurrently with verbose output');

but I don't see what tests of the --concurrently option have to do with this 
patch.  I'm not saying there is anything wrong with this change, but it seems 
out of place.  Am I missing something?

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Add --tablespace option to reindexdb

[Mark Dilger]

> On Feb 25, 2021, at 10:49 PM, Michael Paquier  wrote:
> 
> Anyway, I would rather group the whole set of
> tests together, and using the --tablespace option introduced here
> within a TAP test does the job.

Your check verifies that reindexing a system table on a new tablespace fails, 
but does not verify what the failure was.  I wonder if you might want to make 
it more robust, something like:

diff --git a/src/bin/scripts/t/090_reindexdb.pl 
b/src/bin/scripts/t/090_reindexdb.pl
index 6946268209..8453acc817 100644
--- a/src/bin/scripts/t/090_reindexdb.pl
+++ b/src/bin/scripts/t/090_reindexdb.pl
@@ -3,7 +3,7 @@ use warnings;
 
 use PostgresNode;
 use TestLib;
-use Test::More tests => 54;
+use Test::More tests => 58;
 
 program_help_ok('reindexdb');
 program_version_ok('reindexdb');
@@ -108,23 +108,35 @@ $node->issues_sql_like(
 # names, and CONCURRENTLY cannot be used in transaction blocks, preventing
 # the use of TRY/CATCH blocks in a custom function to filter error
 # messages.
-$node->command_fails(
+$node->command_checks_all(
[ 'reindexdb', '-t', $toast_table, '--tablespace', $tbspace, 'postgres' 
],
+   1,
+   [ ],
+   [ qr/cannot move system relation/ ],
'reindex toast table with tablespace');
-$node->command_fails(
+$node->command_checks_all(
[
'reindexdb','--concurrently', '-t', $toast_table,
'--tablespace', $tbspace, 'postgres'
],
+   1,
+   [ ],
+   [ qr/cannot move system relation/ ],
'reindex toast table concurrently with tablespace');
-$node->command_fails(
+$node->command_checks_all(
[ 'reindexdb', '-i', $toast_index, '--tablespace', $tbspace, 'postgres' 
],
+   1,
+   [ ],
+   [ qr/cannot move system relation/ ],
'reindex toast index with tablespace');
-$node->command_fails(
+$node->command_checks_all(
[
'reindexdb','--concurrently', '-i', $toast_index,
'--tablespace', $tbspace, 'postgres'
],
+   1,
+   [ ],
+   [ qr/cannot move system relation/ ],
'reindex toast index concurrently with tablespace');
 
 # connection strings


—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [PATCH] Bug fix in initdb output

[Alvaro Herrera]

On 2021-Mar-01, Juan José Santamaría Flecha wrote:

> On Mon, Mar 1, 2021 at 5:52 AM Nitin Jadhav 
> wrote:
> 
> >
> >> Please share your thoughts on this. If we go ahead with this change,
> > then we need to back-patch. I would be happy to create those patches.
> 
> A full path works, even with the slashes. The commiter will take care of
> back-patching, if needed. As for HEAD at least, this LGTM.

I don't get it.  I thought the windows API accepted both forward slashes
and backslashes as path separators.  Did you try the command and see it
fail?

-- 
Álvaro Herrera39°49'30"S 73°17'W
"Entristecido, Wutra (canción de Las Barreras)
echa a Freyr a rodar
y a nosotros al mar"

Re: [PATCH] Cross-reference comments on signal handling logic

[Mark Dilger]

> On Jan 17, 2021, at 11:51 PM, Craig Ringer  
> wrote:
> 
> 

In src/backend/postmaster/interrupt.c:

+ * These handlers are NOT used by normal user backends as they do not support

vs.

+ * Most backends use this handler.

These two comments seem to contradict.  If interrupt.c contains handlers that 
normal user backends to not use, then how can it be that most backends use one 
of the handlers in the file?

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [PATCH] Bug fix in initdb output

[Juan José Santamaría Flecha]

El lun., 1 mar. 2021 19:16, Alvaro Herrera 
escribió:

>
> I don't get it.  I thought the windows API accepted both forward slashes
> and backslashes as path separators.  Did you try the command and see it
> fail?
>

This is not a problem with the APi, but the shell. e.g. when using a CMD:

- This works:
c:\>c:\Windows\System32\notepad.exe
c:\>c:/Windows/System32/notepad.exe
c:\>/Windows/System32/notepad.exe

- This doesn't:
c:\>./Windows/System32/notepad.exe
'.' is not recognized as an internal or external command,
operable program or batch file.
c:\>Windows/System32/notepad.exe
'Windows' is not recognized as an internal or external command,
operable program or batch file.

Regards,

Juan José Santamaría Flecha

Re: [PATCH] Bug fix in initdb output

[Alvaro Herrera]

On 2021-Mar-01, Juan José Santamaría Flecha wrote:

> This is not a problem with the APi, but the shell. e.g. when using a CMD:
> 
> - This works:
> c:\>c:\Windows\System32\notepad.exe
> c:\>c:/Windows/System32/notepad.exe
> c:\>/Windows/System32/notepad.exe
> 
> - This doesn't:
> c:\>./Windows/System32/notepad.exe
> '.' is not recognized as an internal or external command,
> operable program or batch file.
> c:\>Windows/System32/notepad.exe
> 'Windows' is not recognized as an internal or external command,
> operable program or batch file.

Ah, so another way to fix it would be to make the path to pg_ctl be
absolute?

-- 
Álvaro Herrera39°49'30"S 73°17'W

Re: [PATCH] Bug fix in initdb output

[Juan José Santamaría Flecha]

On Mon, Mar 1, 2021 at 7:50 PM Alvaro Herrera 
wrote:

> On 2021-Mar-01, Juan José Santamaría Flecha wrote:
>
> > This is not a problem with the APi, but the shell. e.g. when using a CMD:
> >
> > - This works:
> > c:\>c:\Windows\System32\notepad.exe
> > c:\>c:/Windows/System32/notepad.exe
> > c:\>/Windows/System32/notepad.exe
> >
> > - This doesn't:
> > c:\>./Windows/System32/notepad.exe
> > '.' is not recognized as an internal or external command,
> > operable program or batch file.
> > c:\>Windows/System32/notepad.exe
> > 'Windows' is not recognized as an internal or external command,
> > operable program or batch file.
>
> Ah, so another way to fix it would be to make the path to pg_ctl be
> absolute?
>

Yes, that's right. If you call initdb with an absolute path you won't see a
problem.

Regards,

Juan José Santamaría Flecha

Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index.

[Mark Dilger]

> On Feb 9, 2021, at 10:43 AM, Pavel Borisov  wrote:
> 
> вт, 9 февр. 2021 г. в 01:46, Mark Dilger :
> 
> 
> > On Feb 8, 2021, at 2:46 AM, Pavel Borisov  wrote:
> > 
> > 0002 - is a temporary hack for testing. It will allow inserting duplicates 
> > in a table even if an index with the exact name "idx" has a unique 
> > constraint (generally it is prohibited to insert). Then a new amcheck will 
> > tell us about these duplicates. It's pity but testing can not be done 
> > automatically, as it needs a core recompile. For testing I'd recommend a 
> > protocol similar to the following:
> > 
> > - Apply patch 0002
> > - Set autovaccum = off in postgresql.conf
> 
> Thanks Pavel and Anastasia for working on this!
> 
> Updating pg_catalog directly is ugly, but the following seems a simpler way 
> to set up a regression test than having to recompile.  What do you think?
> 
> Very nice idea, thanks!
> I've made a regression test based on it. PFA v.2 of a patch. Now it doesn't 
> need anything external for testing.

If bt_index_check() and bt_index_parent_check() are to have this functionality, 
shouldn't there be an option controlling it much as the option (heapallindexed 
boolean) controls checking whether all entries in the heap are indexed in the 
btree?  It seems inconsistent to have an option to avoid checking the heap for 
that, but not for this.  Alternately, this might even be better coded as its 
own function, named something like bt_unique_index_check() perhaps.  I hope 
Peter might advise?

The regression test you provided is not portable.  I am getting lots of errors 
due to differing output of the form "page lsn=0/4DAD7E0".  You might turn this 
into a TAP test and use a regular expression to check the output.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Use extended statistics to estimate (Var op Var) clauses

[Mark Dilger]

> On Nov 12, 2020, at 5:14 PM, Tomas Vondra  
> wrote:
> 
> <0001-Support-estimation-of-clauses-of-the-form-V-20201113.patch>

Your patch no longer applies.  Can we get a new version please?

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index.

[Pavel Borisov]

>
> The regression test you provided is not portable.  I am getting lots of
> errors due to differing output of the form "page lsn=0/4DAD7E0".  You might
> turn this into a TAP test and use a regular expression to check the output.
>
May I ask you to ensure you used v3 of a patch to check? I've made tests
portable in v3, probably, you've checked not the last version.

Thanks for your attention to the patch
-- 
Best regards,
Pavel Borisov

Postgres Professional: http://postgrespro.com 

buildfarm windows checks / tap tests on windows

[Andres Freund]

Hi,

As part of trying to make the aio branch tests on cirrus CI pass with
some tap tests I noticed that "src/tools/msvc/vcregress.pl recoverycheck"
hangs. A long phase of remote debugging later I figured out that that's
not a fault of the aio branch - it also doesn't pass on master (fixed in
[1]).

Which confused me - shouldn't the buildfarm have noticed? But it seems
that all msvc animals either don't have tap tests enabled or they
disable 'misc-checks' which in turn is what the buildfarm client uses to
trigger all the 'recovery' checks.

It seems we're not just skipping recovery, but also e.g. tap tests in
contrib, all the tests in src/test/modules/...

Andrew, what's the reason for that? Is it just that they hung at some
point? Were too slow?


On that last point: Running the tap tests on windows appears to be
*excruciatingly* slow. How does anybody develop on windows without a
mechanism to actually run tests in parallel?

I think it'd be good if vcregress.pl at least respected PROVE_FLAGS from
the environment - it can't currently be passed in for several of the
vcregress.pl tests, and it does seem to make things to be at least
somewhat less painful.


This makes it even clearer to me that we really need a builtin
testrunner that runs tests efficiently *and* debuggable on windows.

Greetings,

Andres Freund

[1] 
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=1e6e40447115ca7b4749d7d117b81b016ee5e2c2

Re: [PATCH] Bug fix in initdb output

[Alvaro Herrera]

On 2021-Mar-01, Juan José Santamaría Flecha wrote:

> On Mon, Mar 1, 2021 at 7:50 PM Alvaro Herrera 
> wrote:

> > Ah, so another way to fix it would be to make the path to pg_ctl be
> > absolute?
> 
> Yes, that's right. If you call initdb with an absolute path you won't see a
> problem.

So, is make_native_path a better fix than make_absolute_path?  (I find
it pretty surprising that initdb shows a relative path, but maybe that's
just me.)

-- 
Álvaro Herrera39°49'30"S 73°17'W
"How strange it is to find the words "Perl" and "saner" in such close
proximity, with no apparent sense of irony. I doubt that Larry himself
could have managed it." (ncm, http://lwn.net/Articles/174769/)

Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index.

[Mark Dilger]

> On Mar 1, 2021, at 12:05 PM, Pavel Borisov  wrote:
> 
> The regression test you provided is not portable.  I am getting lots of 
> errors due to differing output of the form "page lsn=0/4DAD7E0".  You might 
> turn this into a TAP test and use a regular expression to check the output.
> May I ask you to ensure you used v3 of a patch to check? I've made tests 
> portable in v3, probably, you've checked not the last version.

Yes, my review was of v2.  Updating to v3, I see that the test passes on my 
laptop.  It still looks brittle to have all the tid values in the test output, 
but it does pass.

> Thanks for your attention to the patch

Thanks for the patch!

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Regex back-reference semantics and performance

[Tom Lane]

"Joel Jacobson"  writes:
> On Mon, Mar 1, 2021, at 01:53, Tom Lane wrote:
>> 0001-fix-backref-semantics.patch
>> 0002-backref-performance-hack.patch

> I've successfully tested both patches.

Again, thanks for testing!

> On HEAD the trouble-query took forever, I cancelled it after 23 minutes.

Yeah, I have not had the patience to run it to completion either.

> No notable timing differences:

I'm seeing a win of maybe 1% across the entire corpus, which isn't
much but it's something.  It's not too surprising that this backref
issue is seldom hit, or we'd have had more complaints about it.

BTW, I had what seemed like a great idea to improve the situation in
the left-deep-tree case I talked about: we could remember the places
where we'd had to use the NFA to check a backreference subre, and
then at the point where we capture the reference string, recheck any
previous approximate answers, and fail the capture node's match when
any previous backref doesn't match.  Turns out this only mostly works.
In corner cases where the match is ambiguous, it can change the
results from what we got before, which I don't think is acceptable.
Basically, that allows the backref node to have action-at-a-distance
effects on where the earlier concat node divides a substring, which
changes the behavior.

So it seems this is about the best we can do for now.  I'll wait
a little longer to see if anyone complains about the proposed
semantics change, though.

regards, tom lane

Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index.

[Tom Lane]

Mark Dilger  writes:
> Yes, my review was of v2.  Updating to v3, I see that the test passes on my 
> laptop.  It still looks brittle to have all the tid values in the test 
> output, but it does pass.

Hm, anyone tried it on 32-bit hardware?

regards, tom lane

Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index.

[Pavel Borisov]

>
> If bt_index_check() and bt_index_parent_check() are to have this
> functionality, shouldn't there be an option controlling it much as the
> option (heapallindexed boolean) controls checking whether all entries in
> the heap are indexed in the btree?  It seems inconsistent to have an option
> to avoid checking the heap for that, but not for this.  Alternately, this
> might even be better coded as its own function, named something like
> bt_unique_index_check() perhaps.  I hope Peter might advise?
>

As for heap checking, my reasoning was that we can not check whether a
unique constraint violated by the index, without checking heap tuple
visibility. I.e. we can have many identical index entries without
uniqueness violated if only one of them corresponds to a visible heap
tuple. So heap checking included in my patch is _necessary_ for unique
constraint checking, it should not have an option to be disabled,
otherwise, the only answer we can get is that unique constraint MAY be
violated and may not be, which is quite useless. If you meant something
different, please elaborate.

I can try to rewrite unique constraint checking to be done after all others
check but I suppose it's the performance considerations are that made
previous amcheck routines to do many checks simultaneously. I tried to
stick to this practice. It's also not so elegant to duplicate much code to
make uniqueness checks independently and the resulting patch will be much
bigger and harder to review.

Anyway, your and Peter's further considerations are always welcome.

-- 
Best regards,
Pavel Borisov

Postgres Professional: http://postgrespro.com 

Re: Improvements in prepared statements

[Alejandro Sánchez]

Using any() has the disadvantage that in JDBC it is necessaryto create
an array with connection.createArrayOf() and indicatethe type of the
array, which complicates automation.
With statement.setObject() you can pass any type of parameter.JDBC
could add a method that doesn't need the array type.
String sql = "select author from article where id = any(?)";
try (PreparedStatement statement = connection.prepareStatement(sql)) {  
statement.setArray(1,
connection.createArrayOf("varchar", new String[] {"home",
"system"}));}
VS
query("select author from article where id = any(?)",  new String[]
{"home", "system"});
El lun, 01-03-2021 a las 17:21 +0100, Pavel Stehule escribió:
> po 1. 3. 2021 v 17:15 odesílatel Pavel Stehule <
> pavel.steh...@gmail.com> napsal:
> > po 1. 3. 2021 v 17:08 odesílatel Alejandro Sánchez <
> > a...@nexttypes.com> napsal:
> > > The benefit is ease of use. One of the great advantages of
> > > prepared statements is nothaving to concatenate strings. The use
> > > of arrays would also be very useful. 
> > > query("select " + column1 + "," + column2 from " " + table + "
> > > where id in (?), ids);
> 
> The argument with arrays is not good.  You can work with arrays just
> on binary level, that is more effective. But just you should use
> operator = ANY() instead IN. 
> 
> Regards
> Pavel

Re: Use extended statistics to estimate (Var op Var) clauses

[Tomas Vondra]

On 3/1/21 8:58 PM, Mark Dilger wrote:




On Nov 12, 2020, at 5:14 PM, Tomas Vondra  wrote:

<0001-Support-estimation-of-clauses-of-the-form-V-20201113.patch>


Your patch no longer applies.  Can we get a new version please?



I do not plan to work on this patch in the 2021-03 commitfest. I'll 
focus on the other patch about extended statistics on expressions.


regards

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

Re: [PATCH] Bug fix in initdb output

[Juan José Santamaría Flecha]

On Mon, Mar 1, 2021 at 9:09 PM Alvaro Herrera 
wrote:

> On 2021-Mar-01, Juan José Santamaría Flecha wrote:
>
> > On Mon, Mar 1, 2021 at 7:50 PM Alvaro Herrera 
> > wrote:
>
> > > Ah, so another way to fix it would be to make the path to pg_ctl be
> > > absolute?
> >
> > Yes, that's right. If you call initdb with an absolute path you won't
> see a
> > problem.
>
> So, is make_native_path a better fix than make_absolute_path?  (I find
> it pretty surprising that initdb shows a relative path, but maybe that's
> just me.)
>

Uhm, now that you point it out, an absolute path would make the message
more consistent and reusable.

Regards,

Juan José Santamaría Flecha

Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index.

[Mark Dilger]

> On Mar 1, 2021, at 12:23 PM, Pavel Borisov  wrote:
> 
> If bt_index_check() and bt_index_parent_check() are to have this 
> functionality, shouldn't there be an option controlling it much as the option 
> (heapallindexed boolean) controls checking whether all entries in the heap 
> are indexed in the btree?  It seems inconsistent to have an option to avoid 
> checking the heap for that, but not for this.  Alternately, this might even 
> be better coded as its own function, named something like 
> bt_unique_index_check() perhaps.  I hope Peter might advise?
>  
> As for heap checking, my reasoning was that we can not check whether a unique 
> constraint violated by the index, without checking heap tuple visibility. 
> I.e. we can have many identical index entries without uniqueness violated if 
> only one of them corresponds to a visible heap tuple. So heap checking 
> included in my patch is _necessary_ for unique constraint checking, it should 
> not have an option to be disabled, otherwise, the only answer we can get is 
> that unique constraint MAY be violated and may not be, which is quite 
> useless. If you meant something different, please elaborate. 

I completely agree that checking uniqueness requires looking at the heap, but I 
don't agree that every caller of bt_index_check on an index wants that 
particular check to be performed.  There are multiple ways in which an index 
might be corrupt, and Peter wrote the code to only check some of them by 
default, with options to expand the checks to other things.  This is why 
heapallindexed is optional.  If you don't want to pay the price of checking all 
entries in the heap against the btree, you don't have to.

I'm not against running uniqueness checks on unique indexes.  It seems fairly 
normal that a user would want that.  Perhaps the option should default to 
'true' if unspecified?  But having no option at all seems to run contrary to 
how the other functionality is structured.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Improvements in prepared statements

[Pavel Stehule]

po 1. 3. 2021 v 21:35 odesílatel Alejandro Sánchez 
napsal:

> Using any() has the disadvantage that in JDBC it is necessary
>
> to create an array with connection.createArrayOf() and indicate
>
> the type of the array, which complicates automation.
>
>
> With statement.setObject() you can pass any type of parameter.
>
> JDBC could add a method that doesn't need the array type.
>
>
> String sql = "select author from article where id = any(?)";
> try (PreparedStatement statement = connection.prepareStatement(sql)) {
> statement.setArray(1, connection.createArrayOf("varchar",
> new String[] {"home", "system"}));
> }
>
> VS
>
> query("select author from article where id = any(?)", new String[]
> {"home", "system"});
>

Can be, but this is a client side issue. It is about design of client side
API.

Pavel



> El lun, 01-03-2021 a las 17:21 +0100, Pavel Stehule escribió:
>
>
>
> po 1. 3. 2021 v 17:15 odesílatel Pavel Stehule 
> napsal:
>
>
>
> po 1. 3. 2021 v 17:08 odesílatel Alejandro Sánchez 
> napsal:
>
> The benefit is ease of use. O
>
> ne of the great advantages of prepared statements is not
>
> having to concatenate strings. The use of arrays would also be very useful.
>
>
> query("select " + column1 + "," + column2 from " " + table + " where id in 
> (?), ids);
>
>
>
>
> The argument with arrays is not good.  You can work with arrays just on
> binary level, that is more effective. But just you should use operator =
> ANY() instead IN.
>
> Regards
>
> Pavel
>
>
>

Why does the BF sometimes not dump regression.diffs?

[Thomas Munro]

Hello,

I saw this failure after a recent commit (though the next build
succeeded, and I don't yet have any particular reason to believe that
the commits it blamed are at fault, we'll see...):

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=gombessa&dt=2021-03-01%2004%3A58%3A17

Strangely, it didn't spit out the regression.diffs so I don't know
anything more than "commit_ts failed".  I emailed the owner asking for
that log but I see now it's too late.  I wonder if there is some
scripting in there that doesn't work correctly on OpenBSD for whatever
reason...  I know that we *sometimes* get regression.diffs from
failures in here, but I haven't tried to figure out the pattern (which
animals etc).  Here's a similar failure that does show the .diffs I
wish I could see, same BF client version (11), on Windows:

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=fairywren&dt=2020-07-13%2000:47:47

Re: new heapcheck contrib module

[Robert Haas]

On Wed, Feb 24, 2021 at 1:55 PM Mark Dilger
 wrote:
> [ new patches ]

Regarding 0001:

There seem to be whitespace-only changes to the comment for select_loop().

I wonder if the ParallelSlotsSetupOneDB()/ParallelSlotsSetupMinimal()
changes could be simpler. First idea: Suppose you had
ParallelSlotsSetup(numslots) that just creates the slot array with 0
connections, and then ParallelSlotsAdopt(slots, conn, cparams) if you
want to make it own an existing connection. That seems like it might
be cleaner. Second idea: Why not get rid of ParallelSlotsSetupOneDB()
altogether, and just let ParallelSlotsGetIdle() connect the other
slots as required? Preconnecting all slots before we do anything is
good because ... of what?

I also wonder if things might be simplified by introducing a wrapper
object, e.g. ParallelSlotArray. Suppose a ParallelSlotArray stores the
number of slots (num_slots), the array of actual PGconn objects, and
the ConnParams to be used for new connections, and the initcmd to be
used for new connections. Maybe also the progname. This seems like it
would avoid a bunch of repeated parameter passing: you could just
create the ParallelSlotArray with the right contents, and then pass it
around everywhere, instead of having to keep passing the same stuff
in. If you want to switch to connecting to a different DB, you tweak
the ConnParams - maybe using an accessor function - and the system
figures the rest out.

I wonder if it's really useful to generalize this to a point of caring
about all the ConnParams fields, too. Like, if you only provide
ParallelSlotUpdateDB(slotarray, dbname), then that's the only field
that can change so you don't need to care about the others. And maybe
you also don't really need to keep the ConnParams fields in every
slot, either. Like, couldn't you just say something like: if
(strcmp(PQdb(conn) , slotarray->cparams->dbname) != 0) { wrong DB,
can't reuse without a reconnect }? I know sometimes a dbname is really
a whole connection string, but perhaps we could try to fix that by
using PQconninfoParse() in the right place, so that what ends up in
the cparams is just the db name, not a whole connection string.

This is just based on a relatively short amount of time spent studying
the patch, so I might well be off-base here. What do you think?

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

Re: [PATCH] Bug fix in initdb output

[Alvaro Herrera]

On 2021-Mar-01, Juan José Santamaría Flecha wrote:

> Uhm, now that you point it out, an absolute path would make the message
> more consistent and reusable.

Well.  This code was introduced in a00c58314745, with discussion at
http://postgr.es/m/CAHeEsBeAe1FeBypT3E8R1ZVZU0e8xv3A-7BHg6bEOi=jzny...@mail.gmail.com
which did not touch on the point of the pg_ctl path being relative or
absolute.  The previous decision to use relative seems to have been made
here in commit ee814b4511ec, which was backed by this discussion
https://www.postgresql.org/message-id/flat/200411020134.52513.peter_e%40gmx.net

So I'm not sure that anybody would love me if I change it again to
absolute.

-- 
Álvaro Herrera   Valdivia, Chile
Voy a acabar con todos los humanos / con los humanos yo acabaré
voy a acabar con todos (bis) / con todos los humanos acabaré ¡acabaré! (Bender)

Re: enable_incremental_sort changes query behavior

[rbrooks]

Just a bump to see if there is a time frame for a fix on this.

Google Cloud doesn't yet support setting the *enable_incremental_sort * flag
yet.
Was able to temporarily resolve by running the following though:

ALTER DATABASE corpdb SET enable_incremental_sort TO OFF;

Hope this helps others until the core issue has been resolved.





--
Sent from: https://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html

Re: 64-bit XIDs in deleted nbtree pages

[Peter Geoghegan]

On Sun, Feb 28, 2021 at 8:08 PM Masahiko Sawada  wrote:
> > Even though "the letter of the law" favors removing the
> > vacuum_cleanup_index_scale_factor GUC + param in the way I have
> > outlined, that is not the only thing that matters -- we must also
> > consider "the spirit of the law".

> > I suppose I could ask Tom what he thinks?
>
> +1

Are you going to start a new thread, or should I?

> Since it seems not a bug I personally think we don't need to do
> anything for back branches. But if we want not to trigger an index
> scan by vacuum_cleanup_index_scale_factor, we could change the default
> value to a high value (say, to 1) so that it can skip an index
> scan in most cases.

One reason to remove vacuum_cleanup_index_scale_factor in the back
branches is that it removes any need to fix the
"IndexVacuumInfo.num_heap_tuples is inaccurate outside of
btvacuumcleanup-only VACUUMs" bug -- it just won't matter if
btm_last_cleanup_num_heap_tuples is inaccurate anymore. (I am still
not sure about backpatch being a good idea, though.)

> > Another new concern for me (another concern unique to Postgres 13) is
> > autovacuum_vacuum_insert_scale_factor-driven autovacuums.
>
> IIUC the purpose of autovacuum_vacuum_insert_scale_factor is
> visibility map maintenance. And as per this discussion, it seems not
> necessary to do an index scan in btvacuumcleanup() triggered by
> autovacuum_vacuum_insert_scale_factor.

Arguably the question of skipping scanning the index should have been
considered by the autovacuum_vacuum_insert_scale_factor patch when it
was committed for Postgres 13 -- but it wasn't. There is a regression
that was tied to autovacuum_vacuum_insert_scale_factor in Postgres 13
by Mark Callaghan, which I suspect is relevant:

https://smalldatum.blogspot.com/2021/01/insert-benchmark-postgres-is-still.html

The blog post says: "Updates - To understand the small regression
mentioned above for the l.i1 test (more CPU & write IO) I repeated the
test with 100M rows using 2 configurations: one disabled index
deduplication and the other disabled insert-triggered autovacuum.
Disabling index deduplication had no effect and disabling
insert-triggered autovacuum resolves the regression."

This is quite specifically with an insert-only workload, with 4
indexes (that's from memory, but I'm pretty sure it's 4). I think that
the failure to account for skipping index scans is probably the big
problem here. Scanning the heap to set VM bits is unlikely to be
expensive compared to the full index scans. An insert-only workload is
going to find most of the heap blocks it scans to set VM bits in
shared_buffers. Not so for the indexes.

So in Postgres 13 we have this autovacuum_vacuum_insert_scale_factor
issue, in addition to the deduplication + btvacuumcleanup issue we
talked about (the problems left by my Postgres 13 bug fix commit
48e12913). These two issues make removing
vacuum_cleanup_index_scale_factor tempting, even in the back branches
-- it might actually be the more conservative approach, at least for
Postgres 13.

-- 
Peter Geoghegan

Re: new heapcheck contrib module

[Mark Dilger]

> On Mar 1, 2021, at 1:14 PM, Robert Haas  wrote:
> 
> On Wed, Feb 24, 2021 at 1:55 PM Mark Dilger
>  wrote:
>> [ new patches ]
> 
> Regarding 0001:
> 
> There seem to be whitespace-only changes to the comment for select_loop().
> 
> I wonder if the ParallelSlotsSetupOneDB()/ParallelSlotsSetupMinimal()
> changes could be simpler. First idea: Suppose you had
> ParallelSlotsSetup(numslots) that just creates the slot array with 0
> connections, and then ParallelSlotsAdopt(slots, conn, cparams) if you
> want to make it own an existing connection. That seems like it might
> be cleaner. Second idea: Why not get rid of ParallelSlotsSetupOneDB()
> altogether, and just let ParallelSlotsGetIdle() connect the other
> slots as required? Preconnecting all slots before we do anything is
> good because ... of what?

Mostly because, if --jobs is set too high, you get an error before launching 
any work.  I don't know that it's really a big deal if vacuumdb or reindexdb 
have a bunch of tasks kicked off prior to exit(1) due to not being able to open 
connections for all the slots, but it is a behavioral change.

> I also wonder if things might be simplified by introducing a wrapper
> object, e.g. ParallelSlotArray. Suppose a ParallelSlotArray stores the
> number of slots (num_slots), the array of actual PGconn objects, and
> the ConnParams to be used for new connections, and the initcmd to be
> used for new connections. Maybe also the progname. This seems like it
> would avoid a bunch of repeated parameter passing: you could just
> create the ParallelSlotArray with the right contents, and then pass it
> around everywhere, instead of having to keep passing the same stuff
> in. If you want to switch to connecting to a different DB, you tweak
> the ConnParams - maybe using an accessor function - and the system
> figures the rest out.

The existing version of parallel slots (before any of my changes) could already 
have been written that way, but the author chose not to.  I thought about 
making the sort of change you suggest, and decided against, mostly on the 
principle of stare decisis.  But the idea is very appealing, and since you're 
on board, I think I'll go make that change.

> I wonder if it's really useful to generalize this to a point of caring
> about all the ConnParams fields, too. Like, if you only provide
> ParallelSlotUpdateDB(slotarray, dbname), then that's the only field
> that can change so you don't need to care about the others. And maybe
> you also don't really need to keep the ConnParams fields in every
> slot, either. Like, couldn't you just say something like: if
> (strcmp(PQdb(conn) , slotarray->cparams->dbname) != 0) { wrong DB,
> can't reuse without a reconnect }? I know sometimes a dbname is really
> a whole connection string, but perhaps we could try to fix that by
> using PQconninfoParse() in the right place, so that what ends up in
> the cparams is just the db name, not a whole connection string.

I went a little out of my way to avoid that, as I didn't want the next 
application that uses parallel slots to have to refactor it again, if for 
example they want to process in parallel databases listening on different 
ports, or to process commands issued under different roles.

> This is just based on a relatively short amount of time spent studying
> the patch, so I might well be off-base here. What do you think?

I like the ParallelSlotArray idea, and will go do that now.  I'm happy to defer 
to your judgement on the other stuff, too, but will wait to hear back from you.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: enable_incremental_sort changes query behavior

[Tomas Vondra]

On 3/1/21 8:23 PM, rbrooks wrote:

Just a bump to see if there is a time frame for a fix on this.

Google Cloud doesn't yet support setting the *enable_incremental_sort * flag
yet.
Was able to temporarily resolve by running the following though:

ALTER DATABASE corpdb SET enable_incremental_sort TO OFF;

Hope this helps others until the core issue has been resolved.



I don't follow - all fixes in this thread were committed - that's why 
it's marked like that in the CF app. And thus it should be in the recent 
13.2 release.


I don't know what version is currently supported by Google Cloud, maybe 
they haven't upgraded their PostgreSQL version yet.


regards

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

Re: Table AM modifications to accept column projection lists

[Justin Pryzby]

On Thu, Dec 31, 2020 at 01:02:24PM -0800, Soumyadeep Chakraborty wrote:
> Hey Masahiko,
> 
> I added it to the Jan CF (https://commitfest.postgresql.org/31/2922/).
> 
> PFA a rebased version against latest head.

Thanks for working on this.

-   
pgstat_progress_update_param(PROGRESS_ANALYZE_CURRENT_CHILD_TABLE_RELID,

   
-
RelationGetRelid(childrel));
  
-   

   

Why is this removed ?

+* returningCols of it's base table's RTE.

its (possessive) not it's ("it is")

-- 
Justin

Re: Table AM modifications to accept column projection lists

[Jacob Champion]

On Mon, 2021-03-01 at 16:59 -0600, Justin Pryzby wrote:
> -   
> pgstat_progress_update_param(PROGRESS_ANALYZE_CURRENT_CHILD_TABLE_RELID,  
>   
>
> 
> -
> RelationGetRelid(childrel));  
> 
> - 
>   
>
> 
> Why is this removed ?

Mm, both of those analyze.c changes seem suspect. Possibly a mismerge
from the zedstore branch; let me double-check.

> 
> +* returningCols of it's base table's RTE.
> 
> its (possessive) not it's ("it is")

Thanks, I'll fix this at the same time.

--Jacob

Re: TRIM_ARRAY

[Dian Fay]

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:   tested, passed
Spec compliant:   tested, failed
Documentation:tested, failed

This basically does what it says, and the code looks good. The
documentation is out of alphabetical order (trim_array should appear
under cardinality rather than over)) but good otherwise. I was able to
"break" the function with an untyped null in psql:

select trim_array(null, 2);
ERROR:  could not determine polymorphic type because input has type unknown

I don't know whether there are any circumstances other than manual entry
in psql where this could happen, since column values and variables will
always be typed. I don't have access to the standard, but DB2's docs[1]
note "if any argument is null, the result is the null value", so an
up-front null check might be preferable to a slightly arcane user-facing
error, even if it's a silly invocation of a silly function :)

[1] 
https://www.ibm.com/support/knowledgecenter/en/SSEPEK_12.0.0/sqlref/src/tpc/db2z_bif_trimarray.html

The new status of this patch is: Waiting on Author

Re: 2019-03 CF now in progress

[Justin Pryzby]

Could I suggest to update the CF APP to allow:
| Target version: 15

Also, I wanted to suggest that toward the end of the devel cycle, that
committers set/unset target version to allow more focused review effort.
And so it's not a total surprise when something isn't progressed.
And as a simple way for a committer to mark an "intent to commit" so it's not a
surprise when something *is* committed.

Most of my patches are currently marked v14, but it'd be fine to kick them to
later.

-- 
Justin

Re: TRIM_ARRAY

[Vik Fearing]

On 3/2/21 12:14 AM, Dian Fay wrote:
> The following review has been posted through the commitfest application:
> make installcheck-world:  tested, passed
> Implements feature:   tested, passed
> Spec compliant:   tested, failed
> Documentation:tested, failed

Thank you for looking at my patch!

> This basically does what it says, and the code looks good. The
> documentation is out of alphabetical order (trim_array should appear
> under cardinality rather than over)) but good otherwise.

Hmm.  It appears between cardinality and unnest in the source code and
also my compiled html.  Can you say more about where you're seeing the
wrong order?

> I was able to
> "break" the function with an untyped null in psql:
> 
> select trim_array(null, 2);
> ERROR:  could not determine polymorphic type because input has type unknown
> 
> I don't know whether there are any circumstances other than manual entry
> in psql where this could happen, since column values and variables will
> always be typed. I don't have access to the standard, but DB2's docs[1]
> note "if any argument is null, the result is the null value", so an
> up-front null check might be preferable to a slightly arcane user-facing
> error, even if it's a silly invocation of a silly function :)
> 
> [1] 
> https://www.ibm.com/support/knowledgecenter/en/SSEPEK_12.0.0/sqlref/src/tpc/db2z_bif_trimarray.html

The standard also says that if either argument is null, the result is
null.  The problem here is that postgres needs to know what the return
type is and it can only determine that from the input.

If you give the function a typed null, it returns null as expected.

> The new status of this patch is: Waiting on Author

I put it back to Needs Review without a new patch because I don't know
what I would change.
-- 
Vik Fearing

Re: TRIM_ARRAY

[Tom Lane]

Dian Fay  writes:
> This basically does what it says, and the code looks good. The
> documentation is out of alphabetical order (trim_array should appear
> under cardinality rather than over)) but good otherwise. I was able to
> "break" the function with an untyped null in psql:

> select trim_array(null, 2);
> ERROR:  could not determine polymorphic type because input has type unknown

That's a generic parser behavior for polymorphic functions, not something
this particular function could or should dodge.

regards, tom lane

Re: [PATCH] regexp_positions ( string text, pattern text, flags text ) → setof int4range[]

[Mark Dilger]

> On Mar 1, 2021, at 9:38 AM, Joel Jacobson  wrote:
> 
> Forgive me for just sending a patch without much discussion on the list,
> but it was so easy to implement, so I thought an implementation can
> help the discussion on if this is something we want or not.

I like the idea so I did a bit of testing.  I think the following should not 
error, but does:

+SELECT regexp_positions('foObARbEqUEbAz', $re$(?=beque)$re$, 'i');
+ERROR:  range lower bound must be less than or equal to range upper bound


—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [PATCH] Bug fix in initdb output

[Juan José Santamaría Flecha]

On Mon, Mar 1, 2021 at 10:18 PM Alvaro Herrera 
wrote:

> On 2021-Mar-01, Juan José Santamaría Flecha wrote:
>
> > Uhm, now that you point it out, an absolute path would make the message
> > more consistent and reusable.
>
> Well.  This code was introduced in a00c58314745, with discussion at
>
> http://postgr.es/m/CAHeEsBeAe1FeBypT3E8R1ZVZU0e8xv3A-7BHg6bEOi=jzny...@mail.gmail.com
> which did not touch on the point of the pg_ctl path being relative or
> absolute.  The previous decision to use relative seems to have been made
> here in commit ee814b4511ec, which was backed by this discussion
>
> https://www.postgresql.org/message-id/flat/200411020134.52513.peter_e%40gmx.net
>
> So I'm not sure that anybody would love me if I change it again to
> absolute.
>

For me it is a +1 for the change to absolute. Let's see if more people want
to weigh in on the matter.

Regards,

Juan José Santamaría Flecha

Re: Why does the BF sometimes not dump regression.diffs?

[Andrew Dunstan]

On 3/1/21 4:05 PM, Thomas Munro wrote:
> Hello,
>
> I saw this failure after a recent commit (though the next build
> succeeded, and I don't yet have any particular reason to believe that
> the commits it blamed are at fault, we'll see...):
>
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=gombessa&dt=2021-03-01%2004%3A58%3A17
>
> Strangely, it didn't spit out the regression.diffs so I don't know
> anything more than "commit_ts failed".  I emailed the owner asking for
> that log but I see now it's too late.  I wonder if there is some
> scripting in there that doesn't work correctly on OpenBSD for whatever
> reason...  I know that we *sometimes* get regression.diffs from
> failures in here, but I haven't tried to figure out the pattern (which
> animals etc).  Here's a similar failure that does show the .diffs I
> wish I could see, same BF client version (11), on Windows:
>
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=fairywren&dt=2020-07-13%2000:47:47
>
>


The version numbering is a bit misleading on fairywren, which, as it's a
machine I control runs from a git checkout, which clearly is later than
REL_11 even though that's what the version string says. Commit 13d2143
fixed this. It was included in Release 12.


cheers


andrew



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

Re: Why does the BF sometimes not dump regression.diffs?

[Thomas Munro]

On Tue, Mar 2, 2021 at 1:32 PM Andrew Dunstan  wrote:
> The version numbering is a bit misleading on fairywren, which, as it's a
> machine I control runs from a git checkout, which clearly is later than
> REL_11 even though that's what the version string says. Commit 13d2143
> fixed this. It was included in Release 12.

Oh, thanks!

Re: TRIM_ARRAY

[Dian M Fay]

On Mon Mar 1, 2021 at 6:53 PM EST, Vik Fearing wrote:
> > This basically does what it says, and the code looks good. The
> > documentation is out of alphabetical order (trim_array should appear
> > under cardinality rather than over)) but good otherwise.
>
> Hmm. It appears between cardinality and unnest in the source code and
> also my compiled html. Can you say more about where you're seeing the
> wrong order?

I applied the patch to the latest commit, ffd3944ab9. Table 9.52 is
ordered:

array_to_string
array_upper
trim_array
cardinality
unnest

> The problem here is that postgres needs to know what the return
> type is and it can only determine that from the input.
>
> If you give the function a typed null, it returns null as expected.
>
> > The new status of this patch is: Waiting on Author
>
> I put it back to Needs Review without a new patch because I don't know
> what I would change.

I'd thought that checking v and returning null instead of raising the
error would be more friendly, should it be possible to pass an untyped
null accidentally instead of on purpose, and I couldn't rule that out.
I've got no objections other than the docs having been displaced.

Re: repeated decoding of prepared transactions

[Ajin Cherian]

On Mon, Mar 1, 2021 at 8:08 PM Amit Kapila  wrote:

> Few minor comments on 0002 patch
> =
> 1.
>   ctx->streaming &= enable_streaming;
> - ctx->twophase &= enable_twophase;
> +
>  }
>
> Spurious line addition.

Deleted.

>
> 2.
> -  proallargtypes =>
> '{name,name,text,oid,bool,bool,int4,xid,xid,pg_lsn,pg_lsn,text,int8}',
> -  proargmodes => '{o,o,o,o,o,o,o,o,o,o,o,o,o}',
> -  proargnames =>
> '{slot_name,plugin,slot_type,datoid,temporary,active,active_pid,xmin,catalog_xmin,restart_lsn,confirmed_flush_lsn,wal_status,safe_wal_size}',
> +  proallargtypes =>
> '{name,name,text,oid,bool,bool,int4,xid,xid,pg_lsn,pg_lsn,text,int8,bool}',
> +  proargmodes => '{o,o,o,o,o,o,o,o,o,o,o,o,o,o}',
> +  proargnames =>
> '{slot_name,plugin,slot_type,datoid,temporary,active,active_pid,xmin,catalog_xmin,restart_lsn,confirmed_flush_lsn,wal_status,safe_wal_size,twophase}',
>prosrc => 'pg_get_replication_slots' },
>  { oid => '3786', descr => 'set up a logical replication slot',
>proname => 'pg_create_logical_replication_slot', provolatile => 'v',
> -  proparallel => 'u', prorettype => 'record', proargtypes => 'name name 
> bool',
> -  proallargtypes => '{name,name,bool,name,pg_lsn}',
> -  proargmodes => '{i,i,i,o,o}',
> -  proargnames => '{slot_name,plugin,temporary,slot_name,lsn}',
> +  proparallel => 'u', prorettype => 'record', proargtypes => 'name
> name bool bool',
> +  proallargtypes => '{name,name,bool,bool,name,pg_lsn}',
> +  proargmodes => '{i,i,i,i,o,o}',
> +  proargnames => '{slot_name,plugin,temporary,twophase,slot_name,lsn}',
>
> I think it is better to use two_phase here and at other places as well
> to be consistent with similar parameters.

Updated as requested.
>
> 3.
> --- a/src/backend/catalog/system_views.sql
> +++ b/src/backend/catalog/system_views.sql
> @@ -894,7 +894,8 @@ CREATE VIEW pg_replication_slots AS
>  L.restart_lsn,
>  L.confirmed_flush_lsn,
>  L.wal_status,
> -L.safe_wal_size
> +L.safe_wal_size,
> + L.twophase
>  FROM pg_get_replication_slots() AS L
>
> Indentation issue. Here, you need you spaces instead of tabs.

Updated.
>
> 4.
> @@ -533,6 +533,12 @@ CreateDecodingContext(XLogRecPtr start_lsn,
>
>   ctx->reorder->output_rewrites = ctx->options.receive_rewrites;
>
> + /*
> + * If twophase is set on the slot at create time, then
> + * make sure the field in the context is also updated.
> + */
> + ctx->twophase &= MyReplicationSlot->data.twophase;
> +
>
> Why didn't you made similar change in CreateInitDecodingContext when I
> already suggested the same in my previous email? If we don't make that
> change then during slot initialization two_phase will always be true
> even though user passed in as false. It looks inconsistent and even
> though there is no direct problem due to that but it could be cause of
> possible problem in future.

Updated.

regards,
Ajin Cherian
Fujitsu Australia


v8-0001-Add-option-to-enable-two-phase-commits-in-pg_crea.patch
Description: Binary data

Re: PostmasterIsAlive() in recovery (non-USE_POST_MASTER_DEATH_SIGNAL builds)

[Thomas Munro]

On Tue, Mar 2, 2021 at 12:00 AM Thomas Munro  wrote:
> On Mon, Nov 16, 2020 at 8:56 PM Michael Paquier  wrote:
> > No objections with the two changes from pg_usleep() to WaitLatch() so
> > they could be applied separately first.
>
> I thought about committing that first part, and got as far as
> splitting the patch into two (see attached), but then I re-read
> Fujii-san's message about the speed of promotion and realised that we
> really should have something like a condition variable for walRcvState
> changes.  I'll look into that.

Here's an experimental attempt at that, though I'm not sure if it's
the right approach.  Of course it's not necessary to use condition
variables here: we could use recoveryWakeupLatch, because we're not in
any doubt about who needs to be woken up.  But then you could get
constant wakeups while recovery is paused, unless you also suppressed
that somehow.  You could use the startup process's procLatch,
advertised in shmem, but that's almost a condition variable.  With a
condition variable, you get to name it something like
walRcvStateChanged, and then the programming rule is very clear: if
you change walRcvState, you need to broadcast that fact (and you don't
have to worry about who might be interested).  One question I haven't
got to the bottom of: is it a problem for the startup process that CVs
use CHECK_FOR_INTERRUPTS()?
From 1e28b9c21a953e66c48f78a90192f3a0ca83d0aa Mon Sep 17 00:00:00 2001
From: Thomas Munro 
Date: Tue, 2 Mar 2021 11:08:06 +1300
Subject: [PATCH v6 1/3] Add condition variable for walreceiver state.

Use this new CV to wait for walreceiver shutdown without a sleep/poll
loop, while also benefiting from standard postmaster death handling.

Reviewed-by: Heikki Linnakangas 
Reviewed-by: Fujii Masao 
Reviewed-by: Michael Paquier 
Discussion: https://postgr.es/m/CA%2BhUKGK1607VmtrDUHQXrsooU%3Dap4g4R2yaoByWOOA3m8xevUQ%40mail.gmail.com
---
 doc/src/sgml/monitoring.sgml   |  4 +++
 src/backend/postmaster/pgstat.c|  3 ++
 src/backend/replication/walreceiver.c  | 10 ++
 src/backend/replication/walreceiverfuncs.c | 42 ++
 src/include/pgstat.h   |  1 +
 src/include/replication/walreceiver.h  |  2 ++
 6 files changed, 48 insertions(+), 14 deletions(-)

diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 3513e127b7..cf00210cb3 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -1757,6 +1757,10 @@ postgres   27093  0.0  0.0  30096  2752 ?Ss   11:34   0:00 postgres: ser
   Waiting for confirmation from a remote server during synchronous
replication.
  
+ 
+  WalrcvExit
+  Waiting for the walreceiver to exit.
+ 
  
   XactGroupUpdate
   Waiting for the group leader to update transaction status at
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index f75b52719d..2dbfb81e40 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -4122,6 +4122,9 @@ pgstat_get_wait_ipc(WaitEventIPC w)
 		case WAIT_EVENT_SYNC_REP:
 			event_name = "SyncRep";
 			break;
+		case WAIT_EVENT_WALRCV_EXIT:
+			event_name = "WalrcvExit";
+			break;
 		case WAIT_EVENT_XACT_GROUP_UPDATE:
 			event_name = "XactGroupUpdate";
 			break;
diff --git a/src/backend/replication/walreceiver.c b/src/backend/replication/walreceiver.c
index e5f8a06fea..397a94d7af 100644
--- a/src/backend/replication/walreceiver.c
+++ b/src/backend/replication/walreceiver.c
@@ -207,6 +207,8 @@ WalReceiverMain(void)
 
 		case WALRCV_STOPPED:
 			SpinLockRelease(&walrcv->mutex);
+			/* We might have changed state and fallen through above. */
+			ConditionVariableBroadcast(&walrcv->walRcvStateChanged);
 			proc_exit(1);
 			break;
 
@@ -249,6 +251,8 @@ WalReceiverMain(void)
 
 	SpinLockRelease(&walrcv->mutex);
 
+	ConditionVariableBroadcast(&walrcv->walRcvStateChanged);
+
 	pg_atomic_write_u64(&WalRcv->writtenUpto, 0);
 
 	/* Arrange to clean up at walreceiver exit */
@@ -647,6 +651,8 @@ WalRcvWaitForStartPosition(XLogRecPtr *startpoint, TimeLineID *startpointTLI)
 	walrcv->receiveStartTLI = 0;
 	SpinLockRelease(&walrcv->mutex);
 
+	ConditionVariableBroadcast(&walrcv->walRcvStateChanged);
+
 	set_ps_display("idle");
 
 	/*
@@ -675,6 +681,8 @@ WalRcvWaitForStartPosition(XLogRecPtr *startpoint, TimeLineID *startpointTLI)
 			*startpointTLI = walrcv->receiveStartTLI;
 			walrcv->walRcvState = WALRCV_STREAMING;
 			SpinLockRelease(&walrcv->mutex);
+
+			ConditionVariableBroadcast(&walrcv->walRcvStateChanged);
 			break;
 		}
 		if (walrcv->walRcvState == WALRCV_STOPPING)
@@ -784,6 +792,8 @@ WalRcvDie(int code, Datum arg)
 	walrcv->latch = NULL;
 	SpinLockRelease(&walrcv->mutex);
 
+	ConditionVariableBroadcast(&walrcv->walRcvStateChanged);
+
 	/* Terminate the connection gracefully. */
 	if (wrconn != NULL)
 		walrcv_disconnect(wrconn);
diff --git a/src/backend/replication/walreceiverfuncs.c b/src/backen

Re: [PATCH] Bug fix in initdb output

[Michael Paquier]

On Tue, Mar 02, 2021 at 01:28:57AM +0100, Juan José Santamaría Flecha wrote:
> For me it is a +1 for the change to absolute. Let's see if more people want
> to weigh in on the matter.

FWIW, I don't think that it is a good idea to come back to this
decision for *nix platforms, so I would let it as-is, and use relative
paths if initdb is called using a relative path.

The number of people using a relative path for Windows initialization
sounds rather limited to me.  However, if you can write something that
makes the path printed compatible for a WIN32 terminal while keeping
the behavior consistent with other platforms, people would have no
objections to that.
--
Michael


signature.asc
Description: PGP signature

Re: archive_command / pg_stat_archiver & documentation

[Michael Paquier]

On Mon, Mar 01, 2021 at 05:17:06PM +0800, Julien Rouhaud wrote:
> Maybe this can be better addressed than with a link in the
> documentation.  The final outcome is that it can be difficult to
> monitor the archiver state in such case.  That's orthogonal to this
> patch but maybe we can add a new "archiver_start" timestamptz column
> in pg_stat_archiver, so monitoring tools can detect a problem if it's
> too far away from pg_postmaster_start_time() for instance?

There may be other solutions as well.  I have applied the doc patch
for now.
--
Michael


signature.asc
Description: PGP signature

Re: TRIM_ARRAY

[Vik Fearing]

On 3/2/21 1:02 AM, Dian M Fay wrote:
> On Mon Mar 1, 2021 at 6:53 PM EST, Vik Fearing wrote:
>>> This basically does what it says, and the code looks good. The
>>> documentation is out of alphabetical order (trim_array should appear
>>> under cardinality rather than over)) but good otherwise.
>>
>> Hmm. It appears between cardinality and unnest in the source code and
>> also my compiled html. Can you say more about where you're seeing the
>> wrong order?
> 
> I applied the patch to the latest commit, ffd3944ab9. Table 9.52 is
> ordered:
> 
> array_to_string
> array_upper
> trim_array
> cardinality
> unnest

So it turns out I must have fixed it locally after I posted the patch
and then forgot I did that.  Attached is a new patch with the order
correct.  Thanks for spotting it!

>> The problem here is that postgres needs to know what the return
>> type is and it can only determine that from the input.
>>
>> If you give the function a typed null, it returns null as expected.
>>
>>> The new status of this patch is: Waiting on Author
>>
>> I put it back to Needs Review without a new patch because I don't know
>> what I would change.
> 
> I'd thought that checking v and returning null instead of raising the
> error would be more friendly, should it be possible to pass an untyped
> null accidentally instead of on purpose, and I couldn't rule that out.

As Tom said, that is something that does not belong in this patch.
-- 
Vik Fearing
>From 351bf14400d8ea2948788a96b47ce6c20847de97 Mon Sep 17 00:00:00 2001
From: Vik Fearing 
Date: Tue, 16 Feb 2021 18:38:24 +0100
Subject: [PATCH] implement trim_array

---
 doc/src/sgml/func.sgml   | 18 
 src/backend/catalog/sql_features.txt |  2 +-
 src/backend/utils/adt/arrayfuncs.c   | 42 
 src/include/catalog/pg_proc.dat  |  5 
 src/test/regress/expected/arrays.out | 23 +++
 src/test/regress/sql/arrays.sql  | 19 +
 6 files changed, 108 insertions(+), 1 deletion(-)

diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 08f08322ca..2aac87cf8d 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -17909,6 +17909,24 @@ SELECT NULLIF(value, '(none)') ...

   
 
+  
+   
+
+ trim_array
+
+trim_array ( array anyarray, n integer )
+anyarray
+   
+   
+Trims an array by removing the last n elements.
+If the array is multidimensional, only the first dimension is trimmed.
+   
+   
+trim_array(ARRAY[1,2,3,4,5,6], 2)
+{1,2,3,4}
+   
+  
+
   

 
diff --git a/src/backend/catalog/sql_features.txt b/src/backend/catalog/sql_features.txt
index ab0895ce3c..32eed988ab 100644
--- a/src/backend/catalog/sql_features.txt
+++ b/src/backend/catalog/sql_features.txt
@@ -398,7 +398,7 @@ S301	Enhanced UNNEST			YES
 S401	Distinct types based on array types			NO	
 S402	Distinct types based on distinct types			NO	
 S403	ARRAY_MAX_CARDINALITY			NO	
-S404	TRIM_ARRAY			NO	
+S404	TRIM_ARRAY			YES	
 T011	Timestamp in Information Schema			NO	
 T021	BINARY and VARBINARY data types			NO	
 T022	Advanced support for BINARY and VARBINARY data types			NO	
diff --git a/src/backend/utils/adt/arrayfuncs.c b/src/backend/utils/adt/arrayfuncs.c
index f7012cc5d9..d38a99f0b0 100644
--- a/src/backend/utils/adt/arrayfuncs.c
+++ b/src/backend/utils/adt/arrayfuncs.c
@@ -6631,3 +6631,45 @@ width_bucket_array_variable(Datum operand,
 
 	return left;
 }
+
+/*
+ * Trim the right N elements from an array by calculating an appropriate slice.
+ * Only the first dimension is trimmed.
+ */
+Datum
+trim_array(PG_FUNCTION_ARGS)
+{
+	ArrayType  *v = PG_GETARG_ARRAYTYPE_P(0);
+	int			n = PG_GETARG_INT32(1);
+	int			array_length = ARR_DIMS(v)[0];
+	int16		elmlen;
+	bool		elmbyval;
+	char		elmalign;
+	int			lower[MAXDIM];
+	int			upper[MAXDIM];
+	bool		lowerProvided[MAXDIM];
+	bool		upperProvided[MAXDIM];
+	Datum		result;
+
+	/* Throw an error if out of bounds */
+	if (n < 0 || n > array_length)
+		ereport(ERROR,
+(errcode(ERRCODE_ARRAY_ELEMENT_ERROR),
+ errmsg("number of elements to trim must be between 0 and %d", array_length)));
+
+	/* Set all the bounds as unprovided except the first upper bound */
+	memset(lowerProvided, 0, sizeof(lowerProvided));
+	memset(upperProvided, 0, sizeof(upperProvided));
+	upper[0] = ARR_LBOUND(v)[0] + array_length - n - 1;
+	upperProvided[0] = true;
+
+	/* Fetch the needed information about the element type */
+	get_typlenbyvalalign(ARR_ELEMTYPE(v), &elmlen, &elmbyval, &elmalign);
+
+	/* Get the slice */
+	result = array_get_slice(PointerGetDatum(v), 1,
+			 upper, lower, upperProvided, lowerProvided,
+			 -1, elmlen, elmbyval, elmalign);
+
+	PG_RETURN_DATUM(result);
+}
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 1487710d59..8ab911238d 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.

Re: [BUG] Autovacuum not dynamically decreasing cost_limit and cost_delay

[Masahiko Sawada]

On Mon, Feb 8, 2021 at 11:49 PM Mead, Scott  wrote:
>
> Hello,
>I recently looked at what it would take to make a running autovacuum 
> pick-up a change to either cost_delay or cost_limit.  Users frequently will 
> have a conservative value set, and then wish to change it when autovacuum 
> initiates a freeze on a relation.  Most users end up finding out they are in 
> ‘to prevent wraparound’ after it has happened, this means that if they want 
> the vacuum to take advantage of more I/O, they need to stop and then restart 
> the currently running vacuum (after reloading the GUCs).
>
>   Initially, my goal was to determine feasibility for making this dynamic.  I 
> added debug code to vacuum.c:vacuum_delay_point(void) and found that changes 
> to cost_delay and cost_limit are already processed by a running vacuum.  
> There was a bug preventing the cost_delay or cost_limit from being configured 
> to allow higher throughput however.
>
> I believe this is a bug because currently, autovacuum will dynamically detect 
> and increase the cost_limit or cost_delay, but it can never decrease those 
> values beyond their setting when the vacuum began.  The current behavior is 
> for vacuum to limit the maximum throughput of currently running vacuum 
> processes to the cost_limit that was set when the vacuum process began.

Thanks for your report.

I've not looked at the patch yet but I agree that the calculation for
autovacuum cost delay seems not to work fine if vacuum-delay-related
parameters (e.g., autovacuum_vacuum_cost_delay etc) are changed during
vacuuming a table to speed up running autovacuums. Here is my
analysis:

Suppose we have the following parameters and 3 autovacuum workers are
running on different tables:

autovacuum_vacuum_cost_delay = 100
autovacuum_vacuum_cost_limit = 100

Vacuum cost-based delay parameters for each workers are follows:

worker->wi_cost_limit_base = 100
worker->wi_cost_limit = 66
worker->wi_cost_delay = 100

Each running autovacuum has "wi_cost_limit = 66" because the total
limit (100) is equally rationed. And another point is that the total
wi_cost_limit (198 = 66*3) is less than autovacuum_vacuum_cost_limit,
100. Which are fine.

Here let's change autovacuum_vacuum_cost_delay/limit value to speed up
running autovacuums.

Case 1 : increasing autovacuum_vacuum_cost_limit to 1000.

After reloading the configuration file, vacuum cost-based delay
parameters for each worker become as follows:

worker->wi_cost_limit_base = 100
worker->wi_cost_limit = 100
worker->wi_cost_delay = 100

If we rationed autovacuum_vacuum_cost_limit, 1000, to 3 workers, it
would be 333. But since we cap it by wi_cost_limit_base, the
wi_cost_limit is 100. I think this is what Mead reported here.

Case 2 : decreasing autovacuum_vacuum_cost_delay to 10.

After reloading the configuration file, vacuum cost-based delay
parameters for each workers become as follows:

worker->wi_cost_limit_base = 100
worker->wi_cost_limit = 100
worker->wi_cost_delay = 100

Actually, the result is the same as case 1. But In this case, the
total cost among the three workers is 300, which is greater than
autovacuum_vacuum_cost_limit, 100. This behavior violates what the
documentation explains in the description of
autovacuum_vacuum_cost_limit:

---
Note that the value is distributed proportionally among the running
autovacuum workers, if there is more than one, so that the sum of the
limits for each worker does not exceed the value of this variable.
---

It seems to me that those problems come from the fact that we don't
change both wi_cost_limit_base and wi_cost_delay during auto-vacuuming
a table in spite of using autovacuum_vac_cost_limit/delay to calculate
cost_avail. Such a wrong calculation happens until all running
autovacuum workers finish the current vacuums. When a worker starts to
process a new table, it resets both wi_cost_limit_base and
wi_cost_delay.

Looking at autovac_balance_cost(), it considers worker's
wi_cost_limit_base to calculate the total base cost limit of
participating active workers as follows:

cost_total +=
(double) worker->wi_cost_limit_base / worker->wi_cost_delay;

But what is the point of calculating it while assuming each worker
having a different cost limit? Since workers vacuuming on a table
whose cost parameters are set individually doesn't participate in this
calculation (by commit 1021bd6a8 in 2014), having at_dobalance true, I
wonder if we can just assume all workers have the same cost_limit and
cost_delay except for workers setting at_dobalance true. If we can do
that, I guess we no longer need wi_cost_limit_base.

Also, we don't change wi_cost_delay during vacuuming a table, which
seems wrong to me. autovac_balance_cost() can change workers'
wi_cost_delay, eventually applying to VacuumCostDelay.

What do you think?

Regards,

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

Re: doc review for v14

[Michael Paquier]

On Mon, Mar 01, 2021 at 03:17:40PM +0900, Michael Paquier wrote:
> You could say here "Checksums can be enabled", but "normally" does not
> sound bad to me either as it insists on the fact that it is better to
> do that when the cluster is initdb'd as this has no downtime compared
> to enabling checksums on an existing cluster.

I looked at that again this morning, and the last version sent
upthread still looked fine to me, so I have just applied that.

Thanks for caring about that, Justin.
--
Michael


signature.asc
Description: PGP signature

Re: simplifying foreign key/RI checks

[Amit Langote]

On Mon, Mar 1, 2021 at 3:14 PM Corey Huinker  wrote:
>> > It seems to me 1 (RI_PLAN_CHECK_LOOKUPPK) is still alive. (Yeah, I
>> > know that doesn't mean the usefulness of the macro but the mechanism
>> > the macro suggests, but it is confusing.) On the other hand,
>> > RI_PLAN_CHECK_LOOKUPPK_FROM_PK and RI_PLAN_LAST_ON_PK seem to be no
>> > longer used.  (Couldn't we remove them?)
>>
>> Yeah, better to just remove those _PK macros and say this module no
>> longer runs any queries on the PK table.
>>
>> How about the attached?
>>
>
> Sorry for the delay.
> I see that the changes were made as described.
> Passes make check and make check-world yet again.
> I'm marking this Ready For Committer unless someone objects.

Thank you Corey for the review.

-- 
Amit Langote
EDB: http://www.enterprisedb.com

Re: A reloption for partitioned tables - parallel_workers

[Amit Langote]

On Tue, Mar 2, 2021 at 12:10 AM Laurenz Albe  wrote:
> Here is an updated patch with this fix.

Thanks for updating the patch.  I was about to post an updated version
myself but you beat me to it.

> I added regression tests and adapted the documentation a bit.
>
> I also added support for lowering the number of parallel workers.

+ALTER TABLE pagg_tab_ml SET (parallel_workers = 0);
+EXPLAIN (COSTS OFF)
+SELECT a FROM pagg_tab_ml WHERE b = 42;
+QUERY PLAN
+---
+ Append
+   ->  Seq Scan on pagg_tab_ml_p1 pagg_tab_ml_1
+ Filter: (b = 42)
+   ->  Seq Scan on pagg_tab_ml_p2_s1 pagg_tab_ml_2
+ Filter: (b = 42)
+   ->  Seq Scan on pagg_tab_ml_p2_s2 pagg_tab_ml_3
+ Filter: (b = 42)
+(7 rows)

I got the same result with my implementation, but I am wondering if
setting parallel_workers=0 on the parent table shouldn't really
disable a regular (non-parallel-aware) Append running under Gather
even if it does Parallel Append (parallel-aware)?  So in this test
case, there should have been a Gather atop Append, with individual
partitions scanned using Parallel Seq Scan where applicable.


--
Amit Langote
EDB: http://www.enterprisedb.com

Re: We should stop telling users to "vacuum that database in single-user mode"

[Noah Misch]

On Mon, Mar 01, 2021 at 04:32:23PM +0100, Hannu Krosing wrote:
> It looks like we are unnecessarily instructing our usiers to vacuum their
> databases in single-user mode when just vacuuming would be enough.

> When I started looking at improving the situation I discovered, that there
> already is no need to run VACUUM in single user mode in any currently 
> supported
> PostgreSQL version as you can run VACUUM perfectly well when the wraparound
> protection is active.

A comment in SetTransactionIdLimit() says, "VACUUM requires an XID if it
truncates at wal_level!=minimal."  Hence, I think plain VACUUM will fail some
of the time; VACUUM (TRUNCATE false) should be reliable.  In general, I like
your goal of replacing painful error message advice with less-painful advice.
At the same time, it's valuable for the advice to reliably get the user out of
the bad situation.

Re: repeated decoding of prepared transactions

[vignesh C]

On Tue, Mar 2, 2021 at 6:37 AM Ajin Cherian  wrote:
>
> On Mon, Mar 1, 2021 at 8:08 PM Amit Kapila  wrote:
>
> > Few minor comments on 0002 patch
> > =
> > 1.
> >   ctx->streaming &= enable_streaming;
> > - ctx->twophase &= enable_twophase;
> > +
> >  }
> >
> > Spurious line addition.
>
> Deleted.
>
> >
> > 2.
> > -  proallargtypes =>
> > '{name,name,text,oid,bool,bool,int4,xid,xid,pg_lsn,pg_lsn,text,int8}',
> > -  proargmodes => '{o,o,o,o,o,o,o,o,o,o,o,o,o}',
> > -  proargnames =>
> > '{slot_name,plugin,slot_type,datoid,temporary,active,active_pid,xmin,catalog_xmin,restart_lsn,confirmed_flush_lsn,wal_status,safe_wal_size}',
> > +  proallargtypes =>
> > '{name,name,text,oid,bool,bool,int4,xid,xid,pg_lsn,pg_lsn,text,int8,bool}',
> > +  proargmodes => '{o,o,o,o,o,o,o,o,o,o,o,o,o,o}',
> > +  proargnames =>
> > '{slot_name,plugin,slot_type,datoid,temporary,active,active_pid,xmin,catalog_xmin,restart_lsn,confirmed_flush_lsn,wal_status,safe_wal_size,twophase}',
> >prosrc => 'pg_get_replication_slots' },
> >  { oid => '3786', descr => 'set up a logical replication slot',
> >proname => 'pg_create_logical_replication_slot', provolatile => 'v',
> > -  proparallel => 'u', prorettype => 'record', proargtypes => 'name name 
> > bool',
> > -  proallargtypes => '{name,name,bool,name,pg_lsn}',
> > -  proargmodes => '{i,i,i,o,o}',
> > -  proargnames => '{slot_name,plugin,temporary,slot_name,lsn}',
> > +  proparallel => 'u', prorettype => 'record', proargtypes => 'name
> > name bool bool',
> > +  proallargtypes => '{name,name,bool,bool,name,pg_lsn}',
> > +  proargmodes => '{i,i,i,i,o,o}',
> > +  proargnames => '{slot_name,plugin,temporary,twophase,slot_name,lsn}',
> >
> > I think it is better to use two_phase here and at other places as well
> > to be consistent with similar parameters.
>
> Updated as requested.
> >
> > 3.
> > --- a/src/backend/catalog/system_views.sql
> > +++ b/src/backend/catalog/system_views.sql
> > @@ -894,7 +894,8 @@ CREATE VIEW pg_replication_slots AS
> >  L.restart_lsn,
> >  L.confirmed_flush_lsn,
> >  L.wal_status,
> > -L.safe_wal_size
> > +L.safe_wal_size,
> > + L.twophase
> >  FROM pg_get_replication_slots() AS L
> >
> > Indentation issue. Here, you need you spaces instead of tabs.
>
> Updated.
> >
> > 4.
> > @@ -533,6 +533,12 @@ CreateDecodingContext(XLogRecPtr start_lsn,
> >
> >   ctx->reorder->output_rewrites = ctx->options.receive_rewrites;
> >
> > + /*
> > + * If twophase is set on the slot at create time, then
> > + * make sure the field in the context is also updated.
> > + */
> > + ctx->twophase &= MyReplicationSlot->data.twophase;
> > +
> >
> > Why didn't you made similar change in CreateInitDecodingContext when I
> > already suggested the same in my previous email? If we don't make that
> > change then during slot initialization two_phase will always be true
> > even though user passed in as false. It looks inconsistent and even
> > though there is no direct problem due to that but it could be cause of
> > possible problem in future.
>
> Updated.
>

I have a minor comment regarding the below:
+ 
+  
+   two_phase bool
+  
+  
+  True if two-phase commits are enabled on this slot.
+  
+ 

Can we change something like:
True if the slot is enabled for decoding prepared transaction
information. Refer link for more information.(link should point where
more detailed information is available for two-phase in
pg_create_logical_replication_slot).

Also there is one small indentation in that line, I think there should
be one space before "True if".

Regards,
Vignesh

Re: [PATCH] pgbench: Bug fix for the -d option

[miyake_kouta]

2021-02-26 20:30, Michael Paquier wrote:

By the way, I can help but wonder why pgbench has such a different
handling for the user name, fetching first PGUSER and then looking at
the options while most of the other binaries use get_user_name().  It
seems to me that we could simplify the handling around "login" without
really impacting the usefulness of the tool, no?


Hi.

Thank you for your comment.
I modified the patch based on other binaries.
In this new patch, if there is a $PGUSER, then it's set to login.
If not, get_user_name_or_exit is excuted.
Plese let me know what you think about this change.
--
Kota Miyakediff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 31a4df45f5..b20c7b5b27 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -32,6 +32,7 @@
 #endif
 
 #include "postgres_fe.h"
+#include "common/username.h"
 
 #include 
 #include 
@@ -5487,8 +5488,10 @@ main(int argc, char **argv)
 		pghost = env;
 	if ((env = getenv("PGPORT")) != NULL && *env != '\0')
 		pgport = env;
-	else if ((env = getenv("PGUSER")) != NULL && *env != '\0')
+	if ((env = getenv("PGUSER")) != NULL && *env != '\0')
 		login = env;
+	else
+		login = get_user_name_or_exit(progname);
 
 	state = (CState *) pg_malloc0(sizeof(CState));
 

[PATCH] psql : Improve code for help option

[miyake_kouta]

Hi.

I found some redundant code in psql/help.c, so I propose a patch to fix 
it.
In the current help.c, the variable "user" is set to the value of 
$PGUSER (or get_user_name).

However, $PGUSER is referenced again in the code that follows.
We can replace this part with "user", I think.

Regards.
--
Kota Miyakediff --git a/src/bin/psql/help.c b/src/bin/psql/help.c
index e44120bf76..75a2217cac 100644
--- a/src/bin/psql/help.c
+++ b/src/bin/psql/help.c
@@ -134,9 +134,7 @@ usage(unsigned short int pager)
 	fprintf(output, _("  -p, --port=PORT  database server port (default: \"%s\")\n"),
 			env ? env : DEF_PGPORT_STR);
 	/* Display default user */
-	env = getenv("PGUSER");
-	if (!env)
-		env = user;
+	env = user;
 	fprintf(output, _("  -U, --username=USERNAME  database user name (default: \"%s\")\n"), env);
 	fprintf(output, _("  -w, --no-passwordnever prompt for password\n"));
 	fprintf(output, _("  -W, --password   force password prompt (should happen automatically)\n"));

Re: New IndexAM API controlling index vacuum strategies

[Peter Geoghegan]

On Sun, Feb 21, 2021 at 10:28 PM Masahiko Sawada  wrote:
> Sorry for the late response.

Me too!

> 1. Whereas skipping index vacuum and heap vacuum is a very attractive
> improvement, if we skip that by default I wonder if we need a way to
> disable it. Vacuum plays a role in cleaning and diagnosing tables in
> practice. So in a case where the table is bad state and the user wants
> to clean all heap pages, it would be good to have a way to disable
> this skipping behavior. One solution would be that index_cleanup
> option has three different behaviors: on, auto (or smart), and off. We
> enable this skipping behavior by default in ‘auto’ mode, but
> specifying "INDEX_CLEANUP true” means to enforce index vacuum and
> therefore disabling it.

Sounds reasonable to me. Maybe users should express the skipping
behavior that they desire in terms of the *proportion* of all heap
blocks with LP_DEAD line pointers that we're willing to have while
still skipping index vacuuming + lazy_vacuum_heap() heap scan. In
other words, it can be a "scale" type GUC/param (though based on heap
blocks *at the end* of the first heap scan, not tuples at the point
the av launcher considers launching AV workers).

> @@ -1299,6 +1303,7 @@ lazy_scan_heap(Relation onerel, VacuumParams
> *params, LVRelStats *vacrelstats,
> {
> lazy_record_dead_tuple(dead_tuples, &(tuple.t_self));
> all_visible = false;
> +   has_dead_tuples = true;
> continue;
> }
>
> I added the above change in the patch to count the number of heap
> pages having at least one LP_DEAD line pointer. But it's weird to me
> that we have never set has_dead_tuples true when we found an LP_DEAD
> line pointer.

I think that you're right. However, in practice it isn't harmful
because has_dead_tuples is only used when "all_visible = true", and
only to detect corruption (which should never happen). I think that it
should be fixed as part of this work, though.

Lots of stuff in this area is kind of weird already. Sometimes this is
directly exposed to users, even. This came up recently, when I was
working on VACUUM VERBOSE stuff. (This touched the precise piece of
code you've patched in the quoted diff snippet, so perhaps you know
some of the story I will tell you now already.)

I recently noticed that VACUUM VERBOSE can report a very low
tups_vacuumed/"removable heap tuples" when run against tables where
most pruning is opportunistic pruning rather than VACUUM pruning
(which is very common), provided there are no HOT updates (which is
common but not very common). This can be very confusing, because
VACUUM VERBOSE will report a "tuples_deleted" for the heap relation
that is far far less than the "tuples_removed" it reports for indexes
on the same table -- even though both fields have values that are
technically accurate (e.g., not very affected by concurrent activity
during VACUUM, nothing like that).

This came to my attention when I was running BenchmarkSQL for the
64-bit XID deleted pages patch. One of the BenchmarkSQL tables (though
only one -- the table whose UPDATEs are not HOT safe, which is unique
among BenchmarkSQL/TPC-C tables). I pushed a commit with comment
changes [1] to make that aspect of VACUUM VERBOSE a little less
confusing. (I was actually running a quick-and-dirty hack that made
log_autovacuum show VACUUM VERBOSE index stuff -- I would probably
have missed the weird difference between heap tups_vacuumed and index
tuples_removed without this custom log_autovacuum hack.)

Just to be clear (I think you agree already): we should base any
triggering logic for skipping index vacuuming/lazy_vacuum_heap() on
logic that does not care *when* heap pages first contained LP_DEAD
line pointers (could be that they were counted in tups_vacuumed due to
being pruned during this VACUUM operation, could be from an earlier
opportunistic pruning, etc).

> Currently, we set it to false true in 'tupgone' case but
> it seems to me that we should do that in this case as well since we
> use this flag in the following check:
>
>else if (PageIsAllVisible(page) && has_dead_tuples)
>{
>elog(WARNING, "page containing dead tuples is marked as
> all-visible in relation \"%s\" page %u",
> vacrelstats->relname, blkno);
>PageClearAllVisible(page);
>MarkBufferDirty(buf);
>visibilitymap_clear(onerel, blkno, vmbuffer,
>VISIBILITYMAP_VALID_BITS);
>}

The "tupgone = true"/HEAPTUPLE_DEAD-race case is *extremely* weird. It
has zero test coverage according to coverage.postgresql.org [2],
despite being very complicated.

3 points on the "tupgone = true" weirdness (I'm writing this as a
record for myself, almost):

1. It is the reason why lazy_vacuum_heap() must be prepared to set
tuples LP_UNUSED that are not already LP_DEAD. So when
lazy_vacuum_page() says "the first dead tuple for this page",  that
doesn't

Re: archive_command / pg_stat_archiver & documentation

[Julien Rouhaud]

On Tue, Mar 2, 2021 at 9:29 AM Michael Paquier  wrote:
>
> On Mon, Mar 01, 2021 at 05:17:06PM +0800, Julien Rouhaud wrote:
> > Maybe this can be better addressed than with a link in the
> > documentation.  The final outcome is that it can be difficult to
> > monitor the archiver state in such case.  That's orthogonal to this
> > patch but maybe we can add a new "archiver_start" timestamptz column
> > in pg_stat_archiver, so monitoring tools can detect a problem if it's
> > too far away from pg_postmaster_start_time() for instance?
>
> There may be other solutions as well.  I have applied the doc patch
> for now.

Thanks!

Re: 64-bit XIDs in deleted nbtree pages

[Peter Geoghegan]

On Mon, Mar 1, 2021 at 1:40 PM Peter Geoghegan  wrote:
> > Since it seems not a bug I personally think we don't need to do
> > anything for back branches. But if we want not to trigger an index
> > scan by vacuum_cleanup_index_scale_factor, we could change the default
> > value to a high value (say, to 1) so that it can skip an index
> > scan in most cases.
>
> One reason to remove vacuum_cleanup_index_scale_factor in the back
> branches is that it removes any need to fix the
> "IndexVacuumInfo.num_heap_tuples is inaccurate outside of
> btvacuumcleanup-only VACUUMs" bug -- it just won't matter if
> btm_last_cleanup_num_heap_tuples is inaccurate anymore. (I am still
> not sure about backpatch being a good idea, though.)

Attached is v8 of the patch series, which has new patches. No real
changes compared to v7 for the first patch, though.

There are now two additional prototype patches to remove the
vacuum_cleanup_index_scale_factor GUC/param along the lines we've
discussed. This requires teaching VACUUM ANALYZE about when to trust
VACUUM cleanup to set the statistics (that's what v8-0002* does).

The general idea for VACUUM ANALYZE in v8-0002* is to assume that
cleanup-only VACUUMs won't set the statistics accurately -- so we need
to keep track of this during VACUUM (in case it's a VACUUM ANALYZE,
which now needs to know if index vacuuming was "cleanup only" or not).
This is not a new thing for hash indexes -- they never did anything in
the cleanup-only case (hashvacuumcleanup() just returns NULL). And now
nbtree does the same thing (usually). Not all AMs will, but the new
assumption is much better than the one it replaces.

I thought of another existing case that violated the faulty assumption
made by VACUUM ANALYZE (which v8-0002* fixes): VACUUM's INDEX_CLEANUP
feature (which was added to Postgres 12 by commit a96c41feec6) is
another case where VACUUM does nothing with indexes. VACUUM ANALYZE
mistakenly considers that index vacuuming must have run and set the
pg_class statistics to an accurate value (more accurate than it is
capable of). But with INDEX_CLEANUP we won't even call
amvacuumcleanup().

-- 
Peter Geoghegan
From 967a057607ce2d0b648e324a9085ab4ccecd828e Mon Sep 17 00:00:00 2001
From: Peter Geoghegan 
Date: Thu, 25 Feb 2021 15:17:22 -0800
Subject: [PATCH v8 1/3] Recycle pages deleted during same VACUUM.

Author: Peter Geoghegan 
Discussion: https://postgr.es/m/CAH2-Wzk76_P=67iuscb1un44-gyzl-kgpsxbsxq_bdcma7q...@mail.gmail.com
---
 src/include/access/nbtree.h | 22 ++-
 src/backend/access/nbtree/README| 31 +
 src/backend/access/nbtree/nbtpage.c | 40 
 src/backend/access/nbtree/nbtree.c  | 97 +
 src/backend/access/nbtree/nbtxlog.c | 22 +++
 5 files changed, 211 insertions(+), 1 deletion(-)

diff --git a/src/include/access/nbtree.h b/src/include/access/nbtree.h
index b56b7b7868..876b8f3437 100644
--- a/src/include/access/nbtree.h
+++ b/src/include/access/nbtree.h
@@ -279,7 +279,8 @@ BTPageGetDeleteXid(Page page)
  * Is an existing page recyclable?
  *
  * This exists to centralize the policy on which deleted pages are now safe to
- * re-use.
+ * re-use.  The _bt_newly_deleted_pages_recycle() optimization behaves more
+ * aggressively, though that has certain known limitations.
  *
  * Note: PageIsNew() pages are always safe to recycle, but we can't deal with
  * them here (caller is responsible for that case themselves).  Caller might
@@ -316,14 +317,33 @@ BTPageIsRecyclable(Page page)
  * BTVacState is private nbtree.c state used during VACUUM.  It is exported
  * for use by page deletion related code in nbtpage.c.
  */
+typedef struct BTPendingRecycle
+{
+	BlockNumber blkno;
+	FullTransactionId safexid;
+} BTPendingRecycle;
+
 typedef struct BTVacState
 {
+	/*
+	 * VACUUM operation state
+	 */
 	IndexVacuumInfo *info;
 	IndexBulkDeleteResult *stats;
 	IndexBulkDeleteCallback callback;
 	void	   *callback_state;
 	BTCycleId	cycleid;
+
+	/*
+	 * Page deletion state for VACUUM
+	 */
 	MemoryContext pagedelcontext;
+	BTPendingRecycle *deleted;
+	bool		grow;
+	bool		full;
+	uint32		ndeletedspace;
+	uint64		maxndeletedspace;
+	uint32		ndeleted;
 } BTVacState;
 
 /*
diff --git a/src/backend/access/nbtree/README b/src/backend/access/nbtree/README
index 46d49bf025..265814ea46 100644
--- a/src/backend/access/nbtree/README
+++ b/src/backend/access/nbtree/README
@@ -430,6 +430,37 @@ whenever it is subsequently taken from the FSM for reuse.  The deleted
 page's contents will be overwritten by the split operation (it will become
 the new right sibling page).
 
+Prior to PostgreSQL 14, VACUUM was only able to recycle pages that were
+deleted by a previous VACUUM operation (VACUUM typically placed all pages
+deleted by the last VACUUM into the FSM, though there were and are no
+certainties here).  This had the obvious disadvantage of creating
+uncertainty about when and how pages get recycled, especially with bursty
+workloads.  It was naive, 

Re: [PATCH] refactor ATExec{En,Dis}ableRowSecurity

[Michael Paquier]

On Mon, Mar 01, 2021 at 03:30:44PM +0900, Michael Paquier wrote:
> 0001 is a clean simplification and a good catch, so I'll see about
> applying it.  0002 just makes the code more confusing to the reader
> IMO, and its interface could easily lead to unwanted errors.

0001 has been applied as of fabde52.
--
Michael


signature.asc
Description: PGP signature

Re: Feature improvement: can we add queryId for pg_catalog.pg_stat_activity view?

[Julien Rouhaud]

On Wed, Jan 20, 2021 at 12:43 AM Julien Rouhaud  wrote:
>
> On Fri, Jan 8, 2021 at 1:07 AM Julien Rouhaud  wrote:
> >
> > v15 that fixes recent conflicts.
>
> Rebase only, thanks to the cfbot!  V16 attached.

Recent commit exposed that the explain_filter() doesn't filter
negative sign.  This can now be a problem with query identifiers in
explain output as they use the whole bigint range.  v17 attached fixes
that, also rebased against current HEAD.
From 8904b60ed3cc770f209ae5f97804b4d1ffe7d175 Mon Sep 17 00:00:00 2001
From: Julien Rouhaud 
Date: Sun, 8 Mar 2020 14:34:44 +0100
Subject: [PATCH v17 3/3] Expose query identifier in verbose explain

If a query identifier has been computed, either by enabling compute_queryid or
using a third-party module, verbose explain will display it.

Author: Julien Rouhaud
Reviewed-by:
Discussion: https://postgr.es/m/CA+8PKvQnMfOE-c3YLRwxOsCYXQDyP8VXs6CDtMZp1V4=d4l...@mail.gmail.com
---
 doc/src/sgml/config.sgml  | 14 +++---
 doc/src/sgml/ref/explain.sgml |  6 --
 src/backend/commands/explain.c| 18 ++
 src/test/regress/expected/explain.out | 11 ++-
 src/test/regress/sql/explain.sql  |  5 -
 5 files changed, 43 insertions(+), 11 deletions(-)

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 1763790473..2aeb146223 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -7524,13 +7524,13 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv;
 Enables or disables in core query identifier computation.  A query
 identifier can be displayed in the pg_stat_activity
-view, or emitted in the log if configured via the  parameter.  The  extension also requires a query identifier
-to be computed.  Note that an external module can alternatively be used
-if the in core query identifier computation specification doesn't suit
-your need.  In this case, in core computation must be disabled.  The
-default is off.
+view, using EXPLAIN, or emitted in the log if
+configured via the  parameter.
+The  extension also requires a query
+identifier to be computed.  Note that an external module can
+alternatively be used if the in core query identifier computation
+specification doesn't suit your need.  In this case, in core
+computation must be disabled.  The default is off.

   
  
diff --git a/doc/src/sgml/ref/explain.sgml b/doc/src/sgml/ref/explain.sgml
index c4512332a0..105b069b41 100644
--- a/doc/src/sgml/ref/explain.sgml
+++ b/doc/src/sgml/ref/explain.sgml
@@ -136,8 +136,10 @@ ROLLBACK;
   the output column list for each node in the plan tree, schema-qualify
   table and function names, always label variables in expressions with
   their range table alias, and always print the name of each trigger for
-  which statistics are displayed.  This parameter defaults to
-  FALSE.
+  which statistics are displayed.  The query identifier will also be
+  displayed if one has been compute, see  for more details.  This parameter
+  defaults to FALSE.
  
 

diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index afc45429ba..ac5879c1cf 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -24,6 +24,7 @@
 #include "nodes/extensible.h"
 #include "nodes/makefuncs.h"
 #include "nodes/nodeFuncs.h"
+#include "parser/analyze.h"
 #include "parser/parsetree.h"
 #include "rewrite/rewriteHandler.h"
 #include "storage/bufmgr.h"
@@ -163,6 +164,8 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt,
 {
 	ExplainState *es = NewExplainState();
 	TupOutputState *tstate;
+	JumbleState *jstate = NULL;
+	Query		*query;
 	List	   *rewritten;
 	ListCell   *lc;
 	bool		timing_set = false;
@@ -239,6 +242,13 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt,
 	/* if the summary was not set explicitly, set default value */
 	es->summary = (summary_set) ? es->summary : es->analyze;
 
+	query = castNode(Query, stmt->query);
+	if (compute_queryid)
+		jstate = JumbleQuery(query, pstate->p_sourcetext);
+
+	if (post_parse_analyze_hook)
+		(*post_parse_analyze_hook) (pstate, query, jstate);
+
 	/*
 	 * Parse analysis was done already, but we still have to run the rule
 	 * rewriter.  We do not do AcquireRewriteLocks: we assume the query either
@@ -598,6 +608,14 @@ ExplainOnePlan(PlannedStmt *plannedstmt, IntoClause *into, ExplainState *es,
 	/* Create textual dump of plan tree */
 	ExplainPrintPlan(es, queryDesc);
 
+	if (es->verbose && plannedstmt->queryId != UINT64CONST(0))
+	{
+		char	buf[MAXINT8LEN+1];
+
+		pg_lltoa(plannedstmt->queryId, buf);
+		ExplainPropertyText("Query Identifier", buf, es);
+	}
+
 	/* Show buffer usage in planning */
 	if (bufusage)
 	{
diff --git a/src/test/regress/expected/explain.out b/src/test/regress/expected/explain.out
index dc7ab2ce8b..f45f069f30 100644
--- a/src/

Re: REINDEX backend filtering

[Julien Rouhaud]

On Fri, Feb 26, 2021 at 11:17:26AM +0100, Magnus Hagander wrote:
> On Fri, Feb 26, 2021 at 11:07 AM Julien Rouhaud  wrote:
> >
> > It means that you'll have to distribute the work on a per-table basis
> > rather than a per-index basis.  The time spent to find out that a
> > table doesn't have any impacted index should be negligible compared to
> > the cost of running a reindex.  This obviously won't help that much if
> > you have a lot of table but only one being gigantic.
> 
> Yeah -- or at least a couple of large and many small, which I find to
> be a very common scenario. Or the case of some tables having many
> affected indexes and some having few.
> 
> You'd basically want to order the operation by table on something like
> "total size of the affected indexes on table x" -- which may very well
> put a smaller table with many indexes earlier in the queue. But you
> can't do that without having access to the filter

So, long running reindex due to some gigantic and/or numerous indexes on a
single (or few) table is not something that we can solve, but inefficient
reindex due to wrong table size / to-be-reindexed-indexes-size correlation can
be addressed.

I would still prefer to go to backend implementation, so that all client tools
can benefit from it by default.  We could simply export the current
index_has_oudated_collation(oid) function in sql, and tweak pg_dump to order
tables by the cumulated size of such indexes as you mentioned below, would
that work for you?

Also, given Thomas proposal in a nearby email this function would be renamed to
index_has_oudated_dependencies(oid) or something like that.

> > But even if we put the logic in the client, this still won't help as
> > reindexdb doesn't support multiple job with an index list:
> >
> >  * Index-level REINDEX is not supported with multiple jobs as we
> >  * cannot control the concurrent processing of multiple indexes
> >  * depending on the same relation.
> >  */
> > if (concurrentCons > 1 && indexes.head != NULL)
> > {
> > pg_log_error("cannot use multiple jobs to reindex indexes");
> > exit(1);
> > }
> 
> That sounds like it would be a fixable problem though, in principle.
> It could/should probably still limit all indexes on the same table to
> be processed in the same connection for the locking reasons of course,
> but doing an order by the total size of the indexes like above, and
> ensuring that they are grouped that way, doesn't sound *that* hard. I
> doubt it's that important in the current usecase of manually listing
> the indexes, but it would be useful for something like this.

Yeah, I don't think that in case of oudated dependency the --index will be
useful, it's likely that there will be too many indexes to process.  We can
still try to improve reindexdb to be able to process index lists with parallel
connections, but I would rather keep that separated from this patch.

Re: repeated decoding of prepared transactions

[Amit Kapila]

On Tue, Mar 2, 2021 at 8:20 AM vignesh C  wrote:
>
>
> I have a minor comment regarding the below:
> + 
> +  
> +   two_phase bool
> +  
> +  
> +  True if two-phase commits are enabled on this slot.
> +  
> + 
>
> Can we change something like:
> True if the slot is enabled for decoding prepared transaction
> information. Refer link for more information.(link should point where
> more detailed information is available for two-phase in
> pg_create_logical_replication_slot).
>
> Also there is one small indentation in that line, I think there should
> be one space before "True if".
>

Okay, fixed these but I added a slightly different description. I have
also added the parameter description for
pg_create_logical_replication_slot in docs and changed the comments at
various places in the code. Apart from that ran pgindent. The patch
looks good to me now. Let me know what do you think?

-- 
With Regards,
Amit Kapila.


v9-0001-Add-option-to-enable-two_phase-commits-via-pg_cre.patch
Description: Binary data

Re: 64-bit XIDs in deleted nbtree pages

[Masahiko Sawada]

On Tue, Mar 2, 2021 at 6:40 AM Peter Geoghegan  wrote:
>
> On Sun, Feb 28, 2021 at 8:08 PM Masahiko Sawada  wrote:
> > > Even though "the letter of the law" favors removing the
> > > vacuum_cleanup_index_scale_factor GUC + param in the way I have
> > > outlined, that is not the only thing that matters -- we must also
> > > consider "the spirit of the law".
>
> > > I suppose I could ask Tom what he thinks?
> >
> > +1
>
> Are you going to start a new thread, or should I?

Ok, I'll start a new thread soon.

>
> > Since it seems not a bug I personally think we don't need to do
> > anything for back branches. But if we want not to trigger an index
> > scan by vacuum_cleanup_index_scale_factor, we could change the default
> > value to a high value (say, to 1) so that it can skip an index
> > scan in most cases.
>
> One reason to remove vacuum_cleanup_index_scale_factor in the back
> branches is that it removes any need to fix the
> "IndexVacuumInfo.num_heap_tuples is inaccurate outside of
> btvacuumcleanup-only VACUUMs" bug -- it just won't matter if
> btm_last_cleanup_num_heap_tuples is inaccurate anymore. (I am still
> not sure about backpatch being a good idea, though.)

I think that removing vacuum_cleanup_index_scale_factor in the back
branches would affect the existing installation much. It would be
better to have btree indexes not use this parameter while not changing
the contents of meta page. That is, just remove the check related to
vacuum_cleanup_index_scale_factor from _bt_vacuum_needs_cleanup(). And
I personally prefer to fix the "IndexVacuumInfo.num_heap_tuples is
inaccurate outside of btvacuumcleanup-only VACUUMs" bug separately.

>
> > > Another new concern for me (another concern unique to Postgres 13) is
> > > autovacuum_vacuum_insert_scale_factor-driven autovacuums.
> >
> > IIUC the purpose of autovacuum_vacuum_insert_scale_factor is
> > visibility map maintenance. And as per this discussion, it seems not
> > necessary to do an index scan in btvacuumcleanup() triggered by
> > autovacuum_vacuum_insert_scale_factor.
>
> Arguably the question of skipping scanning the index should have been
> considered by the autovacuum_vacuum_insert_scale_factor patch when it
> was committed for Postgres 13 -- but it wasn't. There is a regression
> that was tied to autovacuum_vacuum_insert_scale_factor in Postgres 13
> by Mark Callaghan, which I suspect is relevant:
>
> https://smalldatum.blogspot.com/2021/01/insert-benchmark-postgres-is-still.html
>
> The blog post says: "Updates - To understand the small regression
> mentioned above for the l.i1 test (more CPU & write IO) I repeated the
> test with 100M rows using 2 configurations: one disabled index
> deduplication and the other disabled insert-triggered autovacuum.
> Disabling index deduplication had no effect and disabling
> insert-triggered autovacuum resolves the regression."
>
> This is quite specifically with an insert-only workload, with 4
> indexes (that's from memory, but I'm pretty sure it's 4). I think that
> the failure to account for skipping index scans is probably the big
> problem here. Scanning the heap to set VM bits is unlikely to be
> expensive compared to the full index scans. An insert-only workload is
> going to find most of the heap blocks it scans to set VM bits in
> shared_buffers. Not so for the indexes.
>
> So in Postgres 13 we have this autovacuum_vacuum_insert_scale_factor
> issue, in addition to the deduplication + btvacuumcleanup issue we
> talked about (the problems left by my Postgres 13 bug fix commit
> 48e12913). These two issues make removing
> vacuum_cleanup_index_scale_factor tempting, even in the back branches
> -- it might actually be the more conservative approach, at least for
> Postgres 13.

Yeah, this argument makes sense to me. The default values of
autovacuum_vacuum_insert_scale_factor/threshold are 0.2 and 1000
respectively whereas one of vacuum_cleanup_index_scale_factor is 0.1.
It means that in insert-only workload with default settings,
autovacuums triggered by autovacuum_vacuum_insert_scale_factor always
scan the all btree index to update the index statistics. I think most
users would not expect this behavior. As I mentioned above, I think we
can have nbtree not use this parameter or increase the default value
of vacuum_cleanup_index_scale_factor in back branches.

Regards,

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

Re: Fix DROP TABLESPACE on Windows with ProcSignalBarrier?

[Thomas Munro]

On Tue, Feb 2, 2021 at 11:16 AM Thomas Munro  wrote:
> Right, the checkpoint itself is probably worse than this
> "close-all-your-files!" thing in some cases [...]

I've been wondering what obscure hazards these "tombstone" (for want
of a better word) files guard against, besides the one described in
the comments for mdunlink().  I've been  thinking about various
schemes that can be summarised as "put the tombstones somewhere else",
but first... this is probably a stupid question, but what would break
if we just ... turned all this stuff off when wal_level is high enough
(as it is by default)?


0001-Make-relfile-tombstone-files-conditional-on-WAL-leve.not-for-cfbot-patch
Description: Binary data