Re: New Table Access Methods for Multi and Single Inserts

[Bharath Rupireddy]

Hi,

I addressed the following review comments and attaching v3 patch set.

1) ExecClearTuple happens before ExecCopySlot in heap_multi_insert_v2
and this allowed us to remove clear_mi_slots flag from
TableInsertState.
2) I retained the flushed variable inside TableInsertState so that the
callers can know whether the buffered slots have been flushed. If yes,
the callers can execute after insert row triggers or perform index
insertions. This is easier than passing the after insert row triggers
info and index info to new multi insert table am and let it do. This
way the functionalities can be kept separate i.e. multi insert ams do
only buffering, decisions on when to flush, insertions and the callers
will execute triggers or index insertions. And also none of the
existing table ams are performing these operations within them, so
this is inline with the current design of the table ams.
3) I have kept the single and multi insert API separate. The previous
suggestion was to have only a single insert API and let the callers
provide initially whether they want multi or single inserts. One
problem with that approach is that we have to allow table ams to
execute the after row triggers or index insertions. That is something
I personally don't like.

0001 - new table ams implementation
0002 - the new multi table ams used in CREATE TABLE AS and REFRESH
MATERIALIZED VIEW
0003 - the new multi table ams used in COPY

Please review the v3 patch set further.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
From 49060fdc2c2a2e6caf1a489fcd16cafd0e1e20a3 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy 
Date: Wed, 17 Feb 2021 11:06:35 +0530
Subject: [PATCH v3] New Table AMs for Multi and Single Inserts

This patch introduces new table access methods for multi and
single inserts. Also implements/rearranges the outside code for
heap am into these new APIs.

Main design goal of these new APIs is to give flexibility to
tableam developers in implementing multi insert logic dependent on
the underlying storage engine. Currently, for all the underlying
storage engines, we follow the same multi insert logic such as when
and how to flush the buffered tuples, tuple size calculation, and
this logic doesn't take into account the underlying storage engine
capabilities.

We can also avoid duplicating multi insert code (for existing COPY,
and upcoming CTAS, CREATE/REFRESH MAT VIEW and INSERT SELECTs). We
can also move bulk insert state allocation and deallocation inside
these APIs.
---
 src/backend/access/heap/heapam.c | 212 +++
 src/backend/access/heap/heapam_handler.c |   5 +
 src/backend/access/table/tableamapi.c|   7 +
 src/backend/executor/execTuples.c|  83 -
 src/include/access/heapam.h  |  49 +-
 src/include/access/tableam.h |  87 ++
 src/include/executor/tuptable.h  |   1 +
 7 files changed, 438 insertions(+), 6 deletions(-)

diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 9926e2bd54..789228aafb 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -67,6 +67,7 @@
 #include "utils/datum.h"
 #include "utils/inval.h"
 #include "utils/lsyscache.h"
+#include "utils/memutils.h"
 #include "utils/relcache.h"
 #include "utils/snapmgr.h"
 #include "utils/spccache.h"
@@ -2522,6 +2523,217 @@ heap_multi_insert(Relation relation, TupleTableSlot **slots, int ntuples,
 	pgstat_count_heap_insert(relation, ntuples);
 }
 
+/*
+ * heap_insert_begin - allocate and initialize TableInsertState
+ *
+ * For single inserts:
+ *  1) Specify is_multi as false, then multi insert state will be NULL.
+ *
+ * For multi inserts:
+ *  1) Specify is_multi as true, then multi insert state will be allocated and
+ * 	   initialized.
+ *
+ *  Other input parameters i.e. relation, command id, options are common for
+ *  both single and multi inserts.
+ */
+TableInsertState*
+heap_insert_begin(Relation rel, CommandId cid, int options, bool is_multi)
+{
+	TableInsertState *state;
+
+	state = palloc(sizeof(TableInsertState));
+	state->rel = rel;
+	state->cid = cid;
+	state->options = options;
+	/* Below parameters are not used for single inserts. */
+	state->mi_slots = NULL;
+	state->mistate = NULL;
+	state->mi_cur_slots = 0;
+	state->flushed = false;
+
+	if (is_multi)
+	{
+		HeapMultiInsertState *mistate;
+
+		mistate = palloc(sizeof(HeapMultiInsertState));
+		state->mi_slots =
+palloc0(sizeof(TupleTableSlot *) * MAX_BUFFERED_TUPLES);
+		mistate->max_slots = MAX_BUFFERED_TUPLES;
+		mistate->max_size = MAX_BUFFERED_BYTES;
+		mistate->cur_size = 0;
+		/*
+		 * Create a temporary memory context so that we can reset once per
+		 * multi insert batch.
+		 */
+		mistate->context = AllocSetContextCreate(CurrentMemoryContext,
+ "heap_multi_insert",
+ ALLOCSET_DEFAULT_SIZES);
+		state->mistate = mistate;
+	}
+
+	return state;
+}
+
+/*
+ * heap_insert_v2 - insert 

Re: pg_collation_actual_version() ERROR: cache lookup failed for collation 123

[Michael Paquier]

On Wed, Feb 17, 2021 at 03:08:36PM +1300, Thomas Munro wrote:
>   tp = SearchSysCache1(COLLOID, ObjectIdGetDatum(oid));
>   if (!HeapTupleIsValid(tp))
> + {
> + if (found)
> + {
> + *found = false;
> + return NULL;
> + }
>   elog(ERROR, "cache lookup failed for collation %u", 
> oid);
> + }
>   collform = (Form_pg_collation) GETSTRUCT(tp);
>   version = get_collation_actual_version(collform->collprovider,
>   
>NameStr(collform->collcollate));
> + if (found)
> + *found = true;
>   }

FWIW, we usually prefer using NULL instead of an error for the result
of a system function if an object cannot be found because it allows
users to not get failures in a middle of a full table scan if things
like an InvalidOid is mixed in the data set.  For example, we do that
in the partition functions, for objectaddress functions, etc.  That
would make this patch set simpler, switching
get_collation_version_for_oid() to just use a missing_ok argument.
And that would be more consistent with the other syscache lookup
functions we have here and there in the tree.
--
Michael


signature.asc
Description: PGP signature

Re: progress reporting for partitioned REINDEX

[Michael Paquier]

On Wed, Feb 17, 2021 at 12:10:43AM -0600, Justin Pryzby wrote:
> On Wed, Feb 17, 2021 at 02:55:04PM +0900, Michael Paquier wrote:
>> I see no bug here.
> 
> pg_stat_progress_create_index includes partitions_{done,total} for
> CREATE INDEX p, so isn't it strange if it wouldn't do likewise for
> REINDEX INDEX p ?

There is always room for improvement.  This stuff applies now only
when creating an index in the non-concurrent case because an index
cannot be created on a partitioned table concurrently, and this
behavior is documented as such.  If we are going to improve this area,
it seems to me that we may want to consider more cases than just the
case of partitions, as it could also help the monitoring of REINDEX on
schemas and databases.

I don't think that this fits as an open item.  That's just a different
feature.
--
Michael


signature.asc
Description: PGP signature

Re: progress reporting for partitioned REINDEX

[Justin Pryzby]

On Wed, Feb 17, 2021 at 02:55:04PM +0900, Michael Paquier wrote:
> I see no bug here.

pg_stat_progress_create_index includes partitions_{done,total} for
CREATE INDEX p, so isn't it strange if it wouldn't do likewise for
REINDEX INDEX p ?

-- 
Justin

Re: progress reporting for partitioned REINDEX

[Michael Paquier]

On Tue, Feb 16, 2021 at 12:39:08PM +0100, Matthias van de Meent wrote:
> These were added to report the index and table that are currently
> being worked on in concurrent reindexes of tables, schemas and
> databases. Before that commit, it would only report up to the last
> index being prepared in phase 1, leaving the user with no info on
> which index is being rebuilt.

Nothing much to add on top of what Matthias is saying here.  REINDEX
for partitioned relations builds first the full list of partitions to
work on, and then processes each one of them in a separate
transaction.  This is consistent with what we do for other commands
that need to handle an object different than a non-partitioned table
or a non-partitioned index.  The progress reporting has to report the
index whose storage is manipulated and its parent table.

> Why pgstat_progress_start_command specifically was chosen? That is
> because there is no method to update the
> beentry->st_progress_command_target other than through
> stat_progress_start_command, and according to the docs that field
> should contain the tableId of the index that is currently being worked
> on. This field needs a pgstat_progress_start_command because CIC / RiC
> reindexes all indexes concurrently at the same time (and not grouped
> by e.g. table), so we must re-start reporting for each index in each
> new phase in which we report data to get the heapId reported correctly
> for that index.

Using pgstat_progress_start_command() for this purpose is fine IMO.
This provides enough information for the user without complicating
more this API layer.

-   if (progress)
-   
pgstat_progress_update_param(PROGRESS_CREATEIDX_ACCESS_METHOD_OID,
-
iRel->rd_rel->relam);
+   // Do this unconditionally?
+   pgstat_progress_update_param(PROGRESS_CREATEIDX_ACCESS_METHOD_OID,
+
iRel->rd_rel->relam);
You cannot do that, this would clobber the progress information of any
upper layer that already reports something to the progress infra in
the backend's MyProc.  CLUSTER is one example calling reindex_relation()
that does *not* want progress reporting to happen in REINDEX. 

+   /* progress reporting for partitioned indexes */
+   if (relkind == RELKIND_PARTITIONED_INDEX)
+   {
+   const int   progress_index[3] = {
+   PROGRESS_CREATEIDX_COMMAND,
+   PROGRESS_CREATEIDX_INDEX_OID,
+   PROGRESS_CREATEIDX_PARTITIONS_TOTAL
+   };
This does not make sense in ReindexPartitions() IMO because this
relation is not reindexed as it has no storage, and you would lose the
context of each partition.

Something that we may want to study instead is whether we'd like to
report to the user the set of relations a REINDEX command is working
on and on which relation the work is currently done.  But I am not
really sure that we need that as long a we have a VERBOSE option that
lets us know via the logs what already happened in a single command.

I see no bug here.
--
Michael


signature.asc
Description: PGP signature

Re: [POC] verifying UTF-8 using SIMD instructions

[John Naylor]

I wrote:

> [v3]
> - It's not smart enough to stop at the last valid character boundary --
it's either all-valid or it must start over with the fallback. That will
have to change in order to work with the proposed noError conversions. It
shouldn't be very hard, but needs thought as to the clearest and safest way
to code it.

In v4, it should be able to return an accurate count of valid bytes even
when the end crosses a character boundary.

> - This is my first time hacking autoconf, and it still seems slightly
broken, yet functional on my machine at least.

It was actually completely broken if you tried to pass the special flags to
configure. I redesigned this part and it seems to work now.

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


v4-SSE4-with-autoconf-support.patch
Description: Binary data

Re: [HACKERS][PATCH] Applying PMDK to WAL operations for persistent memory

[Takashi Menjo]

Rebased to make patchset v5.

I also found that my past replies have separated the thread in the
pgsql-hackers archive. I try to connect this mail to the original
thread [1], and let this point to the separated portions [2][3][4].
Note that the patchset v3 is in [3] and v4 is in [4].

Regards,

[1] 
https://www.postgresql.org/message-id/flat/C20D38E97BCB33DAD59E3A1%40lab.ntt.co.jp
[2] 
https://www.postgresql.org/message-id/flat/000501d4b794%245094d140%24f1be73c0%24%40lab.ntt.co.jp
[3] 
https://www.postgresql.org/message-id/flat/01d4b863%244c9e8fc0%24e5dbaf40%24%40lab.ntt.co.jp
[4] 
https://www.postgresql.org/message-id/flat/01d4c2a1%2488c6cc40%249a5464c0%24%40lab.ntt.co.jp

-- 
Takashi Menjo 


v5-0001-Add-configure-option-for-PMDK.patch
Description: Binary data


v5-0003-Walreceiver-WAL-IO-using-PMDK.patch
Description: Binary data


v5-0002-Read-write-WAL-files-using-PMDK.patch
Description: Binary data

Re: ERROR: invalid spinlock number: 0

[Michael Paquier]

On Tue, Feb 16, 2021 at 11:47:52PM +0900, Fujii Masao wrote:
> On 2021/02/16 15:50, Michael Paquier wrote:
>> +   /*
>> +* Read "writtenUpto" without holding a spinlock. So it may not be
>> +* consistent with other WAL receiver's shared variables protected by a
>> +* spinlock. This is OK because that variable is used only for
>> +* informational purpose and should not be used for data integrity 
>> checks.
>> +*/
>> What about the following?
>> "Read "writtenUpto" without holding a spinlock.  Note that it may not
>> be consistent with the other shared variables of the WAL receiver
>> protected by a spinlock, but this should not be used for data
>> integrity checks."
> 
> Sounds good. Attached is the updated version of the patch.

Thanks, looks good to me.

>> I agree that what has been done with MyProc->waitStart in 46d6e5f is
>> not safe, and that initialization should happen once at postmaster
>> startup, with a write(0) when starting the backend.  There are two of
>> them in proc.c, one in twophase.c.  Do you mind if I add an open item
>> for this one?
> 
> Yeah, please feel free to do that! BTW, I already posted the patch
> addressing that issue, at [1].

Okay, item added with a link to the original thread.
--
Michael


signature.asc
Description: PGP signature

Re: Is it worth accepting multiple CRLs?

[Kyotaro Horiguchi]

The commit fe61df7f82 shot down this.

This patch allows a new GUC ssl_crl_dir and a new libpq connection
option sslcrldir to specify CRL directory, which stores multiple files
that contains one CRL. With that method server loads only CRLs for the
CA of the certificate being validated.

Along with rebasing, the documentation is slightly reworded.

revocation list (CRL).  Certificates listed in this file, if it
 exists, will be rejected while attempting to authenticate the
-server's certificate.  If both sslcrl and sslcrldir are not set,
-this setting is assumed to be
+server's certificate.  If neither sslcrl sslcrldir is set, this
+setting is assumed to be
 ~/.postgresql/root.crl. See

And added a line for the new variable in postgresql.conf.sample.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
>From 9fbe390297174390f30fb1cc31b0cb543b335ae8 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi 
Date: Mon, 1 Feb 2021 11:16:41 +0900
Subject: [PATCH v5] Allow to specify CRL directory

This patch adds another method to specify CRLs, hashed directory
method for both on server and client side. This offers a means for
server or libpq to load only CRLs that are required to verify a
certificate. The CRL directory is specifed by separate GUC variable or
connection option from existing ssl_crl_file and sslcrl so both method
can be used at the same time.
---
 .../postgres_fdw/expected/postgres_fdw.out|  2 +-
 doc/src/sgml/config.sgml  | 21 +++-
 doc/src/sgml/libpq.sgml   | 21 ++--
 doc/src/sgml/runtime.sgml | 33 +++
 src/backend/libpq/be-secure-openssl.c | 26 +--
 src/backend/libpq/be-secure.c |  1 +
 src/backend/utils/misc/guc.c  | 10 ++
 src/backend/utils/misc/postgresql.conf.sample |  1 +
 src/include/libpq/libpq.h |  1 +
 src/interfaces/libpq/fe-connect.c |  6 
 src/interfaces/libpq/fe-secure-openssl.c  | 24 ++
 src/interfaces/libpq/libpq-int.h  |  1 +
 src/test/ssl/Makefile | 20 ++-
 src/test/ssl/ssl/client-crldir/9bb9e3c3.r0| 11 +++
 .../ssl/ssl/root+client-crldir/9bb9e3c3.r0| 11 +++
 .../ssl/ssl/root+client-crldir/a3d11bff.r0| 11 +++
 .../ssl/ssl/root+server-crldir/a3d11bff.r0| 11 +++
 .../ssl/ssl/root+server-crldir/a836cc2d.r0| 11 +++
 src/test/ssl/ssl/server-crldir/a836cc2d.r0| 11 +++
 src/test/ssl/t/001_ssltests.pl| 31 +++--
 src/test/ssl/t/SSLServer.pm   | 14 +++-
 21 files changed, 260 insertions(+), 18 deletions(-)
 create mode 100644 src/test/ssl/ssl/client-crldir/9bb9e3c3.r0
 create mode 100644 src/test/ssl/ssl/root+client-crldir/9bb9e3c3.r0
 create mode 100644 src/test/ssl/ssl/root+client-crldir/a3d11bff.r0
 create mode 100644 src/test/ssl/ssl/root+server-crldir/a3d11bff.r0
 create mode 100644 src/test/ssl/ssl/root+server-crldir/a836cc2d.r0
 create mode 100644 src/test/ssl/ssl/server-crldir/a836cc2d.r0

diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 60c7e115d6..c779c84634 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -8946,7 +8946,7 @@ DO $d$
 END;
 $d$;
 ERROR:  invalid option "password"
-HINT:  Valid options in this context are: service, passfile, channel_binding, connect_timeout, dbname, host, hostaddr, port, options, application_name, keepalives, keepalives_idle, keepalives_interval, keepalives_count, tcp_user_timeout, sslmode, sslcompression, sslcert, sslkey, sslrootcert, sslcrl, requirepeer, ssl_min_protocol_version, ssl_max_protocol_version, gssencmode, krbsrvname, gsslib, target_session_attrs, use_remote_estimate, fdw_startup_cost, fdw_tuple_cost, extensions, updatable, fetch_size, batch_size
+HINT:  Valid options in this context are: service, passfile, channel_binding, connect_timeout, dbname, host, hostaddr, port, options, application_name, keepalives, keepalives_idle, keepalives_interval, keepalives_count, tcp_user_timeout, sslmode, sslcompression, sslcert, sslkey, sslrootcert, sslcrl, sslcrldir, requirepeer, ssl_min_protocol_version, ssl_max_protocol_version, gssencmode, krbsrvname, gsslib, target_session_attrs, use_remote_estimate, fdw_startup_cost, fdw_tuple_cost, extensions, updatable, fetch_size, batch_size
 CONTEXT:  SQL statement "ALTER SERVER loopback_nopw OPTIONS (ADD password 'dummypw')"
 PL/pgSQL function inline_code_block line 3 at EXECUTE
 -- If we add a password for our user mapping instead, we should get a different
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 4df1405d2e..5c2900485c 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -1216,7 +1216,26 @@ include_dir 'conf.d'
 Relative 

Re: [DOC] add missing "[ NO ]" to various "DEPENDS ON" synopses

[Michael Paquier]

On Tue, Feb 16, 2021 at 11:18:47AM +0900, Ian Lawrence Barwick wrote:
> Hmm, with the current implementation "alter index my_index no "
> doesn't work
> anyway; you'd need to add this before the above lines:
> 
> +   else if (Matches("ALTER", "INDEX", MatchAny, "NO"))
> +   COMPLETE_WITH("DEPENDS");
> 
> so AFAICT the patch doesn't change that behaviour. It does mean "alter index
> my_index no depends " no longer completes to "ON EXTENSION", but if
> you've
> typed one of "NO" or "DEPENDS" in that context, "ON EXTENSION" is the only
> completion so I'm not sure what's gained by forcing the user to hit TAB
> twice.

You are right.  It looks like I have tested without a whitespace after
the "NO".  With a whitespace it does not work, so that looks like a
complication for little gain.  Another problem with the code on HEAD
is that you would not complete properly "NO DEPENDS ON", so that feels
half-completed.

> There are quite a few tab completions consisting of more than one word
> (e.g. "MATERIALIZED VIEW", "FORCE ROW LEVEL SECURITY") where tab completion
> is
> ineffective after the first word followed by a space, e.g. "alter
> materialized
> " doesn't result in any expansion either. I suppose we could go
> through all
> those and handle each word individually, but presumably there's a reason why
> that hasn't been done already (maybe no-one has complained?).

Because that's just extra maintenance as most people will just
complete after typing the first set of characters?  This part got
discussed as of 1e324cb:
https://www.postgresql.org/message-id/caltqxtcogrfevp9uou5vftngsn+vhzuu9+9a0inarfyvohs...@mail.gmail.com

Anyway, after sleeping on it, I have just applied your original patch
as that's simpler, and will cover the cases people would care for.
--
Michael


signature.asc
Description: PGP signature

Re: [Patch] ALTER SYSTEM READ ONLY

[Andres Freund]

Hi,

On 2021-02-16 17:11:06 -0500, Robert Haas wrote:
> I can't promise that what I'm about to write is an entirely faithful
> representation of what he said, but hopefully it's not so far off that
> he gets mad at me or something.

Seems accurate - and also I'm way too tired that I'd be mad ;)


> 1. If the server starts up and is read-only and
> ArchiveRecoveryRequested, clear the read-only state in memory and also
> in the control file, log a message saying that this has been done, and
> proceed. This makes some other cases simpler to deal with.

It seems also to make sense from a behaviour POV to me: Imagine a
"smooth" planned failover with ASRO:
1) ASRO on primary
2) promote standby
3) edit primary config to include primary_conninfo, add standby.signal
4) restart "read only primary"

There's not really any spot in which it'd be useful to do disable ASRO,
right? But 4) should make the node a normal standby.

Greetings,

Andres Freund

Re: pg_collation_actual_version() ERROR: cache lookup failed for collation 123

[Thomas Munro]

On Mon, Jan 18, 2021 at 11:22 AM Thomas Munro  wrote:
> On Mon, Jan 18, 2021 at 10:59 AM Justin Pryzby > |postgres=# SELECT
> pg_collation_actual_version(123);
> > |ERROR:  cache lookup failed for collation 123
>
> Yeah, not a great user experience.  Will fix next week; perhaps
> get_collation_version_for_oid() needs missing_ok and found flags, or
> something like that.

Here's a patch that gives:

postgres=# select pg_collation_actual_version(123);
ERROR:  no collation found for OID 123

It's a bit of an odd function, it's user-facing yet deals in OIDs.

> I'm also wondering if it would be better to name that thing with
> "current" rather than "actual".

Here's a patch to do that (note to self: would need a catversion bump).

While tidying up around here, I was dissatisfied with the fact that
there are three completely different ways of excluding "C[.XXX]" and
"POSIX" for three OSes, so here's a patch to merge them.

Also, here's the missing tab completion for CREATE COLLATION, since
it's rare enough to be easy to forget the incantations required.
From 992608eb81265856af3b9cbcb63597d91876090a Mon Sep 17 00:00:00 2001
From: Thomas Munro 
Date: Wed, 17 Feb 2021 14:10:40 +1300
Subject: [PATCH 1/4] Improve error message for pg_collation_actual_version().

Instead of an internal "cache lookup failed" message, show a message
intended for end user consumption, considering this is a user-exposed
function.

Reported-by: Justin Pryzby 
Discussion: https://postgr.es/m/20210117215940.GE8560%40telsasoft.com
---
 src/backend/catalog/index.c  |  5 +++--
 src/backend/catalog/pg_depend.c  |  3 ++-
 src/backend/commands/collationcmds.c |  8 +++-
 src/backend/utils/adt/pg_locale.c| 17 +++--
 src/include/utils/pg_locale.h|  2 +-
 5 files changed, 28 insertions(+), 7 deletions(-)

diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 1514937748..1c808f55c5 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -1290,7 +1290,8 @@ do_collation_version_check(const ObjectAddress *otherObject,
 		return false;
 
 	/* Ask the provider for the current version.  Give up if unsupported. */
-	current_version = get_collation_version_for_oid(otherObject->objectId);
+	current_version = get_collation_version_for_oid(otherObject->objectId,
+	NULL);
 	if (!current_version)
 		return false;
 
@@ -1369,7 +1370,7 @@ do_collation_version_update(const ObjectAddress *otherObject,
 	if (OidIsValid(*coll) && otherObject->objectId != *coll)
 		return false;
 
-	*new_version = get_collation_version_for_oid(otherObject->objectId);
+	*new_version = get_collation_version_for_oid(otherObject->objectId, NULL);
 
 	return true;
 }
diff --git a/src/backend/catalog/pg_depend.c b/src/backend/catalog/pg_depend.c
index 63da24322d..aee59eef39 100644
--- a/src/backend/catalog/pg_depend.c
+++ b/src/backend/catalog/pg_depend.c
@@ -116,7 +116,8 @@ recordMultipleDependencies(const ObjectAddress *depender,
 	referenced->objectId == POSIX_COLLATION_OID)
 	continue;
 
-version = get_collation_version_for_oid(referenced->objectId);
+version = get_collation_version_for_oid(referenced->objectId,
+		NULL);
 
 /*
  * Default collation is pinned, so we need to force recording
diff --git a/src/backend/commands/collationcmds.c b/src/backend/commands/collationcmds.c
index 9634ae6809..b3c59e6e9f 100644
--- a/src/backend/commands/collationcmds.c
+++ b/src/backend/commands/collationcmds.c
@@ -272,8 +272,14 @@ pg_collation_actual_version(PG_FUNCTION_ARGS)
 {
 	Oid			collid = PG_GETARG_OID(0);
 	char	   *version;
+	bool		found;
 
-	version = get_collation_version_for_oid(collid);
+	version = get_collation_version_for_oid(collid, );
+
+	if (!found)
+		ereport(ERROR,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("no collation found for OID %u", collid)));
 
 	if (version)
 		PG_RETURN_TEXT_P(cstring_to_text(version));
diff --git a/src/backend/utils/adt/pg_locale.c b/src/backend/utils/adt/pg_locale.c
index e9c1231f9b..3cd9257800 100644
--- a/src/backend/utils/adt/pg_locale.c
+++ b/src/backend/utils/adt/pg_locale.c
@@ -1726,10 +1726,12 @@ get_collation_actual_version(char collprovider, const char *collcollate)
 /*
  * Get provider-specific collation version string for a given collation OID.
  * Return NULL if the provider doesn't support versions, or the collation is
- * unversioned (for example "C").
+ * unversioned (for example "C").  If "found" is non-NULL, unknown OIDs are
+ * reported through this output parameter; otherwise, an internal error is
+ * raised for unknown OIDs.
  */
 char *
-get_collation_version_for_oid(Oid oid)
+get_collation_version_for_oid(Oid oid, bool *found)
 {
 	HeapTuple	tp;
 	char	   *version;
@@ -1744,6 +1746,8 @@ get_collation_version_for_oid(Oid oid)
 		dbform = (Form_pg_database) GETSTRUCT(tp);
 		version = get_collation_actual_version(COLLPROVIDER_LIBC,
 			   NameStr(dbform->datcollate));
+		if 

Re: Parallel INSERT (INTO ... SELECT ...)

[Greg Nancarrow]

On Wed, Feb 17, 2021 at 12:19 AM Amit Langote  wrote:
>
> On Mon, Feb 15, 2021 at 4:39 PM Greg Nancarrow  wrote:
> > On Sat, Feb 13, 2021 at 12:17 AM Amit Langote  
> > wrote:
> > > On Thu, Feb 11, 2021 at 4:43 PM Greg Nancarrow  
> > > wrote:
> > > > Actually, I tried adding the following in the loop that checks the
> > > > parallel-safety of each partition and it seemed to work:
> > > >
> > > > glob->relationOids =
> > > > lappend_oid(glob->relationOids, pdesc->oids[i]);
> > > >
> > > > Can you confirm, is that what you were referring to?
> > >
> > > Right.  I had mistakenly mentioned PlannerGlobal.invalItems, sorry.
> > >
> > > Although it gets the job done, I'm not sure if manipulating
> > > relationOids from max_parallel_hazard() or its subroutines is okay,
> > > but I will let the committer decide that.  As I mentioned above, the
> > > person who designed this decided for some reason that it is
> > > extract_query_dependencies()'s job to populate
> > > PlannerGlobal.relationOids/invalItems.
> >
> > Yes, it doesn't really seem right doing it within max_parallel_hazard().
> > I tried doing it in extract_query_dependencies() instead - see
> > attached patch - and it seems to work, but I'm not sure if there might
> > be any unintended side-effects.
>
> One issue I see with the patch is that it fails to consider
> multi-level partitioning, because it's looking up partitions only in
> the target table's PartitionDesc and no other.
>
> @@ -3060,8 +3066,36 @@ extract_query_dependencies_walker(Node *node,
> PlannerInfo *context)
> RangeTblEntry *rte = (RangeTblEntry *) lfirst(lc);
>
> if (rte->rtekind == RTE_RELATION)
> -   context->glob->relationOids =
> -   lappend_oid(context->glob->relationOids, rte->relid);
> +   {
> +   PlannerGlobal   *glob;
> +
> +   glob = context->glob;
> +   glob->relationOids =
> +   lappend_oid(glob->relationOids, rte->relid);
> +   if (query->commandType == CMD_INSERT &&
> +   rte->relkind == RELKIND_PARTITIONED_TABLE)
>
> The RTE whose relkind is being checked here may not be the INSERT
> target relation's RTE, even though that's perhaps always true today.
> So, I suggest to pull the new block out of the loop over rtable and
> perform its deeds on the result RTE explicitly fetched using
> rt_fetch(), preferably using a separate recursive function.  I'm
> thinking something like the attached revised version.
>
>

Thanks. Yes, I'd forgotten about the fact a partition may itself be
partitioned, so it needs to be recursive (like in the parallel-safety
checks).
Your revised version seems OK, though I do have a concern:
Is the use of "table_close(rel, NoLock)'' intentional? That will keep
the lock (lockmode) until end-of-transaction.

Regards,
Greg Nancarrow
Fujitsu Australia

Re: How to get Relation tuples in C function

[Andy Fan]

On Sun, Feb 14, 2021 at 7:56 PM Michael Paquier  wrote:

> On Sun, Feb 14, 2021 at 09:29:08AM +0800, Andy Fan wrote:
> > Thank you tom for the reply.  What would be the difference between the
> > SPI and "write a pure SQL UDF" and call it with DirectFunctionCall1? I
> > just ran into a similar situation some days before.   Currently I think
> > DirectFunctionCall1 doesn't need to maintain a connection but SPI has to
> > do that.
>
> Hard to say without knowing your use case.  A PL function is more
> simple to maintain than a C function, though usually less performant
> from the pure point of view of its operations.  A SQL function could
> finish by being inlined, allowing the planner to apply optimizations
> as it would know the function body.  Going with SPI has the advantage
> to have code able to work without any changes across major versions,
> which is a no-brainer when it comes to long-term maintenance.
> --
> Michael
>

Thank you Michael for the response.

-- 
Best Regards
Andy Fan (https://www.aliyun.com/)

Re: Support for NSS as a libpq TLS backend

[Jacob Champion]

On Mon, 2020-07-20 at 15:35 +0200, Daniel Gustafsson wrote:
> This version adds support for sslinfo on NSS for most the functions.

I've poked around to see what can be done about the
unimplemented ssl_client_dn_field/ssl_issuer_field functions. There's a
nasty soup of specs to wade around in, and it's not really clear to me
which ones take precedence since they're mostly centered on LDAP.

My take on it is that OpenSSL has done its own thing here, with almost-
based-on-a-spec-but-not-quite semantics. NSS has no equivalents to many
of the field names that OpenSSL supports (e.g. "commonName"). Likewise,
OpenSSL doesn't support case-insensitivity (e.g. "cn" in addition to
"CN") as many of the relevant RFCs require. They do both support
dotted-decimal representations, so we could theoretically get feature
parity there without a huge amount of work.

For the few attributes that NSS has a public API for retrieving:
- common name
- country
- locality
- state
- organization
- domain component
- org. unit
- DN qualifier
- uid
- email address(es?)
we could hardcode the list of OpenSSL-compatible names, and just
translate manually in sslinfo. Then leave the rest up to dotted-decimal 
OIDs.

Would that be desirable, or do we want this interface to be something
more generally compatible with (some as-of-yet unspecified) spec?

--Jacob

Re: TRIM_ARRAY

[Vik Fearing]

On 2/16/21 11:38 PM, Vik Fearing wrote:
> On 2/16/21 7:32 PM, Isaac Morland wrote:
>> On Tue, 16 Feb 2021 at 12:54, Vik Fearing  wrote:
>>
>>> The SQL standard defines a function called TRIM_ARRAY that surprisingly
>>> has syntax that looks like a function!  So I implemented it using a thin
>>> wrapper around our array slice syntax.  It is literally just ($1)[1:$2].
>>>
>>> An interesting case that I decided to handle by explaining it in the
>>> docs is that this won't give you the first n elements if your lower
>>> bound is not 1.  My justification for this is 1) non-standard lower
>>> bounds are so rare in the wild that 2) people using them can just not
>>> use this function.  The alternative is to go through the unnest dance
>>> (or write it in C) which defeats inlining.
>>>
>>
>> I don't recall ever seeing non-default lower bounds, so I actually think
>> it's OK to just rule out that scenario, but why not something like this:
>>
>> ($1)[:array_lower ($1, 1) + $2 - 1]
> 
> I'm kind of embarrassed that I didn't think about doing that; it is a
> much better solution.  You lose the non-standard bounds but I don't
> think there is any way besides C to keep the lower bound regardless of
> how you trim it.

I've made a bit of a mess out of this, but I partly blame the standard
which is very unclear.  It actually describes trimming the right n
elements instead of the left n like I've done here.  I'll be back later
with a better patch that does what it's actually supposed to.
-- 
Vik Fearing

Re: PATCH: Batch/pipelining support for libpq

[Zhihong Yu]

Hi,
+   if (querymode == QUERY_SIMPLE)
+   {
+   commandFailed(st, "startpipeline", "cannot use pipeline mode
with the simple query protocol");
+   st->state = CSTATE_ABORTED;
+   return CSTATE_ABORTED;

I wonder why the st->state is only assigned for this if block. The state is
not set for other cases where CSTATE_ABORTED is returned.

Should PQ_PIPELINE_OFF be returned for the following case ?

+PQpipelineStatus(const PGconn *conn)
+{
+   if (!conn)
+   return false;

Cheers

On Tue, Feb 16, 2021 at 3:14 PM Alvaro Herrera 
wrote:

> Here's a new version, where I've renamed everything to "pipeline".  I
> think the docs could use some additional tweaks now in order to make a
> coherent story on pipeline mode, how it can be used in a batched
> fashion, etc.
>
> Here's the renames I applied.  It's mostly mechanical, except
> PQbatchSendQueue is now PQsendPipeline:
>
> PQBatchStatus -> PGpipelineStatus (enum)
> PQBATCH_MODE_OFF -> PQ_PIPELINE_OFF
> PQBATCH_MODE_ON -> PQ_PIPELINE_ON
> PQBATCH_MODE_ABORTED -> PQ_PIPELINE_ABORTED
> PQbatchStatus -> PQpipelineStatus (function)
> PQenterBatchMode -> PQenterPipelineMode
> PQexitBatchMode -> PQexitPipelineMode
> PQbatchSendQueue -> PQsendPipeline
> PGRES_BATCH_END -> PGRES_PIPELINE_END
> PGRES_BATCH_ABORTED -> PGRES_PIPELINE_ABORTED
>
> Also, PQbatchStatus(conn) returns enum PGpipelineStatus (it previously
> returned int).
>
> I'm tempted to rename PGASYNC_QUEUED to PGASYNC_PIPELINE_IDLE (not sure
> if PGASYNC_PIPELINE_READY fits better with the existing one).
>
>
> In pgbench, I changed the metacommands to be \startpipeline and
> \endpipeline.  There's a failing Assert() there which I commented out;
> needs fixed.
>
> --
> Álvaro Herrera39°49'30"S 73°17'W
>

Re: PATCH: Batch/pipelining support for libpq

[Alvaro Herrera]

Here's a new version, where I've renamed everything to "pipeline".  I
think the docs could use some additional tweaks now in order to make a
coherent story on pipeline mode, how it can be used in a batched
fashion, etc.

Here's the renames I applied.  It's mostly mechanical, except
PQbatchSendQueue is now PQsendPipeline:

PQBatchStatus -> PGpipelineStatus (enum)
PQBATCH_MODE_OFF -> PQ_PIPELINE_OFF
PQBATCH_MODE_ON -> PQ_PIPELINE_ON
PQBATCH_MODE_ABORTED -> PQ_PIPELINE_ABORTED
PQbatchStatus -> PQpipelineStatus (function)
PQenterBatchMode -> PQenterPipelineMode
PQexitBatchMode -> PQexitPipelineMode
PQbatchSendQueue -> PQsendPipeline
PGRES_BATCH_END -> PGRES_PIPELINE_END
PGRES_BATCH_ABORTED -> PGRES_PIPELINE_ABORTED

Also, PQbatchStatus(conn) returns enum PGpipelineStatus (it previously
returned int).

I'm tempted to rename PGASYNC_QUEUED to PGASYNC_PIPELINE_IDLE (not sure
if PGASYNC_PIPELINE_READY fits better with the existing one).


In pgbench, I changed the metacommands to be \startpipeline and
\endpipeline.  There's a failing Assert() there which I commented out;
needs fixed.

-- 
Álvaro Herrera39°49'30"S 73°17'W
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index b7a82453f0..9f98257dbe 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -3099,6 +3099,31 @@ ExecStatusType PQresultStatus(const PGresult *res);

   
  
+
+ 
+  PGRES_PIPELINE_END
+  
+   
+The PGresult represents the end of a pipeline.
+This status occurs only when pipeline mode has been selected.
+   
+  
+ 
+
+ 
+  PGRES_PIPELINE_ABORTED
+  
+   
+The PGresult represents a pipeline that's
+received an error from the server.  PQgetResult
+must be called repeatedly, and it will return this status code,
+until the end of the current pipeline, at which point it will return
+PGRES_PIPELINE_END and normal processing can
+resume.
+   
+  
+ 
+
 
 
 If the result status is PGRES_TUPLES_OK or
@@ -4857,6 +4882,494 @@ int PQflush(PGconn *conn);
 
  
 
+ 
+  Pipeline Mode
+
+  
+   libpq
+   pipeline mode
+  
+
+  
+   pipelining
+   in libpq
+  
+
+  
+   batch mode
+   in libpq
+  
+
+  
+   libpq pipeline mode allows applications to
+   send a query without having to read the result of the previously
+   sent query.  Taking advantage of the pipeline mode, a client will wait
+   less for the server, since multiple queries/results can be sent/
+   received in a single network transaction.
+  
+
+  
+   While pipeline mode provides a significant performance boost, writing
+   clients using the pipeline mode is more complex because it involves
+   managing a queue of pending queries and finding which result
+   corresponds to which query in the queue.
+  
+
+  
+   Pipeline mode also generally consumes more memory on both the client and server,
+   though careful and aggressive management of the send/receive queue can mitigate
+   this.  This applies whether or not the connection is in blocking or non-blocking
+   mode.
+  
+
+  
+   Using Pipeline Mode
+
+   
+To issue pipelines the application must switch a connection into pipeline mode.
+Enter pipeline mode with 
+or test whether pipeline mode is active with
+.
+In pipeline mode, only asynchronous operations
+are permitted, and COPY is disallowed.
+Using any synchronous command execution functions,
+such as PQfn, or PQexec
+and its sibling functions, is an error condition.
+   
+
+   
+
+ It is best to use pipeline mode with libpq in
+ non-blocking mode. If used
+ in blocking mode it is possible for a client/server deadlock to occur.
+  
+   
+The client will block trying to send queries to the server, but the
+server will block trying to send results to the client from queries
+it has already processed. This only occurs when the client sends
+enough queries to fill its output buffer and the server's receive
+buffer before switching to processing input from the server,
+but it's hard to predict exactly when that will happen.
+   
+  
+
+   
+
+   
+Issuing Queries
+
+
+ After entering pipeline mode the application dispatches requests using
+ normal asynchronous libpq functions, such as:
+ PQsendQueryParams, PQsendPrepare,
+ PQsendQueryPrepared, PQsendDescribePortal,
+ and PQsendDescribePrepared.
+ The asynchronous requests are followed by a
+ 
+ call to mark the end of the pipeline. The client need not
+ call PQgetResult immediately after
+ dispatching each operation.
+ Result processing
+ is handled separately.
+
+
+
+ The 

Re: SSL SNI

[Jacob Champion]

On Mon, 2021-02-15 at 15:09 +0100, Peter Eisentraut wrote:
> The question I had was whether this should be an optional behavior, or 
> conversely a behavior that can be turned off, or whether it should just 
> be turned on all the time.

Personally I think there should be a toggle, so that any users for whom
hostnames are potentially sensitive don't have to make that information
available on the wire. Opt-in, to avoid having any new information
disclosure after a version upgrade?

> The Wikipedia page[1] discusses some privacy concerns in the context of 
> web browsing, but it seems there is no principled solution to those.

I think Encrypted Client Hello is the new-and-improved Encrypted SNI,
and it's on the very bleeding edge. You'd need to load a public key
into the client using some out-of-band communication -- e.g. browsers
would use DNS-over-TLS, but it might not make sense for a Postgres
client to use that same system.

NSS will probably be receiving any final implementation before OpenSSL,
if I had to guess, since Mozilla is driving pieces of the
implementation.

--Jacob

Re: Trigger execution role

[Andrew Dunstan]

On 2/16/21 3:59 PM, Isaac Morland wrote:
> On Fri, 12 Feb 2021 at 12:58, Tom Lane  > wrote:
>
> Isaac Morland  > writes:
> > I was trying to use triggers, and ran into something I hadn't
> realized
> > until now: triggers run, not as the owner of the table, but as
> the user who
> > is doing the insert/update/delete.
>
> If you don't want that, you can make the trigger function SECURITY
> DEFINER.  If we forced such behavior, there'd be no way to get the
> other behavior.
>
>
> I did think about SECURITY DEFINER, but that has at least a couple of
> significant downsides:
>
> - can't re-use the same generic trigger function for different table
> owners; would need to duplicate the function and use the one whose
> owner matches the table
> - other users could make the function a trigger for their tables and
> then invoke it unexpectedly (i.e., in a scenario I didn’t anticipate)
> - have to grant EXECUTE on the function to the same users that need
> permission to change the table contents
>
> In what scenarios is it needed for the trigger to run as the role
> doing the INSERT/UPDATE/DELETE? There are lots of scenarios where it
> doesn't matter — I can think of any number of constraint enforcement
> triggers that just compute a boolean and which could run as either —
> but I find it a lot easier to imagine a scenario where the table owner
> wants to do something when an INSERT/UPDATE/DELETE occurs than one in
> which the table owner wants to make sure the role changing the table
> does something.


One fairly obvious example is where the trigger is inserting audit data.
It needs to log the name of the user running the triggering statement
rather than the table owner.

TBH, I've used triggers very extensively over the years for a wide
variety of purposes and not found this to be a great issue.


>
> Additionally, with the present behaviour, what happens when I change a
> table's contents is completely unpredictable. A table could have a
> trigger on it which drops all my tables, to take an extreme example.
> If “I” am postgres then this could be problematic: it’s not safe for a
> privileged user to make changes to the contents of another role’s
> tables unless they are first verified to have no triggers on them (or,
> in theory, that the triggers are harmless, but I’ve been playing
> enough Baba is You lately to consider any judgement that the triggers
> are harmless to be worthless without a formally verified proof of same).


Well, that's true of any function no matter how it's invoked. If the
function is malicious it will do damage. If you suspect the database to
contain malicious trigger code, maybe disabling user triggers is in order.

Anyway, just speculating, but maybe there is a case for allowing running
a trigger as the table owner, as part of the trigger creation. Certainly
we're a very long way past the time when we could reasonably change the
default.


cheers


andrew


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

Re: TRIM_ARRAY

[Vik Fearing]

On 2/16/21 7:32 PM, Isaac Morland wrote:
> On Tue, 16 Feb 2021 at 12:54, Vik Fearing  wrote:
> 
>> The SQL standard defines a function called TRIM_ARRAY that surprisingly
>> has syntax that looks like a function!  So I implemented it using a thin
>> wrapper around our array slice syntax.  It is literally just ($1)[1:$2].
>>
>> An interesting case that I decided to handle by explaining it in the
>> docs is that this won't give you the first n elements if your lower
>> bound is not 1.  My justification for this is 1) non-standard lower
>> bounds are so rare in the wild that 2) people using them can just not
>> use this function.  The alternative is to go through the unnest dance
>> (or write it in C) which defeats inlining.
>>
> 
> I don't recall ever seeing non-default lower bounds, so I actually think
> it's OK to just rule out that scenario, but why not something like this:
> 
> ($1)[:array_lower ($1, 1) + $2 - 1]

I'm kind of embarrassed that I didn't think about doing that; it is a
much better solution.  You lose the non-standard bounds but I don't
think there is any way besides C to keep the lower bound regardless of
how you trim it.

V2 attached.
-- 
Vik Fearing
>From 21fb2060516a42b8b3edd1c9df3773ec8920d62b 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   | 17 +
 src/include/catalog/pg_proc.dat  |  4 
 src/test/regress/expected/arrays.out | 19 +++
 src/test/regress/sql/arrays.sql  | 16 
 4 files changed, 56 insertions(+)

diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 1ab31a9056..58ab7860f4 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -17916,6 +17916,23 @@ SELECT NULLIF(value, '(none)') ...

   
 
+  
+   
+
+ trim_array
+
+trim_array ( array anyarray, n integer )
+anyarray
+   
+   
+Trims an array to the first n elements.
+   
+   
+trim_array(ARRAY[1,2,3,4,5,6], 3)
+{1,2,3}
+   
+  
+
   

 
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 1487710d59..76e2030865 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -1674,6 +1674,10 @@
   proname => 'arraycontjoinsel', provolatile => 's', prorettype => 'float8',
   proargtypes => 'internal oid internal int2 internal',
   prosrc => 'arraycontjoinsel' },
+{ oid => '8819', descr => 'trim an array down to n elements',
+  proname => 'trim_array', prolang => 'sql', provolatile => 'i',
+  prorettype => 'anyarray', proargtypes => 'anyarray int4',
+  prosrc => 'select ($1)[:array_lower($1, 1) + $2 - 1]' },
 
 { oid => '764', descr => 'large object import',
   proname => 'lo_import', provolatile => 'v', proparallel => 'u',
diff --git a/src/test/regress/expected/arrays.out b/src/test/regress/expected/arrays.out
index 8bc7721e7d..3fb1b8dcef 100644
--- a/src/test/regress/expected/arrays.out
+++ b/src/test/regress/expected/arrays.out
@@ -2399,3 +2399,22 @@ SELECT width_bucket(5, ARRAY[3, 4, NULL]);
 ERROR:  thresholds array must not contain NULLs
 SELECT width_bucket(5, ARRAY[ARRAY[1, 2], ARRAY[3, 4]]);
 ERROR:  thresholds must be one-dimensional array
+-- trim_array
+CREATE TABLE trim_array_test (arr integer[]);
+INSERT INTO trim_array_test
+VALUES ('{1,2,3,4,5,6}'),
+   ('[-15:-10]={1,2,3,4,5,6}'),
+   ('[10:15]={1,2,3,4,5,6}'),
+   ('{{1,10},{2,20},{3,30},{4,40},{5,50},{6,60}}');
+SELECT arr, trim_array(arr, 3)
+FROM trim_array_test
+ORDER BY arr;
+ arr |   trim_array   
+-+
+ [-15:-10]={1,2,3,4,5,6} | {1,2,3}
+ {1,2,3,4,5,6}   | {1,2,3}
+ [10:15]={1,2,3,4,5,6}   | {1,2,3}
+ {{1,10},{2,20},{3,30},{4,40},{5,50},{6,60}} | {{1,10},{2,20},{3,30}}
+(4 rows)
+
+DROP TABLE trim_array_test;
diff --git a/src/test/regress/sql/arrays.sql b/src/test/regress/sql/arrays.sql
index c40619a8d5..6d34cc468e 100644
--- a/src/test/regress/sql/arrays.sql
+++ b/src/test/regress/sql/arrays.sql
@@ -722,3 +722,19 @@ SELECT width_bucket(5, '{}');
 SELECT width_bucket('5'::text, ARRAY[3, 4]::integer[]);
 SELECT width_bucket(5, ARRAY[3, 4, NULL]);
 SELECT width_bucket(5, ARRAY[ARRAY[1, 2], ARRAY[3, 4]]);
+
+
+-- trim_array
+
+CREATE TABLE trim_array_test (arr integer[]);
+INSERT INTO trim_array_test
+VALUES ('{1,2,3,4,5,6}'),
+   ('[-15:-10]={1,2,3,4,5,6}'),
+   ('[10:15]={1,2,3,4,5,6}'),
+   ('{{1,10},{2,20},{3,30},{4,40},{5,50},{6,60}}');
+
+SELECT arr, trim_array(arr, 3)
+FROM trim_array_test
+ORDER BY arr;
+
+DROP TABLE trim_array_test;
-- 
2.25.1

Re: [Patch] ALTER SYSTEM READ ONLY

[Robert Haas]

On Thu, Jan 28, 2021 at 7:17 AM Amul Sul  wrote:
> I am still on this. The things that worried me here are the wal records 
> sequence
> being written in the startup process -- UpdateFullPageWrites() generate record
> just before the recovery check-point record and XLogReportParameters() just
> after that but before any other backend could write any wal record. We might
> also need to follow the same sequence while changing the system to read-write.

I was able to chat with Andres about this topic for a while today and
he made some proposals which seemed pretty good to me. I can't promise
that what I'm about to write is an entirely faithful representation of
what he said, but hopefully it's not so far off that he gets mad at me
or something.

1. If the server starts up and is read-only and
ArchiveRecoveryRequested, clear the read-only state in memory and also
in the control file, log a message saying that this has been done, and
proceed. This makes some other cases simpler to deal with.

2. Create a new function with a name like XLogAcceptWrites(). Move the
following things from StartupXLOG() into that function: (1) the call
to UpdateFullPageWrites(), (2) the following block of code that does
either CreateEndOfRecoveryRecord() or RequestCheckpoint() or
CreateCheckPoint(), (3) the next block of code that runs
recovery_end_command, (4) the call to XLogReportParameters(), and (5)
the call to CompleteCommitTsInitialization(). Call the new function
from the place where we now call XLogReportParameters(). This would
mean that (1)-(3) happen later than they do now, which might require
some adjustments.

3. If the system is starting up read only (and the read-only state
didn't get cleared because of #1 above) then don't call
XLogAcceptWrites() at the end of StartupXLOG() and instead have the
checkpointer do it later when the system is going read-write for the
first time.

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

Re: Tid scan improvements

[Andres Freund]

Hi,

On 2021-02-04 23:51:39 +1300, David Rowley wrote:

> I ended up adding just two new API functions to table AM.
> 
> void (*scan_set_tid_range) (TableScanDesc sscan,
>ItemPointer mintid,
>ItemPointer maxtid);
> 
> and
> bool (*scan_tid_range_nextslot) (TableScanDesc sscan,
> ScanDirection direction,
> TupleTableSlot *slot);
> 
> I added an additional function in tableam.h that does not have a
> corresponding API function:
> 
> static inline TableScanDesc
> table_tid_range_start(Relation rel, Snapshot snapshot,
>   ItemPointer mintid,
>   ItemPointer maxtid)
> 
> This just calls the standard scan_begin then calls scan_set_tid_range
> setting the specified mintid and maxtid.

Hm. But that means we can't rescan?


> I also added 2 new fields to TableScanDesc:
> 
> ItemPointerData rs_mintid;
> ItemPointerData rs_maxtid;
> 
> I didn't quite see a need to have a new start and end scan API function.

Yea. I guess they're not that large. Avoiding that was one of the two
reasons to have a separate scan state somewhere. The other that it
seemed like it'd possibly a bit cleaner API wise to deal with rescan.


> +bool
> +heap_getnextslot_tidrange(TableScanDesc sscan, ScanDirection direction,
> +   TupleTableSlot *slot)
> +{
> + HeapScanDesc scan = (HeapScanDesc) sscan;
> + ItemPointer mintid = >rs_mintid;
> + ItemPointer maxtid = >rs_maxtid;
> +
> + /* Note: no locking manipulations needed */
> + for (;;)
> + {
> + if (sscan->rs_flags & SO_ALLOW_PAGEMODE)
> + heapgettup_pagemode(scan, direction, sscan->rs_nkeys, 
> sscan->rs_key);
> + else
> + heapgettup(scan, direction, sscan->rs_nkeys, 
> sscan->rs_key);
> +
> + if (scan->rs_ctup.t_data == NULL)
> + {
> + ExecClearTuple(slot);
> + return false;
> + }
> +
> + /*
> +  * heap_set_tidrange will have used heap_setscanlimits to limit 
> the
> +  * range of pages we scan to only ones that can contain the TID 
> range
> +  * we're scanning for.  Here we must filter out any tuples from 
> these
> +  * pages that are outwith that range.
> +  */
> + if (ItemPointerCompare(>rs_ctup.t_self, mintid) < 0)
> + {
> + ExecClearTuple(slot);
> +
> + /*
> +  * When scanning backwards, the TIDs will be in 
> descending order.
> +  * Future tuples in this direction will be lower still, 
> so we can
> +  * just return false to indicate there will be no more 
> tuples.
> +  */
> + if (ScanDirectionIsBackward(direction))
> + return false;
> +
> + continue;
> + }
> +
> + /*
> +  * Likewise for the final page, we must filter out TIDs greater 
> than
> +  * maxtid.
> +  */
> + if (ItemPointerCompare(>rs_ctup.t_self, maxtid) > 0)
> + {
> + ExecClearTuple(slot);
> +
> + /*
> +  * When scanning forward, the TIDs will be in ascending 
> order.
> +  * Future tuples in this direction will be higher 
> still, so we can
> +  * just return false to indicate there will be no more 
> tuples.
> +  */
> + if (ScanDirectionIsForward(direction))
> + return false;
> + continue;
> + }
> +
> + break;
> + }
> +
> + /*
> +  * if we get here it means we have a new current scan tuple, so point to
> +  * the proper return buffer and return the tuple.
> +  */
> + pgstat_count_heap_getnext(scan->rs_base.rs_rd);

I wonder if there's an argument for counting the misses above via
pgstat_count_heap_fetch()? Probably not, right?


> +#define IsCTIDVar(node)  \
> + ((node) != NULL && \
> +  IsA((node), Var) && \
> +  ((Var *) (node))->varattno == SelfItemPointerAttributeNumber && \
> +  ((Var *) (node))->varlevelsup == 0)
> +
> +typedef enum
> +{
> + TIDEXPR_UPPER_BOUND,
> + TIDEXPR_LOWER_BOUND
> +} TidExprType;
> +
> +/* Upper or lower range bound for scan */
> +typedef struct TidOpExpr
> +{
> + TidExprType exprtype;   /* type of op; lower or upper */
> + ExprState  *exprstate;  /* ExprState for a TID-yielding subexpr 
> */
> + boolinclusive;  /* whether op is inclusive */
> +} TidOpExpr;
> +
> +/*
> + * For the given 'expr', build and return an appropriate TidOpExpr taking 
> into
> + * account the expr's operator and operand order.
> + */
> +static TidOpExpr *
> +MakeTidOpExpr(OpExpr *expr, TidRangeScanState *tidstate)
> +{
> + Node   

Re: Trigger execution role

[Isaac Morland]

On Fri, 12 Feb 2021 at 12:58, Tom Lane  wrote:

> Isaac Morland  writes:
> > I was trying to use triggers, and ran into something I hadn't realized
> > until now: triggers run, not as the owner of the table, but as the user
> who
> > is doing the insert/update/delete.
>
> If you don't want that, you can make the trigger function SECURITY
> DEFINER.  If we forced such behavior, there'd be no way to get the
> other behavior.
>

I did think about SECURITY DEFINER, but that has at least a couple of
significant downsides:

- can't re-use the same generic trigger function for different table
owners; would need to duplicate the function and use the one whose owner
matches the table
- other users could make the function a trigger for their tables and then
invoke it unexpectedly (i.e., in a scenario I didn’t anticipate)
- have to grant EXECUTE on the function to the same users that need
permission to change the table contents

In what scenarios is it needed for the trigger to run as the role doing the
INSERT/UPDATE/DELETE? There are lots of scenarios where it doesn't matter —
I can think of any number of constraint enforcement triggers that just
compute a boolean and which could run as either — but I find it a lot
easier to imagine a scenario where the table owner wants to do something
when an INSERT/UPDATE/DELETE occurs than one in which the table owner wants
to make sure the role changing the table does something.

Additionally, with the present behaviour, what happens when I change a
table's contents is completely unpredictable. A table could have a trigger
on it which drops all my tables, to take an extreme example. If “I” am
postgres then this could be problematic: it’s not safe for a privileged
user to make changes to the contents of another role’s tables unless they
are first verified to have no triggers on them (or, in theory, that the
triggers are harmless, but I’ve been playing enough Baba is You lately to
consider any judgement that the triggers are harmless to be worthless
without a formally verified proof of same).

Re: 64-bit XIDs in deleted nbtree pages

[Peter Geoghegan]

On Tue, Feb 16, 2021 at 11:35 AM Peter Geoghegan  wrote:
> Isn't btvacuumcleanup() (or any other amvacuumcleanup() routine)
> entitled to rely on the bulk delete stats being set in the way I've
> described? I assumed that that was okay in general, but I haven't
> tested parallel VACUUM specifically. Will parallel VACUUM really fail
> to ensure that values in bulk stats fields (like pages_deleted and
> pages_free) get set correctly for amvacuumcleanup() callbacks?

I tested the pages_deleted_not_free stuff with a version of my patch
that consistently calls _bt_update_meta_cleanup_info() during
btvacuumcleanup(), and never during btbulkdelete(). And it works just
fine -- including with parallel VACUUM.

Evidently my understanding of what btvacuumcleanup() (or any other
amvacuumcleanup() routine) can expect from bulk delete stats was
correct. It doesn't matter whether or not parallel VACUUM happens to
be involved -- it works just as well.

This is good news, since of course it means that it's okay to stick to
the simple approach of calculating pages_deleted_not_free. Passing
pages_deleted_not_free (a.k.a. btm_last_cleanup_num_delpages) to
_bt_update_meta_cleanup_info() during btvacuumcleanup() works just as
well when combined with my fix for the the
"IndexVacuumInfo.num_heap_tuples is inaccurate during btbulkdelete()"
bug. That approach to fixing the IndexVacuumInfo.num_heap_tuples bug
creates no new problems for my patch. There is still no need to think
about when or how the relevant bulk delete fields (pages_deleted and
pages_free) were set. And it doesn't matter whether or not parallel
VACUUM is involved.

(Of course it's also true that we can't do that on the backbranches.
Purely because we must worry about btpo.xact/oldestBtpoXact on the
backbranches. We'll probably have to teach the code in released
versions to set btm_oldest_btpo_xact and
btm_last_cleanup_num_heap_tuples in separate calls -- since there is
no easy way to "send" the oldestBtpoXact value determined during a
btbulkdelete() to a later corresponding btvacuumcleanup(). That's a
bit of a kludge, but I'm not worried about it.)

-- 
Peter Geoghegan

Re: [PATCH] pg_hba.conf error messages for logical replication connections

[Paul Martinez]

On Tue, Feb 16, 2021 at 2:22 AM Amit Kapila  wrote:
>
> I don't think we need to update the error messages, it makes the code
> a bit difficult to parse without much benefit. How about just adding
> errdetail? See attached and let me know what you think?
>

Yeah, I think that looks good. Thanks!

- Paul

Re: 64-bit XIDs in deleted nbtree pages

[Peter Geoghegan]

On Tue, Feb 16, 2021 at 4:17 AM Masahiko Sawada  wrote:
> Ugh, yes, I think it's a bug.

I was actually thinking of a similar bug in nbtree deduplication when
I spotted this one -- see commit 48e12913. The index AM API stuff is
tricky.

> When developing this feature, in an old version patch, we used to set
> invalid values to both btm_oldest_btpo_xact and
> btm_last_cleanup_num_heap_tuples in btbulkdelete() to reset these
> values. But we decided to set valid values to both even in
> btbulkdelete(). I believe that decision was correct in terms of
> btm_oldest_btpo_xact because with the old version patch we will do an
> unnecessary index scan during btvacuumcleanup(). But it’s wrong in
> terms of btm_last_cleanup_num_heap_tuples, as you pointed out.

Right.

> This bug would make the check of vacuum_cleanup_index_scale_factor
> untrust. So I think it’s better to backpatch but I think we need to
> note that to fix this issue properly, in a case where a vacuum called
> btbulkdelete() earlier, probably we should update only
> btm_oldest_btpo_xact in btbulkdelete() and then update
> btm_last_cleanup_num_heap_tuples in btvacuumcleanup(). In this case,
> we don’t know the oldest btpo.xact among the deleted pages in
> btvacuumcleanup(). This means that we would need to update the meta
> page twice, leading to WAL logging twice. Since we already could
> update the meta page more than once when a vacuum calls btbulkdelete()
> multiple times I think it would not be a problem, though.

I agree that that approach is fine. Realistically, we won't even have
to update the metapage twice in most cases. Because most indexes never
have even one page deletion anyway.

> As I mentioned above, we might need to consider how btbulkdelete() can
> tell btvacuumcleanup() btm_last_cleanup_num_delpages in a case where a
> vacuum called btbulkdelete earlier. During parallel vacuum, two
> different processes could do btbulkdelete() and btvacuumcleanup()
> respectively. Updating those values separately in those callbacks
> would be straightforward.

I don't see why it should be a problem for my patch/Postgres 14,
because we don't have the same btpo.xact/oldestBtpoXact issue that the
original Postgres 11 commit dealt with. The patch determines a value
for btm_last_cleanup_num_delpages (which I call
pages_deleted_not_free) by subtracting fields from the bulk delete
stats: we just use "stats->pages_deleted - stats->pages_free".

Isn't btvacuumcleanup() (or any other amvacuumcleanup() routine)
entitled to rely on the bulk delete stats being set in the way I've
described? I assumed that that was okay in general, but I haven't
tested parallel VACUUM specifically. Will parallel VACUUM really fail
to ensure that values in bulk stats fields (like pages_deleted and
pages_free) get set correctly for amvacuumcleanup() callbacks?

-- 
Peter Geoghegan

Re: proposal: possibility to read dumped table's name from file

[Pavel Stehule]

Hi

rebase

Regards

Pavel
diff --git a/doc/src/sgml/ref/pg_dump.sgml b/doc/src/sgml/ref/pg_dump.sgml
index bcbb7a25fb..f24b3b5262 100644
--- a/doc/src/sgml/ref/pg_dump.sgml
+++ b/doc/src/sgml/ref/pg_dump.sgml
@@ -956,6 +956,42 @@ PostgreSQL documentation
   
  
 
+ 
+  --options-file=filename
+  
+   
+Read options from file (one option per line). Short or long options
+are supported. If you use "-" as a filename, the filters are read
+from stdin.
+   
+
+   
+With the following options file, the dump would include table
+mytable1 and data from foreign tables of
+some_foreign_server foreign server, but exclude data
+from table mytable2.
+
+-t mytable1
+--include-foreign-data=some_foreign_server
+--exclude-table-data=mytable2
+
+   
+
+   
+The text after symbol # is ignored. This can
+be used for comments, notes. Empty lines are ignored too.
+   
+
+   
+The option --options-file can be used more times,
+and the nesting is allowed. The options from options files are
+processed first, other options from command line later. Any option
+file is processed only one time. In next time the processing is
+ignored.
+   
+  
+ 
+
  
   --quote-all-identifiers
   
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index eb988d7eb4..17fef1fedf 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -54,9 +54,11 @@
 #include "catalog/pg_trigger_d.h"
 #include "catalog/pg_type_d.h"
 #include "common/connect.h"
+#include "common/string.h"
 #include "dumputils.h"
 #include "fe_utils/string_utils.h"
 #include "getopt_long.h"
+#include "lib/stringinfo.h"
 #include "libpq/libpq-fs.h"
 #include "parallel.h"
 #include "pg_backup_db.h"
@@ -123,18 +125,35 @@ static SimpleOidList tabledata_exclude_oids = {NULL, NULL};
 static SimpleStringList foreign_servers_include_patterns = {NULL, NULL};
 static SimpleOidList foreign_servers_include_oids = {NULL, NULL};
 
+static SimpleStringList optsfilenames_processed = {NULL, NULL};
+
 static const CatalogId nilCatalogId = {0, 0};
 
 /* override for standard extra_float_digits setting */
 static bool have_extra_float_digits = false;
 static int	extra_float_digits;
 
+static const char *filename = NULL;
+static const char *format = "p";
+static bool g_verbose = false;
+static const char *dumpencoding = NULL;
+static const char *dumpsnapshot = NULL;
+static char *use_role = NULL;
+static long rowsPerInsert;
+static int numWorkers = 1;
+static int compressLevel = -1;
+
 /*
  * The default number of rows per INSERT when
  * --inserts is specified without --rows-per-insert
  */
 #define DUMP_DEFAULT_ROWS_PER_INSERT 1
 
+/*
+ * Option's code of "options-file" option
+ */
+#define OPTIONS_FILE_OPTNUM 12
+
 /*
  * Macro for producing quoted, schema-qualified name of a dumpable object.
  */
@@ -296,14 +315,221 @@ static void appendReloptionsArrayAH(PQExpBuffer buffer, const char *reloptions,
 static char *get_synchronized_snapshot(Archive *fout);
 static void setupDumpWorker(Archive *AHX);
 static TableInfo *getRootTableInfo(const TableInfo *tbinfo);
+static bool read_options_from_file(const char *filename,
+   DumpOptions *dopt,
+   const char *optstring,
+   const struct option *longopts,
+   const char *progname);
+
+/*
+ * It assigns the values of options to related DumpOption fields or to
+ * some global values. Options file loading is not processed here.
+ */
+static bool
+process_option(int opt,
+			   char *optargstr,
+			   DumpOptions *dopt,
+			   const char *progname)
+{
+	char	   *endptr;
+
+	switch (opt)
+	{
+		case 'a':			/* Dump data only */
+			dopt->dataOnly = true;
+			break;
+
+		case 'b':			/* Dump blobs */
+			dopt->outputBlobs = true;
+			break;
+
+		case 'B':			/* Don't dump blobs */
+			dopt->dontOutputBlobs = true;
+			break;
+
+		case 'c':			/* clean (i.e., drop) schema prior to create */
+			dopt->outputClean = 1;
+			break;
+
+		case 'C':			/* Create DB */
+			dopt->outputCreateDB = 1;
+			break;
+
+		case 'd':			/* database name */
+			dopt->cparams.dbname = pg_strdup(optargstr);
+			break;
+
+		case 'E':			/* Dump encoding */
+			dumpencoding = pg_strdup(optargstr);
+			break;
+
+		case 'f':
+			filename = pg_strdup(optargstr);
+			break;
+
+		case 'F':
+			format = pg_strdup(optargstr);
+			break;
+
+		case 'h':			/* server host */
+			dopt->cparams.pghost = pg_strdup(optargstr);
+			break;
+
+		case 'j':			/* number of dump jobs */
+			numWorkers = atoi(optargstr);
+			break;
+
+		case 'n':			/* include schema(s) */
+			simple_string_list_append(_include_patterns, optargstr);
+			dopt->include_everything = false;
+			break;
+
+		case 'N':			/* exclude schema(s) */
+			simple_string_list_append(_exclude_patterns, optargstr);
+			break;
+
+		case 'O':			/* Don't reconnect to match owner */
+			dopt->outputNoOwner = 1;
+			

Re: PoC/WIP: Extended statistics on expressions

[Tomas Vondra]

On 1/27/21 12:02 PM, Dean Rasheed wrote:
> On Fri, 22 Jan 2021 at 03:49, Tomas Vondra
>  wrote:
>>
>> Whooops. A fixed version attached.
>>
> 
> The change to pg_stats_ext_exprs isn't quite right, because now it
> cross joins expressions and their stats, which leads to too many rows,
> with the wrong stats being listed against expressions. For example:
> 
> CREATE TABLE foo (a int, b text);
> INSERT INTO foo SELECT 1, 'xxx' FROM generate_series(1,1000);
> CREATE STATISTICS foo_s ON (a*10), upper(b) FROM foo;
> ANALYSE foo;
> 
> SELECT tablename, statistics_name, expr, most_common_vals
>   FROM pg_stats_ext_exprs;
> 
>  tablename | statistics_name |   expr   | most_common_vals
> ---+-+--+--
>  foo   | foo_s   | (a * 10) | {10}
>  foo   | foo_s   | (a * 10) | {XXX}
>  foo   | foo_s   | upper(b) | {10}
>  foo   | foo_s   | upper(b) | {XXX}
> (4 rows)
> 
> 
> More protection is still required for tables with no analysable
> columns. For example:
> 
> CREATE TABLE foo();
> CREATE STATISTICS foo_s ON (1) FROM foo;
> INSERT INTO foo SELECT FROM generate_series(1,1000);
> ANALYSE foo;
> 
> Program received signal SIGSEGV, Segmentation fault.
> 0x0090e9d4 in lookup_var_attr_stats (rel=0x7f7766b37598, attrs=0x0,
> exprs=0x216b258, nvacatts=0, vacatts=0x216cb40) at extended_stats.c:664
> 664stats[i]->tupDesc = vacatts[0]->tupDesc;
> 
> #0  0x0090e9d4 in lookup_var_attr_stats (rel=0x7f7766b37598,
> attrs=0x0, exprs=0x216b258, nvacatts=0, vacatts=0x216cb40)
> at extended_stats.c:664
> #1  0x0090da93 in BuildRelationExtStatistics (onerel=0x7f7766b37598,
> totalrows=1000, numrows=100, rows=0x216d040, natts=0,
> vacattrstats=0x216cb40) at extended_stats.c:161
> #2  0x0066ea97 in do_analyze_rel (onerel=0x7f7766b37598,
> params=0x7ffc06f7d450, va_cols=0x0,
> acquirefunc=0x66f71a , relpages=4, inh=false,
> in_outer_xact=false, elevel=13) at analyze.c:595
> 
> 
> Attached is an incremental update fixing those issues, together with a
> few more suggested improvements:
> 
> There was quite a bit of code duplication in extended_stats.c which I
> attempted to reduce by
> 
> 1). Deleting examine_opclause_expression() in favour of examine_clause_args().
> 2). Deleting examine_opclause_expression2() in favour of 
> examine_clause_args2().
> 3). Merging examine_clause_args() and examine_clause_args2(), renaming
> it examine_opclause_args() (which was actually the name it had in its
> original doc comment, despite the name in the code being different).
> 4). Merging statext_extract_expression() and
> statext_extract_expression_internal() into
> statext_is_compatible_clause() and
> statext_is_compatible_clause_internal() respectively.
> 
> That last change goes beyond just removing code duplication. It allows
> support for compound clauses that contain a mix of attribute and
> expression clauses, for example, this simple test case wasn't
> previously estimated well:
> 
> CREATE TABLE foo (a int, b int, c int);
> INSERT INTO foo SELECT x/100, x/100, x/100 FROM generate_series(1,1) g(x);
> CREATE STATISTICS foo_s on a,b,(c*c) FROM foo;
> ANALYSE foo;
> EXPLAIN ANALYSE SELECT * FROM foo WHERE a=1 AND (b=1 OR c*c=1);
> 
> I didn't add any new regression tests, but perhaps it would be worth
> adding something to test a case like that.
> 
> I changed choose_best_statistics() in a couple of ways. Firstly, I
> think it wants to only count expressions from fully covered clauses,
> just as we only count attributes if the stat covers all the attributes
> from a clause, since otherwise the stat cannot estimate the clause, so
> it shouldn't count. Secondly, I think the number of expressions in the
> stat needs to be added to it's number of keys, so that the choice of
> narrowest stat with the same number of matches counts expressions in
> the same way as attributes.
> 
> I simplified the code in statext_mcv_clauselist_selectivity(), by
> attempting to handle expressions and attributes together in the same
> way, making it much closer to the original code. I don't think that
> the check for the existence of a stat covering all the expressions in
> a clause was necessary when pre-processing the list of clauses, since
> that's checked later on, so it's enough to just detect compatible
> clauses. Also, it now checks for stats that cover both the attributes
> and the expressions from each clause, rather than one or the other, to
> cope with examples like the one above. I also updated the check for
> simple_clauses -- what's wanted there is to identify clauses that only
> reference a single column or a single expression, so that the later
> code doesn't apply multi-column estimates to it.
> 
> I'm attaching it as a incremental patch (0004) on top of your patches,
> but if 0003 and 0004 are collapsed together, the total number of diffs
> is less than 0003 alone.
> 

Thanks. All 

Re: TRIM_ARRAY

[Isaac Morland]

On Tue, 16 Feb 2021 at 12:54, Vik Fearing  wrote:

> The SQL standard defines a function called TRIM_ARRAY that surprisingly
> has syntax that looks like a function!  So I implemented it using a thin
> wrapper around our array slice syntax.  It is literally just ($1)[1:$2].
>
> An interesting case that I decided to handle by explaining it in the
> docs is that this won't give you the first n elements if your lower
> bound is not 1.  My justification for this is 1) non-standard lower
> bounds are so rare in the wild that 2) people using them can just not
> use this function.  The alternative is to go through the unnest dance
> (or write it in C) which defeats inlining.
>

I don't recall ever seeing non-default lower bounds, so I actually think
it's OK to just rule out that scenario, but why not something like this:

($1)[:array_lower ($1, 1) + $2 - 1]

Note that I've used the 9.6 feature that allows omitting the lower bound.

TRIM_ARRAY

[Vik Fearing]

The SQL standard defines a function called TRIM_ARRAY that surprisingly
has syntax that looks like a function!  So I implemented it using a thin
wrapper around our array slice syntax.  It is literally just ($1)[1:$2].

An interesting case that I decided to handle by explaining it in the
docs is that this won't give you the first n elements if your lower
bound is not 1.  My justification for this is 1) non-standard lower
bounds are so rare in the wild that 2) people using them can just not
use this function.  The alternative is to go through the unnest dance
(or write it in C) which defeats inlining.

Patch attached.
-- 
Vik Fearing
>From 6429316ab6060a16889b7c188ca577e17a5c7e4c 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   | 19 +++
 src/include/catalog/pg_proc.dat  |  4 
 src/test/regress/expected/arrays.out | 19 +++
 src/test/regress/sql/arrays.sql  | 16 
 4 files changed, 58 insertions(+)

diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 1ab31a9056..c3e157622f 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -17916,6 +17916,25 @@ SELECT NULLIF(value, '(none)') ...

   
 
+  
+   
+
+ trim_array
+
+trim_array ( array anyarray, n integer )
+anyarray
+   
+   
+Trims an array to the elements 1 through n.  Usually these are
+the first n elements of the array, but may not be if the lower
+bound is different from 1.
+   
+   
+select trim_array(ARRAY[1,2,3,4,5,6], 3)
+{1,2,3}
+   
+  
+
   

 
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 1487710d59..0aae4daf3b 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -1674,6 +1674,10 @@
   proname => 'arraycontjoinsel', provolatile => 's', prorettype => 'float8',
   proargtypes => 'internal oid internal int2 internal',
   prosrc => 'arraycontjoinsel' },
+{ oid => '8819', descr => 'trim an array down to n elements',
+  proname => 'trim_array', prolang => 'sql', provolatile => 'i',
+  prorettype => 'anyarray', proargtypes => 'anyarray int4',
+  prosrc => 'select ($1)[1:$2]' },
 
 { oid => '764', descr => 'large object import',
   proname => 'lo_import', provolatile => 'v', proparallel => 'u',
diff --git a/src/test/regress/expected/arrays.out b/src/test/regress/expected/arrays.out
index 8bc7721e7d..a79ad36cb0 100644
--- a/src/test/regress/expected/arrays.out
+++ b/src/test/regress/expected/arrays.out
@@ -2399,3 +2399,22 @@ SELECT width_bucket(5, ARRAY[3, 4, NULL]);
 ERROR:  thresholds array must not contain NULLs
 SELECT width_bucket(5, ARRAY[ARRAY[1, 2], ARRAY[3, 4]]);
 ERROR:  thresholds must be one-dimensional array
+-- trim_array
+CREATE TABLE trim_array_test (arr integer[]);
+INSERT INTO trim_array_test
+VALUES ('{1,2,3,4,5,6}'),
+   ('[-15:-10]={1,2,3,4,5,6}'),
+   ('[10:15]={1,2,3,4,5,6}'),
+   ('{{1,10},{2,20},{3,30},{4,40},{5,50},{6,60}}');
+SELECT arr, trim_array(arr, 3)
+FROM trim_array_test
+ORDER BY arr;
+ arr |   trim_array   
+-+
+ [-15:-10]={1,2,3,4,5,6} | {}
+ {1,2,3,4,5,6}   | {1,2,3}
+ [10:15]={1,2,3,4,5,6}   | {}
+ {{1,10},{2,20},{3,30},{4,40},{5,50},{6,60}} | {{1,10},{2,20},{3,30}}
+(4 rows)
+
+DROP TABLE trim_array_test;
diff --git a/src/test/regress/sql/arrays.sql b/src/test/regress/sql/arrays.sql
index c40619a8d5..6d34cc468e 100644
--- a/src/test/regress/sql/arrays.sql
+++ b/src/test/regress/sql/arrays.sql
@@ -722,3 +722,19 @@ SELECT width_bucket(5, '{}');
 SELECT width_bucket('5'::text, ARRAY[3, 4]::integer[]);
 SELECT width_bucket(5, ARRAY[3, 4, NULL]);
 SELECT width_bucket(5, ARRAY[ARRAY[1, 2], ARRAY[3, 4]]);
+
+
+-- trim_array
+
+CREATE TABLE trim_array_test (arr integer[]);
+INSERT INTO trim_array_test
+VALUES ('{1,2,3,4,5,6}'),
+   ('[-15:-10]={1,2,3,4,5,6}'),
+   ('[10:15]={1,2,3,4,5,6}'),
+   ('{{1,10},{2,20},{3,30},{4,40},{5,50},{6,60}}');
+
+SELECT arr, trim_array(arr, 3)
+FROM trim_array_test
+ORDER BY arr;
+
+DROP TABLE trim_array_test;
-- 
2.25.1

Re: proposal: schema variables

[Pavel Stehule]

Hi

út 2. 2. 2021 v 9:43 odesílatel Pavel Stehule 
napsal:

> Hi
>
> rebase and set PK for pg_variable table
>

rebase

Pavel


> Regards
>
> Pavel
>


schema-variables-20200216.patch.gz
Description: application/gzip

Re: Tid scan improvements

[David Fetter]

On Tue, Feb 16, 2021 at 10:22:41PM +1300, David Rowley wrote:
> On Thu, 4 Feb 2021 at 23:51, David Rowley  wrote:
> > Updated patch attached.
> 
> I made another pass over this patch and did a bit of renaming work
> around the heap_* functions and the tableam functions. I think the new
> names are a bit more aligned to the existing names.

Thanks! I'm looking forward to making use of this :)

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

Re: PATCH: Batch/pipelining support for libpq

[Alvaro Herrera]

On 2021-Feb-16, Craig Ringer wrote:

> FWIW I'm also thinking of revising the docs to mostly use the term
> "pipeline" instead of "batch". Use "pipelining and batching" in the chapter
> topic, and mention "batch" in the index, and add a para that explains how
> to run batches on top of pipelining, but otherwise use the term "pipeline"
> not "batch".

Hmm, this is a good point.  It means I have a lot of API renaming to do.
I'll get on it now and come back with a proposal.

-- 
Álvaro Herrera   Valdivia, Chile

Re: [Proposal] Page Compression for OLTP

[chenhj]

At 2021-02-16 21:51:14, "Daniel Gustafsson"  wrote:

>> On 16 Feb 2021, at 15:45, chenhj  wrote:
>
>> I want to know whether this patch can be accepted by the community, that is, 
>> whether it is necessary for me to continue working for this Patch. 
>> If you have any suggestions, please feedback to me.
>
>It doesn't seem like the patch has been registered in the commitfest app so it
>may have been forgotten about, the number of proposed patches often outnumber
>the code review bandwidth.  Please register it at:
>
>   https://commitfest.postgresql.org/32/
>
>..to make sure it doesn't get lost.
>
>--

>Daniel Gustafsson  https://vmware.com/


Thanks, I will complete this patch and registered it later.
Chen Huajun

Re: libpq PQresultErrorMessage vs PQerrorMessage API issue

[Tom Lane]

Craig Ringer  writes:
> This morning for the the umpteenth time I saw:
>  some error message: [blank here]
> output from a libpq program.

> That's because passing a NULL PGresult to PQgetResultErrorMessage() returns
> "". But a NULL PGresult is a normal result from PQexec when it fails to
> submit a query due to an invalid connection, when PQgetResult can't get a
> result from an invalid connection, etc.

How much of this is due to programmers not bothering to check whether
PQconnectXXX succeeded?  I do not think we need to go far out of our
way to cope with that scenario.

The idea of having a static PGresult that we can hand back to denote
out-of-memory scenarios is kind of cute.  But again, I wonder how
often the situation comes up in the real world.  It might be worth
doing just to have a more consistent API spec, though.  Particularly
for PQgetResult, where a NULL result has a defined, non-error meaning.

In general I doubt there's enough of a problem here to justify
inventing new or different APIs.  But if we can sand down some
rough edges without doing that, let's have a look.

regards, tom lane

Re: POC: postgres_fdw insert batching

[Tomas Vondra]

On 2/16/21 10:25 AM, Amit Langote wrote:

On Tue, Feb 16, 2021 at 1:36 AM Tomas Vondra
 wrote:

On 2/5/21 3:52 AM, Amit Langote wrote:

Tsunakwa-san,

On Mon, Jan 25, 2021 at 1:21 PM tsunakawa.ta...@fujitsu.com
 wrote:

From: Amit Langote 

Yes, it can be simplified by using a local join to prevent the update of the 
foreign
partition from being pushed to the remote server, for which my example in the
previous email used a local trigger.  Note that the update of the foreign
partition to be done locally is a prerequisite for this bug to occur.


Thank you, I was aware that UPDATE calls ExecInsert() but forgot about it 
partway.  Good catch (and my bad miss.)


It appears I had missed your reply, sorry.


+   PgFdwModifyState *fmstate = resultRelInfo->ri_FdwState ?
+   (PgFdwModifyState *) 
resultRelInfo->ri_FdwState :
+   NULL;

This can be written as:

+   PgFdwModifyState *fmstate = (PgFdwModifyState *) 
resultRelInfo->ri_FdwState;


Facepalm, yes.

Patch updated.  Thanks for the review.



Thanks for the patch, it seems fine to me.


Thanks for checking.


I wonder it the commit
message needs some tweaks, though. At the moment it says:

 Prevent FDW insert batching during cross-partition updates

but what the patch seems to be doing is simply initializing the info
only for CMD_INSERT operations. Which does the trick, but it affects
everything, i.e. all updates, no? Not just cross-partition updates.


You're right.  Please check the message in the updated patch.



Thanks. I'm not sure I understand what "FDW may not be able to handle 
both the original update operation and the batched insert operation 
being performed at the same time" means. I mean, if we translate the 
UPDATE into DELETE+INSERT, then we don't run both the update and insert 
at the same time, right? What exactly is the problem with allowing 
batching for inserts in cross-partition updates?


On a closer look, it seems the problem actually lies in a small 
inconsistency between create_foreign_modify and ExecInitRoutingInfo. The 
former only set batch_size for CMD_INSERT while the latter called the 
BatchSize() for all operations, expecting >= 1 result. So we may either 
relax create_foreign_modify and set batch_size for all DML, or make 
ExecInitRoutingInfo stricter (which is what the patches here do).


Is there a reason not to do the first thing, allowing batching of 
inserts during cross-partition updates? I tried to do that, but it 
dawned on me that we can't mix batched and un-batched operations, e.g. 
DELETE + INSERT, because that'd break the order of execution, leading to 
bogus results in case the same row is modified repeatedly, etc.


Am I getting this right?


regards

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

Re: Keep notnullattrs in RelOptInfo (Was part of UniqueKey patch series)

[Andy Fan]

On Tue, Feb 16, 2021 at 10:03 PM Andy Fan  wrote:

>
>
> On Tue, Feb 16, 2021 at 12:01 PM David Rowley 
> wrote:
>
>> On Fri, 12 Feb 2021 at 15:18, Andy Fan  wrote:
>> >
>> > On Fri, Feb 12, 2021 at 9:02 AM David Rowley 
>> wrote:
>> >> The reason I don't really like this is that it really depends where
>> >> you want to use RelOptInfo.notnullattrs.  If someone wants to use it
>> >> to optimise something before the base quals are evaluated then they
>> >> might be unhappy that they found some NULLs.
>> >>
>> >
>> > Do you mean the notnullattrs is not set correctly before the base quals
>> are
>> > evaluated?  I think we have lots of data structures which are set just
>> after some
>> > stage.  but notnullattrs is special because it is set at more than 1
>> stage.  However
>> > I'm doubtful it is unacceptable, Some fields ever change their meaning
>> at different
>> > stages like Var->varno.  If a user has a misunderstanding on it, it
>> probably will find it
>> > at the testing stage.
>>
>> You're maybe focusing too much on your use case for notnullattrs. It
>> only cares about NULLs in the result for each query level.
>>
>>  thinks of an example...
>>
>> OK, let's say I decided that COUNT(*) is faster than COUNT(id) so
>> decided that I might like to write a patch which rewrite the query to
>> use COUNT(*) when it was certain that "id" could not contain NULLs.
>>
>> The query is:
>>
>> SELECT p.partid, p.partdesc,COUNT(s.saleid) FROM part p LEFT OUTER
>> JOIN sales s ON p.partid = s.partid GROUP BY p.partid;
>>
>> sale.saleid is marked as NOT NULL in pg_attribute.  As the writer of
>> the patch, I checked the comment for notnullattrs and it says "Not
>> null attrs, start from -FirstLowInvalidHeapAttributeNumber", so I
>> should be ok to assume since sales.saleid is marked in notnullattrs
>> that I can rewrite the query?!
>>
>> The documentation about the RelOptInfo.notnullattrs needs to be clear
>> what exactly it means. I'm not saying your representation of how to
>> record NOT NULL in incorrect. I'm saying that you need to be clear
>> what exactly is being recorded in that field.
>>
>> If you want it to mean "attribute marked here cannot output NULL
>> values at this query level", then you should say something along those
>> lines.
>>
>
> I think I get what you mean. You are saying notnullattrs is only correct
> at the *current* stage, namely set_rel_size.  It would not be true after
> that,  but the data is still there.  That would cause some confusion.  I
> admit
> that is something I didn't realize before.  I checked other fields of
> RelOptInfo,
> looks no one filed works like this, so I am not really happy with this
> design
> now.  I'm OK with saying more things along these lines.  That can be done
> we all understand each other well.  Any better design is welcome as well.
> I think the  "Var represents null stuff" is good, until I see your
> comments below.
>
>
>
>> However, having said that, because this is a Bitmapset of
>> pg_attribute.attnums, it's only possible to record Vars from base
>> relations.  It does not seem like you have any means to record
>> attributes that are normally NULLable, but cannot produce NULL values
>> due to a strict join qual.
>>
>> e.g: SELECT t.nullable FROM t INNER JOIN j ON t.nullable = j.something;
>>
>> I'd expect the RelOptInfo for t not to contain a bit for the
>> "nullable" column, but there's no way to record the fact that the join
>> RelOptInfo for {t,j} cannot produce a NULL for that column. It might
>> be quite useful to know that for the UniqueKeys patch.
>>
>>
> The current patch can detect t.nullable is not null correctly. That
> is done by find_nonnullable_vars(qual) and deconstruct_recure stage.
>
>
>> I know there's another discussion here between Ashutosh and Tom about
>> PathTarget's and Vars.   I had the Var idea too once myself [1] but it
>> was quickly shot down.  Tom's reasoning there in [1] seems legit.  I
>> guess we'd need some sort of planner version of Var and never confuse
>> it with the Parse version of Var.  That sounds like quite a big
>> project which would have quite a lot of code churn. I'm not sure how
>> acceptable it would be to have Var represent both these things.  It
>> gets complex when you do equal(var1, var2) and expect that to return
>> true when everything matches apart from the notnull field. We
>> currently have this issue with the "location" field and we even have a
>> special macro which just ignores those in equalfuncs.c.  I imagine not
>> many people would like to expand that to other fields.
>>
>>
> Thanks for sharing this.
>
>
>> It would be good to agree on the correct representation for Vars that
>> cannot produce NULLs so that we don't shut the door on classes of
>> optimisation that require something other than what you need for your
>> case.
>>
>>
> Agreed.   The simplest way is just adding some comments.  If go a
> step further, how about just reset the notnullattrs when it is nullable
> later like 

How to customize postgres for sharing read-only tables in multiple data-dirs between servers

[Guttman, Maoz]

Hi,

Problem statement:
I have to develop a solution in which a single source populates a table. Once 
the table is populated, it is considered as read-only and then we run many 
read-only queries on it.
Such read-only tables are generated by multiple simulation runs: each 
simulation populates an independent table, meaning there is no cross-write to 
the tables.
However, the read-only queries can be executed on a single or multiple tables.
In my environment I have plenty of machines to run the simulations and I can’t 
use these machines to have a postgres compute farm as a cloud solution. So I 
can’t use my machines to run endless postgres server jobs as the solution is 
intended for.

My idea is:
Stage 1: Ad-hoc server+client for populating a table: start a server+client on 
a local machine, populate the table and stop the server+client. The data-dir is 
hosted in a central file system (e.g. NFS).
Stage 2: Ad-hoc server+client for querying the now read-only table(s) from step 
1:  start a server+client on a local machine, run read-only queries and stop 
the server+client.
In order to implement stage 2 I will:
1.Create a new ad-hoc empty data-dir
2.Create a soft-link from each data-dirtable files (including its 
index files) that is needed for the query to the ad-hoc data-dir.
Note that files in multiple data-dirs can be linked to the ad-hoc data-dir.
3.Update postgress catalog tables in the ad-hoc data-dir according 
to above soft-links
4.To guarantee that there will be no modifications of read-only 
table files, I will implement a table-am (access methods) which registers ONLY 
the table-am callback functions that are relevant for running read-only queries.
Since it is possible to run multiple queries on each table, there can be 
multiple instances of client-server describes in stage 2 running simultaneously.

Any thoughts?
Can it work?
My concern is for the process described in stage2#4: can I truly rely on 
callback functions running read-only queries do not update behind the scene the 
read-only table files?
Any other suggestion to develop & maintain a sustainable solution?

Thanks,
Maoz
-
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [Proposal] Page Compression for OLTP

[Daniel Gustafsson]

> On 16 Feb 2021, at 15:45, chenhj  wrote:

> I want to know whether this patch can be accepted by the community, that is, 
> whether it is necessary for me to continue working for this Patch. 
> If you have any suggestions, please feedback to me.

It doesn't seem like the patch has been registered in the commitfest app so it
may have been forgotten about, the number of proposed patches often outnumber
the code review bandwidth.  Please register it at:

https://commitfest.postgresql.org/32/

..to make sure it doesn't get lost.

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

Re: ERROR: invalid spinlock number: 0

[Fujii Masao]

On 2021/02/16 15:50, Michael Paquier wrote:

On Tue, Feb 16, 2021 at 12:43:42PM +0900, Fujii Masao wrote:

On 2021/02/16 6:28, Andres Freund wrote:

So what? It's just about free to initialize a spinlock, whether it's
using the fallback implementation or not. Initializing upon walsender
startup adds a lot of complications, because e.g. somebody could already
hold the spinlock because the previous walsender just disconnected, and
they were looking at the stats.


Okay.


Even if we initialize "writtenUpto" in WalRcvShmemInit(), WalReceiverMain()
still needs to initialize (reset to 0) by using pg_atomic_write_u64().


Yes, you have to do that.


Basically we should not acquire new spinlock while holding another spinlock,
to shorten the spinlock duration. Right? If yes, we need to move
pg_atomic_read_u64() of "writtenUpto" after the release of spinlock in
pg_stat_get_wal_receiver.


It would not matter much as a NULL tuple is returned as long as the
WAL receiver information is not ready to be displayed.  The only
reason why all the fields are read before checking for
ready_to_display is that we can be sure that everything is consistent
with the PID.  So reading writtenUpto before or after does not really
matter logically.  I would just move it after the check, as you did
previously.


OK.



+   /*
+* Read "writtenUpto" without holding a spinlock. So it may not be
+* consistent with other WAL receiver's shared variables protected by a
+* spinlock. This is OK because that variable is used only for
+* informational purpose and should not be used for data integrity checks.
+*/
What about the following?
"Read "writtenUpto" without holding a spinlock.  Note that it may not
be consistent with the other shared variables of the WAL receiver
protected by a spinlock, but this should not be used for data
integrity checks."


Sounds good. Attached is the updated version of the patch.



I agree that what has been done with MyProc->waitStart in 46d6e5f is
not safe, and that initialization should happen once at postmaster
startup, with a write(0) when starting the backend.  There are two of
them in proc.c, one in twophase.c.  Do you mind if I add an open item
for this one?


Yeah, please feel free to do that! BTW, I already posted the patch
addressing that issue, at [1].

[1] https://postgr.es/m/1df88660-6f08-cc6e-b7e2-f85296a2b...@oss.nttdata.com

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
diff --git a/src/backend/replication/walreceiver.c 
b/src/backend/replication/walreceiver.c
index 723f513d8b..9ec71238c4 100644
--- a/src/backend/replication/walreceiver.c
+++ b/src/backend/replication/walreceiver.c
@@ -249,7 +249,7 @@ WalReceiverMain(void)
 
SpinLockRelease(>mutex);
 
-   pg_atomic_init_u64(>writtenUpto, 0);
+   pg_atomic_write_u64(>writtenUpto, 0);
 
/* Arrange to clean up at walreceiver exit */
on_shmem_exit(WalRcvDie, 0);
@@ -1325,7 +1325,6 @@ pg_stat_get_wal_receiver(PG_FUNCTION_ARGS)
state = WalRcv->walRcvState;
receive_start_lsn = WalRcv->receiveStart;
receive_start_tli = WalRcv->receiveStartTLI;
-   written_lsn = pg_atomic_read_u64(>writtenUpto);
flushed_lsn = WalRcv->flushedUpto;
received_tli = WalRcv->receivedTLI;
last_send_time = WalRcv->lastMsgSendTime;
@@ -1345,6 +1344,14 @@ pg_stat_get_wal_receiver(PG_FUNCTION_ARGS)
if (pid == 0 || !ready_to_display)
PG_RETURN_NULL();
 
+   /*
+* Read "writtenUpto" without holding a spinlock.  Note that it may not 
be
+* consistent with the other shared variables of the WAL receiver
+* protected by a spinlock, but this should not be used for data 
integrity
+* checks.
+*/
+   written_lsn = pg_atomic_read_u64(>writtenUpto);
+
/* determine result type */
if (get_call_result_type(fcinfo, NULL, ) != TYPEFUNC_COMPOSITE)
elog(ERROR, "return type must be a row type");
diff --git a/src/backend/replication/walreceiverfuncs.c 
b/src/backend/replication/walreceiverfuncs.c
index 69b91a7dab..63e60478ea 100644
--- a/src/backend/replication/walreceiverfuncs.c
+++ b/src/backend/replication/walreceiverfuncs.c
@@ -63,6 +63,7 @@ WalRcvShmemInit(void)
MemSet(WalRcv, 0, WalRcvShmemSize());
WalRcv->walRcvState = WALRCV_STOPPED;
SpinLockInit(>mutex);
+   pg_atomic_init_u64(>writtenUpto, 0);
WalRcv->latch = NULL;
}
 }
diff --git a/src/test/regress/expected/sysviews.out 
b/src/test/regress/expected/sysviews.out
index 81bdacf59d..6d048e309c 100644
--- a/src/test/regress/expected/sysviews.out
+++ b/src/test/regress/expected/sysviews.out
@@ -83,6 +83,13 @@ select count(*) = 1 as ok from pg_stat_wal;
  t
 (1 row)
 
+-- We expect no walreceiver running in this test
+select count(*) = 0 as ok from pg_stat_wal_receiver;
+ ok 
+
+ t

Re: [Proposal] Page Compression for OLTP

[chenhj]

Hi, hackers


I want to know whether this patch can be accepted by the community, that is, 
whether it is necessary for me to continue working for this Patch. 
If you have any suggestions, please feedback to me.


Best Regards
Chen Huajun

Re: [HACKERS] Custom compression methods

[Dilip Kumar]

On Sat, Feb 13, 2021 at 8:14 PM Dilip Kumar  wrote:
>
> On Thu, Feb 11, 2021 at 8:17 PM Robert Haas  wrote:
> >
> > On Thu, Feb 11, 2021 at 7:36 AM Dilip Kumar  wrote:
> > > W.R.T the attached patch, In HeapTupleHeaderGetDatum, we don't even
> > > attempt to detoast if there is no external field in the tuple, in POC
> > > I have got rid of that check, but I think we might need to do better.
> > > Maybe we can add a flag in infomask to detect whether the tuple has
> > > any compressed data or not as we have for detecting the external data
> > > (HEAP_HASEXTERNAL).
> >
> > No. This feature isn't close to being important enough to justify
> > consuming an infomask bit.
>
> Okay,
>
> > I don't really see why we need it anyway. If array construction
> > already categorically detoasts, why can't we do the same thing here?
> > Would it really cost that much? In what case? Having compressed values
> > in a record we're going to store on disk actually seems like a pretty
> > dumb idea. We might end up trying to recompress something parts of
> > which have already been compressed.
> >
>
> If we refer the comments atop function "toast_flatten_tuple_to_datum"
>
> ---
> *We have a general rule that Datums of container types (rows, arrays,
>  *ranges, etc) must not contain any external TOAST pointers.  Without
>  *this rule, we'd have to look inside each Datum when preparing a tuple
>  *for storage, which would be expensive and would fail to extend cleanly
>  *to new sorts of container types.
>  *
>  *However, we don't want to say that tuples represented as HeapTuples
>  *can't contain toasted fields, so instead this routine should be called
>  *when such a HeapTuple is being converted into a Datum.
>  *
>  *While we're at it, we decompress any compressed fields too.  This is not
>  *necessary for correctness, but reflects an expectation that compression
>  *will be more effective if applied to the whole tuple not individual
>  *fields.  We are not so concerned about that that we want to deconstruct
>  *and reconstruct tuples just to get rid of compressed fields, however.
>  *So callers typically won't call this unless they see that the tuple has
>  *at least one external field.
> 
>
> It appears that the general rule we want to follow is that while
> creating the composite type we want to flatten any external pointer,
> but while doing that we also decompress any compressed field  with the
> assumption that compressing the whole row/array will be a better idea
> instead of keeping them compressed individually.  However, if there
> are no external toast pointers then we don't want to make an effort to
> just decompress the compressed data.
>
> Having said that I don't think this rule is followed throughout the
> code for example
>
> 1. "ExecEvalRow" is calling HeapTupleHeaderGetDatum only if there is
> any external field and which is calling "toast_flatten_tuple_to_datum"
> so this is following the rule.
> 2. "ExecEvalWholeRowVar" is calling "toast_build_flattened_tuple", but
> this is just flattening the external toast pointer but not doing
> anything to the compressed data.
> 3. "ExecEvalArrayExpr" is calling "construct_md_array", which will
> detoast the attribute if attlen is -1, so this will decompress any
> compressed data even though there is no external toast pointer.
>
> So in 1 we are following the rule but in 2 and 3 we are not.
>
> IMHO, for the composite data types we should make common a rule and we
> should follow that everywhere.   As you said it will be good if we can
> always detoast any external/compressed data, that will help in getting
> better compression as well as fetching the data will be faster because
> we can avoid multi level detoasting/decompression.  I will analyse
> this further and post a patch for the same.

I have done further analysis of this issue and came up with the
attached patch.  So with this patch, like external toast posiners we
will not allow any compressed data also in the composite types.  The
problem is that now we will be processing all the tuple while forming
the composite type irrespective of the source of the tuple, I mean if
user is directly inserting into the array type and not selecting from
another table then there will not be any compressed data so now
checking each field of tuple for compressed data is unnecessary but I
am not sure how to distinguish between those cases.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
From 59a8520a5278c43f712315035ed2261d83e310ca Mon Sep 17 00:00:00 2001
From: Dilip Kumar 
Date: Tue, 16 Feb 2021 19:24:38 +0530
Subject: [PATCH v1] Disallow compressed data inside container types

Currently, we have a general rule that Datums of container types
(rows, arrays, ranges, etc) must not contain any external TOAST
pointers.  But the rule for the compressed data is not defined
and no specific rule is followed e.g. while constructing 

Re: Performing partition pruning using row value

[Anastasia Lubennikova]

On 21.07.2020 11:24, kato-...@fujitsu.com wrote:

So, after looking at these functions and modifying this patch, I would like to 
add this patch to the next

I updated this patch and registered for the next CF .

https://commitfest.postgresql.org/29/2654/

regards,
sho kato


Thank you for working on this improvement. I took a look at the code.

1) This piece of code is unneeded:

            switch (get_op_opfamily_strategy(opno, partopfamily))
            {
                case BTLessStrategyNumber:
                case BTLessEqualStrategyNumber:
                case BTGreaterEqualStrategyNumber:
                case BTGreaterStrategyNumber:

See the comment for RowCompareExpr, which states that "A RowCompareExpr 
node is only generated for the < <= > >= cases".


2) It's worth to add a regression test for this feature.

Other than that, the patch looks good to me.

--
Anastasia Lubennikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: Keep notnullattrs in RelOptInfo (Was part of UniqueKey patch series)

[Andy Fan]

On Tue, Feb 16, 2021 at 12:01 PM David Rowley  wrote:

> On Fri, 12 Feb 2021 at 15:18, Andy Fan  wrote:
> >
> > On Fri, Feb 12, 2021 at 9:02 AM David Rowley 
> wrote:
> >> The reason I don't really like this is that it really depends where
> >> you want to use RelOptInfo.notnullattrs.  If someone wants to use it
> >> to optimise something before the base quals are evaluated then they
> >> might be unhappy that they found some NULLs.
> >>
> >
> > Do you mean the notnullattrs is not set correctly before the base quals
> are
> > evaluated?  I think we have lots of data structures which are set just
> after some
> > stage.  but notnullattrs is special because it is set at more than 1
> stage.  However
> > I'm doubtful it is unacceptable, Some fields ever change their meaning
> at different
> > stages like Var->varno.  If a user has a misunderstanding on it, it
> probably will find it
> > at the testing stage.
>
> You're maybe focusing too much on your use case for notnullattrs. It
> only cares about NULLs in the result for each query level.
>
>  thinks of an example...
>
> OK, let's say I decided that COUNT(*) is faster than COUNT(id) so
> decided that I might like to write a patch which rewrite the query to
> use COUNT(*) when it was certain that "id" could not contain NULLs.
>
> The query is:
>
> SELECT p.partid, p.partdesc,COUNT(s.saleid) FROM part p LEFT OUTER
> JOIN sales s ON p.partid = s.partid GROUP BY p.partid;
>
> sale.saleid is marked as NOT NULL in pg_attribute.  As the writer of
> the patch, I checked the comment for notnullattrs and it says "Not
> null attrs, start from -FirstLowInvalidHeapAttributeNumber", so I
> should be ok to assume since sales.saleid is marked in notnullattrs
> that I can rewrite the query?!
>
> The documentation about the RelOptInfo.notnullattrs needs to be clear
> what exactly it means. I'm not saying your representation of how to
> record NOT NULL in incorrect. I'm saying that you need to be clear
> what exactly is being recorded in that field.
>
> If you want it to mean "attribute marked here cannot output NULL
> values at this query level", then you should say something along those
> lines.
>

I think I get what you mean. You are saying notnullattrs is only correct
at the *current* stage, namely set_rel_size.  It would not be true after
that,  but the data is still there.  That would cause some confusion.  I
admit
that is something I didn't realize before.  I checked other fields of
RelOptInfo,
looks no one filed works like this, so I am not really happy with this
design
now.  I'm OK with saying more things along these lines.  That can be done
we all understand each other well.  Any better design is welcome as well.
I think the  "Var represents null stuff" is good, until I see your comments
below.



> However, having said that, because this is a Bitmapset of
> pg_attribute.attnums, it's only possible to record Vars from base
> relations.  It does not seem like you have any means to record
> attributes that are normally NULLable, but cannot produce NULL values
> due to a strict join qual.
>
> e.g: SELECT t.nullable FROM t INNER JOIN j ON t.nullable = j.something;
>
> I'd expect the RelOptInfo for t not to contain a bit for the
> "nullable" column, but there's no way to record the fact that the join
> RelOptInfo for {t,j} cannot produce a NULL for that column. It might
> be quite useful to know that for the UniqueKeys patch.
>
>
The current patch can detect t.nullable is not null correctly. That
is done by find_nonnullable_vars(qual) and deconstruct_recure stage.


> I know there's another discussion here between Ashutosh and Tom about
> PathTarget's and Vars.   I had the Var idea too once myself [1] but it
> was quickly shot down.  Tom's reasoning there in [1] seems legit.  I
> guess we'd need some sort of planner version of Var and never confuse
> it with the Parse version of Var.  That sounds like quite a big
> project which would have quite a lot of code churn. I'm not sure how
> acceptable it would be to have Var represent both these things.  It
> gets complex when you do equal(var1, var2) and expect that to return
> true when everything matches apart from the notnull field. We
> currently have this issue with the "location" field and we even have a
> special macro which just ignores those in equalfuncs.c.  I imagine not
> many people would like to expand that to other fields.
>
>
Thanks for sharing this.


> It would be good to agree on the correct representation for Vars that
> cannot produce NULLs so that we don't shut the door on classes of
> optimisation that require something other than what you need for your
> case.
>
>
Agreed.   The simplest way is just adding some comments.  If go a
step further, how about just reset the notnullattrs when it is nullable
later like outer join?  I have added this logic in the attached patch.
(comment for the notnullattrs is still not touched).  I think we only
need to handle this in build_join_rel stage.  With the v2 

Re: A reloption for partitioned tables - parallel_workers

[Laurenz Albe]

On Tue, 2021-02-16 at 16:29 +0900, Amit Langote wrote:
> > I am +1 on allowing to override the degree of parallelism on a parallel
> > append.  If "parallel_workers" on the partitioned table is an option for
> > that, it might be a simple solution.  On the other hand, perhaps it would
> > be less confusing to have a different storage parameter name rather than
> > having "parallel_workers" do double duty.
> > Also, since there is a design rule that storage parameters can only be used
> > on partitions, we would have to change that - is that a problem for anybody?
> 
> I am not aware of a rule that suggests that parallel_workers is always
> interpreted using storage-level considerations.  If that is indeed a
> popular interpretation at this point, then yes, we should be open to
> considering a new name for the parameter that this patch wants to add.

Well, 
https://www.postgresql.org/docs/current/sql-createtable.html#SQL-CREATETABLE-STORAGE-PARAMETERS
says:

 "Specifying these parameters for partitioned tables is not supported,
  but you may specify them for individual leaf partitions."

If we re-purpose "parallel_workers" like this, we'd have to change this.

Then for a normal table, "parallel_workers" would mean how many workers
work on a parallel table scan.  For a partitioned table, it determines
how many workers work on a parallel append.

Perhaps that is similar enough that it is not confusing.

Yours,
Laurenz Albe

Re: Parallel INSERT (INTO ... SELECT ...)

[Amit Langote]

On Mon, Feb 15, 2021 at 4:39 PM Greg Nancarrow  wrote:
> On Sat, Feb 13, 2021 at 12:17 AM Amit Langote  wrote:
> > On Thu, Feb 11, 2021 at 4:43 PM Greg Nancarrow  wrote:
> > > Actually, I tried adding the following in the loop that checks the
> > > parallel-safety of each partition and it seemed to work:
> > >
> > > glob->relationOids =
> > > lappend_oid(glob->relationOids, pdesc->oids[i]);
> > >
> > > Can you confirm, is that what you were referring to?
> >
> > Right.  I had mistakenly mentioned PlannerGlobal.invalItems, sorry.
> >
> > Although it gets the job done, I'm not sure if manipulating
> > relationOids from max_parallel_hazard() or its subroutines is okay,
> > but I will let the committer decide that.  As I mentioned above, the
> > person who designed this decided for some reason that it is
> > extract_query_dependencies()'s job to populate
> > PlannerGlobal.relationOids/invalItems.
>
> Yes, it doesn't really seem right doing it within max_parallel_hazard().
> I tried doing it in extract_query_dependencies() instead - see
> attached patch - and it seems to work, but I'm not sure if there might
> be any unintended side-effects.

One issue I see with the patch is that it fails to consider
multi-level partitioning, because it's looking up partitions only in
the target table's PartitionDesc and no other.

@@ -3060,8 +3066,36 @@ extract_query_dependencies_walker(Node *node,
PlannerInfo *context)
RangeTblEntry *rte = (RangeTblEntry *) lfirst(lc);

if (rte->rtekind == RTE_RELATION)
-   context->glob->relationOids =
-   lappend_oid(context->glob->relationOids, rte->relid);
+   {
+   PlannerGlobal   *glob;
+
+   glob = context->glob;
+   glob->relationOids =
+   lappend_oid(glob->relationOids, rte->relid);
+   if (query->commandType == CMD_INSERT &&
+   rte->relkind == RELKIND_PARTITIONED_TABLE)

The RTE whose relkind is being checked here may not be the INSERT
target relation's RTE, even though that's perhaps always true today.
So, I suggest to pull the new block out of the loop over rtable and
perform its deeds on the result RTE explicitly fetched using
rt_fetch(), preferably using a separate recursive function.  I'm
thinking something like the attached revised version.



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


setrefs-v2.patch
Description: Binary data

Re: 64-bit XIDs in deleted nbtree pages

[Masahiko Sawada]

On Tue, Feb 16, 2021 at 3:52 PM Peter Geoghegan  wrote:
>
> On Mon, Feb 15, 2021 at 7:26 PM Peter Geoghegan  wrote:
> > Actually, there is one more reason why I bring up idea 1 now: I want
> > to hear your thoughts on the index AM API questions now, which idea 1
> > touches on. Ideally all of the details around the index AM VACUUM APIs
> > (i.e. when and where the extra work happens -- btvacuumcleanup() vs
> > btbulkdelete()) won't need to change much in the future. I worry about
> > getting this index AM API stuff right, at least a little.
>
> Speaking of problems like this, I think I spotted an old one: we call
> _bt_update_meta_cleanup_info() in either btbulkdelete() or
> btvacuumcleanup(). I think that we should always call it in
> btvacuumcleanup(), though -- even in cases where there is no call to
> btvacuumscan() inside btvacuumcleanup() (because btvacuumscan()
> happened earlier instead, during the btbulkdelete() call).
>
> This makes the value of IndexVacuumInfo.num_heap_tuples (which is what
> we store in the metapage) much more accurate -- right now it's always
> pg_class.reltuples from *before* the VACUUM started. And so the
> btm_last_cleanup_num_heap_tuples value in a nbtree metapage is often
> kind of inaccurate.
>
> This "estimate during ambulkdelete" issue is documented here (kind of):
>
> /*
>  * Struct for input arguments passed to ambulkdelete and amvacuumcleanup
>  *
>  * num_heap_tuples is accurate only when estimated_count is false;
>  * otherwise it's just an estimate (currently, the estimate is the
>  * prior value of the relation's pg_class.reltuples field, so it could
>  * even be -1).  It will always just be an estimate during ambulkdelete.
>  */
> typedef struct IndexVacuumInfo
> {
> ...
> }
>
> The name of the metapage field is already
> btm_last_cleanup_num_heap_tuples, which already suggests the approach
> that I propose now. So why don't we do it like that already?
>
> (Thinks some more...)
>
> I wonder: did this detail change at the last minute during the
> development of the feature (just before commit 857f9c36) back in early
> 2018? That change have made it easier to deal with
> oldestBtpoXact/btm_oldest_btpo_xact, which IIRC was a late addition to
> the patch -- so maybe it's truly an accident that the code doesn't
> work the way that I suggest it should already. (It's annoying to make
> state from btbulkdelete() appear in btvacuumcleanup(), unless it's
> from IndexVacuumInfo or something -- I can imagine this changing at
> the last minute, just for that reason.)
>
> Do you think that this needs to be treated as a bug in the
> backbranches, Masahiko? I'm not sure...

Ugh, yes, I think it's a bug.

When developing this feature, in an old version patch, we used to set
invalid values to both btm_oldest_btpo_xact and
btm_last_cleanup_num_heap_tuples in btbulkdelete() to reset these
values. But we decided to set valid values to both even in
btbulkdelete(). I believe that decision was correct in terms of
btm_oldest_btpo_xact because with the old version patch we will do an
unnecessary index scan during btvacuumcleanup(). But it’s wrong in
terms of btm_last_cleanup_num_heap_tuples, as you pointed out.

This bug would make the check of vacuum_cleanup_index_scale_factor
untrust. So I think it’s better to backpatch but I think we need to
note that to fix this issue properly, in a case where a vacuum called
btbulkdelete() earlier, probably we should update only
btm_oldest_btpo_xact in btbulkdelete() and then update
btm_last_cleanup_num_heap_tuples in btvacuumcleanup(). In this case,
we don’t know the oldest btpo.xact among the deleted pages in
btvacuumcleanup(). This means that we would need to update the meta
page twice, leading to WAL logging twice. Since we already could
update the meta page more than once when a vacuum calls btbulkdelete()
multiple times I think it would not be a problem, though.

>
> In any case we should probably make this change as part of Postgres
> 14. Don't you think? It's certainly easy to do it this way now, since
> there will be no need to keep around a oldestBtpoXact value until
> btvacuumcleanup() (in the common case where btbulkdelete() is where we
> call btvacuumscan()). The new btm_last_cleanup_num_delpages field
> (which replaces btm_oldest_btpo_xact) has a value that just comes from
> the bulk stats, which is easy anyway.

Agreed.

As I mentioned above, we might need to consider how btbulkdelete() can
tell btvacuumcleanup() btm_last_cleanup_num_delpages in a case where a
vacuum called btbulkdelete earlier. During parallel vacuum, two
different processes could do btbulkdelete() and btvacuumcleanup()
respectively. Updating those values separately in those callbacks
would be straightforward.

Regards,

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

Re: progress reporting for partitioned REINDEX

[Matthias van de Meent]

On Tue, 16 Feb 2021, 07:42 Justin Pryzby,  wrote:
>
> It looks like we missed this in a6642b3ae.
>
> I think it's an odd behavior of pg_stat_progress_create_index to 
> simultaneously
> show the global progress as well as the progress for the current partition ...
>
> It seems like for partitioned reindex, reindex_index() should set the AM, 
> which
> is used in the view:
>
> src/backend/catalog/system_views.sql-   WHEN 2 THEN 
> 'building index' ||
> src/backend/catalog/system_views.sql:   COALESCE((': 
> ' || pg_indexam_progress_phasename(S.param9::oid, S.param11)),
>
> Maybe it needs a new flag, like:
> params->options & REINDEXOPT_REPORT_PROGRESS_AM
>
> I don't understand why e66bcfb4c added multiple calls to
> pgstat_progress_start_command().


These were added to report the index and table that are currently
being worked on in concurrent reindexes of tables, schemas and
databases. Before that commit, it would only report up to the last
index being prepared in phase 1, leaving the user with no info on
which index is being rebuilt.

Why pgstat_progress_start_command specifically was chosen? That is
because there is no method to update the
beentry->st_progress_command_target other than through
stat_progress_start_command, and according to the docs that field
should contain the tableId of the index that is currently being worked
on. This field needs a pgstat_progress_start_command because CIC / RiC
reindexes all indexes concurrently at the same time (and not grouped
by e.g. table), so we must re-start reporting for each index in each
new phase in which we report data to get the heapId reported correctly
for that index.


With regards,

Matthias van de Meent

Re: [PATCH] pg_hba.conf error messages for logical replication connections

[Amit Kapila]

On Tue, Feb 2, 2021 at 1:43 AM Paul Martinez  wrote:
>
> On Fri, Jan 29, 2021 at 8:41 PM Amit Kapila  wrote:
> >
> > Yeah, hints or more details might improve the situation but I am not
> > sure we want to add more branching here. Can we write something
> > similar to HOSTNAME_LOOKUP_DETAIL for hints? Also, I think what you
> > are proposing to write is more of a errdetail kind of message. See
> > more error routines in the docs [1].
> >
>
> Alright, I've updated both sets of error messages to use something like
> HOSTNAME_LOOKUP_DETAIL, both for the error message itself, and for the
> extra detail message about the replication keyword. Since now we specify
> both an errdetail (sent to the client) and an errdetail_log (sent to the
> log), I renamed HOSTNAME_LOOKUP_DETAIL to HOSTNAME_LOOKUP_DETAIL_LOG.
>

I don't think we need to update the error messages, it makes the code
a bit difficult to parse without much benefit. How about just adding
errdetail? See attached and let me know what you think?

-- 
With Regards,
Amit Kapila.


pg_hba_conf_error_message_patch_v02.patch
Description: Binary data

RE: pg_replication_origin_session_setup and superuser

[Zohar Gofer]

Thanks. This seems to be the fix we need.
Would it be possible to push it to previous versions? 12 or 13?

Zohar

-Original Message-
From: Michael Paquier  
Sent: Tuesday, February 16, 2021 2:52 AM
To: Zohar Gofer 
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: pg_replication_origin_session_setup and superuser

On Mon, Feb 15, 2021 at 09:37:53AM +, Zohar Gofer wrote:
> In my mind the requirement for superuser is too strong. I think that 
> requiring privileges of a replication user is more suitable. This way 
> we can require that only a user with replication privileges will 
> actually do replication, even if this is not really a replication.

PostgreSQL 14 will remove those hardcoded superuser checks.  Please see this 
thread:
https://www.postgresql.org/message-id/capdie1xjmzokql3dghmurpqyszkgwzsmxetfkkhynbab7-0...@mail.gmail.com
And its related commit:
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=cc072641d41c55c6aa24a331fc1f8029e0a8d799

While the default is still superuser-only, it becomes possible to grant access 
to this stuff to other roles that have no need to be superusers.
--
Michael
This email and the information contained herein is proprietary and confidential 
and subject to the Amdocs Email Terms of Service, which you may review at 
https://www.amdocs.com/about/email-terms-of-service 

Re: POC: postgres_fdw insert batching

[Amit Langote]

On Tue, Feb 16, 2021 at 1:36 AM Tomas Vondra
 wrote:
> On 2/5/21 3:52 AM, Amit Langote wrote:
> > Tsunakwa-san,
> >
> > On Mon, Jan 25, 2021 at 1:21 PM tsunakawa.ta...@fujitsu.com
> >  wrote:
> >> From: Amit Langote 
> >>> Yes, it can be simplified by using a local join to prevent the update of 
> >>> the foreign
> >>> partition from being pushed to the remote server, for which my example in 
> >>> the
> >>> previous email used a local trigger.  Note that the update of the foreign
> >>> partition to be done locally is a prerequisite for this bug to occur.
> >>
> >> Thank you, I was aware that UPDATE calls ExecInsert() but forgot about it 
> >> partway.  Good catch (and my bad miss.)
> >
> > It appears I had missed your reply, sorry.
> >
> >> +   PgFdwModifyState *fmstate = resultRelInfo->ri_FdwState ?
> >> +   (PgFdwModifyState 
> >> *) resultRelInfo->ri_FdwState :
> >> +   NULL;
> >>
> >> This can be written as:
> >>
> >> +   PgFdwModifyState *fmstate = (PgFdwModifyState *) 
> >> resultRelInfo->ri_FdwState;
> >
> > Facepalm, yes.
> >
> > Patch updated.  Thanks for the review.
> >
>
> Thanks for the patch, it seems fine to me.

Thanks for checking.

> I wonder it the commit
> message needs some tweaks, though. At the moment it says:
>
> Prevent FDW insert batching during cross-partition updates
>
> but what the patch seems to be doing is simply initializing the info
> only for CMD_INSERT operations. Which does the trick, but it affects
> everything, i.e. all updates, no? Not just cross-partition updates.

You're right.  Please check the message in the updated patch.


--
Amit Langote
EDB: http://www.enterprisedb.com
From b1d470fc764279ba12787271a04015a123d20b4f Mon Sep 17 00:00:00 2001
From: amitlan 
Date: Sat, 23 Jan 2021 16:35:21 +0900
Subject: [PATCH v4] Fix tuple routing to initialize batching only for inserts

Currently, the insert component of a cross-partition update of a
partitioned table (internally implemented as a delete followed by an
insert) inadvertently ends up using batching for the insert. That may
pose a problem if the insert target is a foreign partition, because
the partition's FDW may not be able to handle both the original update
operation and the batched insert operation being performed at the same
time.  So tighten up the check in ExecInitRoutingInfo() to initialize
batching only if the query's original operation is also INSERT.
---
 .../postgres_fdw/expected/postgres_fdw.out| 23 ++-
 contrib/postgres_fdw/postgres_fdw.c   | 13 +--
 contrib/postgres_fdw/sql/postgres_fdw.sql | 19 ++-
 src/backend/executor/execPartition.c  |  3 ++-
 4 files changed, 53 insertions(+), 5 deletions(-)

diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 60c7e115d6..3326f1b542 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -9414,5 +9414,26 @@ SELECT COUNT(*) FROM batch_table;
 66
 (1 row)
 
+-- Check that enabling batched inserts doesn't interfere with cross-partition
+-- updates
+CREATE TABLE batch_cp_upd_test (a int) PARTITION BY LIST (a);
+CREATE TABLE batch_cp_upd_test1 (LIKE batch_cp_upd_test);
+CREATE FOREIGN TABLE batch_cp_upd_test1_f
+	PARTITION OF batch_cp_upd_test
+	FOR VALUES IN (1)
+	SERVER loopback
+	OPTIONS (table_name 'batch_cp_upd_test1', batch_size '10');
+CREATE TABLE batch_cp_up_test1 PARTITION OF batch_cp_upd_test
+	FOR VALUES IN (2);
+INSERT INTO batch_cp_upd_test VALUES (1), (2);
+-- The following moves a row from the local partition to the foreign one
+UPDATE batch_cp_upd_test t SET a = 1 FROM (VALUES (1), (2)) s(a) WHERE t.a = s.a;
+SELECT tableoid::regclass, * FROM batch_cp_upd_test;
+   tableoid   | a 
+--+---
+ batch_cp_upd_test1_f | 1
+ batch_cp_upd_test1_f | 1
+(2 rows)
+
 -- Clean up
-DROP TABLE batch_table CASCADE;
+DROP TABLE batch_table, batch_cp_upd_test CASCADE;
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 368997d9d1..35b48575c5 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -1934,17 +1934,26 @@ static int
 postgresGetForeignModifyBatchSize(ResultRelInfo *resultRelInfo)
 {
 	int	batch_size;
+	PgFdwModifyState *fmstate = resultRelInfo->ri_FdwState ?
+			(PgFdwModifyState *) resultRelInfo->ri_FdwState :
+			NULL;
 
 	/* should be called only once */
 	Assert(resultRelInfo->ri_BatchSize == 0);
 
+	/*
+	 * Should never get called when the insert is being performed as part of
+	 * a row movement operation.
+	 */
+	Assert(fmstate == NULL || fmstate->aux_fmstate == NULL);
+
 	/*
 	 * In EXPLAIN without ANALYZE, ri_fdwstate is NULL, so we have to lookup
 	 * the option directly in server/table options. Otherwise just use the
 	 * value we 

ERROR: "ft1" is of the wrong type.

[Kyotaro Horiguchi]

Hello,

If I invoked a wrong ALTER TABLE command like this, I would see an
unexpected error.

=# ALTER TABLE  ATTACH PARTITION 
ERROR:  "ft1" is of the wrong type

The cause is that ATWrongRelkidError doesn't handle ATT_TABLE |
ATT_ATT_PARTITIONED_INDEX.

After checking all callers of ATSimplePermissions, I found that;

The two below are no longer used.

  ATT_TABLE | ATT_VIEW
  ATT_TABLE | ATT_MATVIEW | ATT_INDEX

The four below are not handled.

  ATT_TABLE | ATT_MATVIEW | ATT_INDEX | ATT_PARTITIONED_INDEX:
  ATT_TABLE | ATT_MATVIEW | ATT_INDEX | ATT_PARTITIONED_INDEX | 
ATT_FOREIGN_TABLE
  ATT_TABLE | ATT_PARTITIONED_INDEX:
  ATT_INDEX:

The attached is just fixing that.  I tried to make it generic but
didn't find a clean and translatable way.

Also I found that only three cases in the function are excecised by
make check.

ATT_TABLE : foreign_data, indexing checks 
ATT_TABLE | ATT_FOREIGN_TABLE : alter_table
ATT_TABLE | ATT_COMPOSITE_TYPE | ATT_FOREIGN_TABLE : alter_table

I'm not sure it's worth the trouble so the attached doesn't do
anything for that.


Versions back to PG11 have similar but different mistakes.

PG11, 12:
 the two below are not used
   ATT_TABLE | ATT_VIEW
   ATT_TABLE | ATT_MATVIEW | ATT_INDEX

 the two below are not handled
   ATT_TABLE | ATT_MATVIEW | ATT_INDEX | ATT_PARTITIONED_INDEX
   ATT_TABLE | ATT_PARTITIONED_INDEX

PG13:
 the two below are not used
   ATT_TABLE | ATT_VIEW
   ATT_TABLE | ATT_MATVIEW | ATT_INDEX

 the three below are not handled
   ATT_TABLE | ATT_MATVIEW | ATT_INDEX | ATT_PARTITIONED_INDEX
   ATT_TABLE | ATT_MATVIEW | ATT_INDEX | ATT_PARTITIONED_INDEX | 
ATT_FOREIGN_TABLE
   ATT_TABLE | ATT_PARTITIONED_INDEX

PG10:
  ATT_TABLE | ATT_VIEW is not used
  (all values are handled)

So the attached are the patches for PG11, 12, 13 and master.

It seems that the case lines in the function are intended to be in the
ATT_*'s definition order, but some of the them are out of that
order. However, I didn't reorder existing lines in the attached. I
didn't check the value itself is correct for the callers.

regareds.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index a149ca044c..cf572a7c58 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -5063,9 +5063,6 @@ ATWrongRelkindError(Relation rel, int allowed_targets)
 		case ATT_TABLE:
 			msg = _("\"%s\" is not a table");
 			break;
-		case ATT_TABLE | ATT_VIEW:
-			msg = _("\"%s\" is not a table or view");
-			break;
 		case ATT_TABLE | ATT_VIEW | ATT_FOREIGN_TABLE:
 			msg = _("\"%s\" is not a table, view, or foreign table");
 			break;
@@ -5075,8 +5072,8 @@ ATWrongRelkindError(Relation rel, int allowed_targets)
 		case ATT_TABLE | ATT_MATVIEW:
 			msg = _("\"%s\" is not a table or materialized view");
 			break;
-		case ATT_TABLE | ATT_MATVIEW | ATT_INDEX:
-			msg = _("\"%s\" is not a table, materialized view, or index");
+		case ATT_TABLE | ATT_MATVIEW | ATT_INDEX | ATT_PARTITIONED_INDEX:
+			msg = _("\"%s\" is not a table, materialized view, index, or partitioned index");
 			break;
 		case ATT_TABLE | ATT_MATVIEW | ATT_FOREIGN_TABLE:
 			msg = _("\"%s\" is not a table, materialized view, or foreign table");
@@ -5090,6 +5087,9 @@ ATWrongRelkindError(Relation rel, int allowed_targets)
 		case ATT_TABLE | ATT_MATVIEW | ATT_INDEX | ATT_FOREIGN_TABLE:
 			msg = _("\"%s\" is not a table, materialized view, index, or foreign table");
 			break;
+		case ATT_TABLE | ATT_PARTITIONED_INDEX:
+			msg = _("\"%s\" is not a table or partitioned index");
+			break;
 		case ATT_VIEW:
 			msg = _("\"%s\" is not a view");
 			break;
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 6f4a3e70a4..6c15a4e034 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -5350,9 +5350,6 @@ ATWrongRelkindError(Relation rel, int allowed_targets)
 		case ATT_TABLE:
 			msg = _("\"%s\" is not a table");
 			break;
-		case ATT_TABLE | ATT_VIEW:
-			msg = _("\"%s\" is not a table or view");
-			break;
 		case ATT_TABLE | ATT_VIEW | ATT_FOREIGN_TABLE:
 			msg = _("\"%s\" is not a table, view, or foreign table");
 			break;
@@ -5362,8 +5359,8 @@ ATWrongRelkindError(Relation rel, int allowed_targets)
 		case ATT_TABLE | ATT_MATVIEW:
 			msg = _("\"%s\" is not a table or materialized view");
 			break;
-		case ATT_TABLE | ATT_MATVIEW | ATT_INDEX:
-			msg = _("\"%s\" is not a table, materialized view, or index");
+		case ATT_TABLE | ATT_MATVIEW | ATT_INDEX | ATT_PARTITIONED_INDEX:
+			msg = _("\"%s\" is not a table, materialized view, index, or partitioned index");
 			break;
 		case ATT_TABLE | ATT_MATVIEW | ATT_FOREIGN_TABLE:
 			msg = _("\"%s\" is not a table, materialized view, or foreign table");
@@ -5377,6 +5374,9 @@ ATWrongRelkindError(Relation rel, int allowed_targets)
 		case ATT_TABLE | ATT_MATVIEW | ATT_INDEX | ATT_FOREIGN_TABLE:
 			

Re: [PoC] Non-volatile WAL buffer

[Takashi Menjo]

Hi Takayuki,

Thank you for your helpful comments.

In "Allocates WAL buffers on shared buffers", "shared buffers" should be
> DRAM because shared buffers in Postgres means the buffer cache for database
> data.
>

That's true. Fixed.


> I haven't tracked the whole thread, but could you collect information like
> the following?  I think such (partly basic) information will be helpful to
> decide whether it's worth casting more efforts into complex code, or it's
> enough to place WAL on DAX-aware filesystems and tune the filesystem.
>
> * What approaches other DBMSs take, and their performance gains (Oracle,
> SQL Server, HANA, Cassandra, etc.)
> The same DBMS should take different approaches depending on the file type:
> Oracle recommends different things to data files and REDO logs.
>

I also think it will be helpful. Adding "Other DBMSes using PMEM" section.

* The storage capabilities of PMEM compared to the fast(est) alternatives
> such as NVMe SSD (read/write IOPS, latency, throughput, concurrency, which
> may be posted on websites like Tom's Hardware or SNIA)
>

This will be helpful, too. Adding "Basic performance" subsection under
"Overview of persistent memory (PMEM)."

* What's the situnation like on Windows?
>

Sorry but I don't know Windows' PMEM support very much. All I know is that
Windows Server 2016 and 2019 supports PMEM (2016 partially) [1] and PMDK
supports Windows [2].

All the above contents will be updated gradually. Please stay tuned.

Regards,

[1]
https://docs.microsoft.com/en-us/windows-server/storage/storage-spaces/deploy-pmem
[2]
https://docs.pmem.io/persistent-memory/getting-started-guide/installing-pmdk/installing-pmdk-on-windows

-- 
Takashi Menjo 

Re: Parallel INSERT (INTO ... SELECT ...)

[Greg Nancarrow]

On Mon, Feb 8, 2021 at 8:13 PM Hou, Zhijie  wrote:
>
> > > Did it actually use a parallel plan in your testing?
> > > When I ran these tests with the Parallel INSERT patch applied, it did
> > > not naturally choose a parallel plan for any of these cases.
> >
> > Yes, these cases pick parallel plan naturally on my test environment.
> >
> > postgres=# explain verbose insert into testscan select a from x where
> > a<8 or (a%2=0 and a>19990);
> > QUERY PLAN
> > --
> > -
> >  Gather  (cost=4346.89..1281204.64 rows=81372 width=0)
> >Workers Planned: 4
> >->  Insert on public.testscan  (cost=3346.89..1272067.44 rows=0
> > width=0)
> >  ->  Parallel Bitmap Heap Scan on public.x1
> > (cost=3346.89..1272067.44 rows=20343 width=8)
> >Output: x1.a, NULL::integer
> >Recheck Cond: ((x1.a < 8) OR (x1.a > 19990))
> >Filter: ((x1.a < 8) OR (((x1.a % 2) = 0) AND (x1.a >
> > 19990)))
> >->  BitmapOr  (cost=3346.89..3346.89 rows=178808
> > width=0)
> >  ->  Bitmap Index Scan on x1_a_idx
> > (cost=0.00..1495.19 rows=80883 width=0)
> >Index Cond: (x1.a < 8)
> >  ->  Bitmap Index Scan on x1_a_idx
> > (cost=0.00..1811.01 rows=97925 width=0)
> >Index Cond: (x1.a > 19990)
> >
> > PSA is my postgresql.conf file, maybe you can have a look. Besides, I didn't
> > do any parameters tuning in my test session.
>
> I reproduced this on my machine.
>
> I think we'd better do "analyze" before insert which helps reproduce this 
> easier.
> Like:
>
> -
> analyze;
> explain analyze verbose insert into testscan select a from x where a<8 or 
> (a%2=0 and a>19990);
> -
>

Thanks, I tried test_bimap.sql in my own environment, and added
"analyze", and I also found it naturally chose a parallel INSERT with
parallel bitmap heap scan for each of these cases.
However, I didn't see any performance degradation when compared
against serial INSERT with bitmap heap scan.
The parallel plan in these cases seems to run a bit faster.
(Note that I'm using a release build of Postgres, and using default
postgresql.conf)


test=# set max_parallel_workers_per_gather=4;
SET
test=# explain analyze verbose insert into testscan select a from x
where a<8 or (a%2=0 and a>19990);

QUERY PLAN


 Gather  (cost=4193.29..1255440.94 rows=74267 width=0) (actual
time=210.587..212.135 rows=0 loops=1)
   Workers Planned: 4
   Workers Launched: 4
   ->  Insert on public.testscan  (cost=3193.29..1247014.24 rows=0
width=0) (actual time=195.296..195.298 rows=0 loops=5)
 Worker 0:  actual time=189.512..189.514 rows=0 loops=1
 Worker 1:  actual time=194.843..194.844 rows=0 loops=1
 Worker 2:  actual time=193.986..193.988 rows=0 loops=1
 Worker 3:  actual time=188.035..188.037 rows=0 loops=1
 ->  Parallel Bitmap Heap Scan on public.x
(cost=3193.29..1247014.24 rows=18567 width=8) (actual
time=7.992..25.837 row
s=26000 loops=5)
   Output: x.a, NULL::integer
   Recheck Cond: ((x.a < 8) OR (x.a > 19990))
   Filter: ((x.a < 8) OR (((x.a % 2) = 0) AND (x.a >
19990)))
   Rows Removed by Filter: 1
   Heap Blocks: exact=261
   Worker 0:  actual time=1.473..14.458 rows=22465 loops=1
   Worker 1:  actual time=7.370..31.359 rows=30525 loops=1
   Worker 2:  actual time=8.765..19.838 rows=18549 loops=1
   Worker 3:  actual time=0.279..17.269 rows=23864 loops=1
   ->  BitmapOr  (cost=3193.29..3193.29 rows=170535
width=0) (actual time=21.775..21.777 rows=0 loops=1)
 ->  Bitmap Index Scan on x_a_idx
(cost=0.00..1365.94 rows=73783 width=0) (actual time=11.961..11.961
rows=
7 loops=1)
   Index Cond: (x.a < 8)
 ->  Bitmap Index Scan on x_a_idx
(cost=0.00..1790.21 rows=96752 width=0) (actual time=9.809..9.809
rows=10
 loops=1)
   Index Cond: (x.a > 19990)
 Planning Time: 0.276 ms
 Execution Time: 212.189 ms
(25 rows)


test=# truncate testscan;
TRUNCATE TABLE
test=# set max_parallel_workers_per_gather=0;
SET
test=# explain analyze verbose insert into testscan select a from x
where a<8 or (a%2=0 and a>19990);
   QUERY
PLAN


 Insert on public.testscan  (cost=3193.29..3625636.35 rows=0 width=0)
(actual time=241.222..241.224 rows=0 loops=1)
   ->  

Re: [POC] Fast COPY FROM command for the table with foreign partitions

[Andrey V. Lepikhov]

On 2/15/21 1:31 PM, Amit Langote wrote:

Tsunakawa-san, Andrey,
+static void
+postgresBeginForeignCopy(ModifyTableState *mtstate,
+  ResultRelInfo *resultRelInfo)
+{
...
+   if (resultRelInfo->ri_RangeTableIndex == 0)
+   {
+   ResultRelInfo *rootResultRelInfo = resultRelInfo->ri_RootResultRelInfo;
+
+   rte = exec_rt_fetch(rootResultRelInfo->ri_RangeTableIndex, estate);

It's better to add an Assert(rootResultRelInfo != NULL) here.
Apparently, there are cases where ri_RangeTableIndex == 0 without
ri_RootResultRelInfo being set.  The Assert will ensure that
BeginForeignCopy() is not mistakenly called on such ResultRelInfos.


+1


I can't parse what the function's comment says about "using list of
parameters".  Maybe it means to say "list of columns" specified in the
COPY FROM statement.  How about writing this as:

/*
  * Deparse remote COPY FROM statement
  *
  * Note that this explicitly specifies the list of COPY's target columns
  * to account for the fact that the remote table's columns may not match
  * exactly with the columns declared in the local definition.
  */

I'm hoping that I'm interpreting the original note correctly.  Andrey?


Yes, this is a good option.



+
+ mtstate is the overall state of the
+ ModifyTable plan node being executed;
global data about
+ the plan and execution state is available via this structure.
...
+typedef void (*BeginForeignCopy_function) (ModifyTableState *mtstate,
+  ResultRelInfo *rinfo);

Maybe a bit late realizing this, but why does BeginForeignCopy()
accept a ModifyTableState pointer whereas maybe just an EState pointer
will do?  I can't imagine why an FDW would want to look at the
ModifyTableState.  Case in point, I see that
postgresBeginForeignCopy() only uses the EState from the
ModifyTableState passed to it.  I think the ResultRelInfo that's being
passed to the Copy APIs contains most of the necessary information.
Also, EndForeignCopy() seems fine with just receiving the EState.


+1


If the intention is to only prevent this error, maybe the condition
above could be changed as this:

 /*
  * Check whether we support copying data out of the specified relation,
  * unless the caller also passed a non-NULL data_dest_cb, in which case,
  * the callback will take care of it
  */
 if (rel != NULL && rel->rd_rel->relkind != RELKIND_RELATION &&
 data_dest_cb == NULL)


Agreed. This is an atavism. In the first versions, I did not use the 
data_dest_cb routine. But now this is a redundant parameter.


--
regards,
Andrey Lepikhov
Postgres Professional

RE: [PoC] Non-volatile WAL buffer

[tsunakawa.ta...@fujitsu.com]

From: Takashi Menjo  
> I made a new page at PostgreSQL Wiki to gather and summarize information and 
> discussion about PMEM-backed WAL designs and implementations. Some parts of 
> the page are TBD. I will continue to maintain the page. Requests are welcome.
> 
> Persistent Memory for WAL
> https://wiki.postgresql.org/wiki/Persistent_Memory_for_WAL

Thank you for putting together the information.

In "Allocates WAL buffers on shared buffers", "shared buffers" should be DRAM 
because shared buffers in Postgres means the buffer cache for database data.

I haven't tracked the whole thread, but could you collect information like the 
following?  I think such (partly basic) information will be helpful to decide 
whether it's worth casting more efforts into complex code, or it's enough to 
place WAL on DAX-aware filesystems and tune the filesystem.

* What approaches other DBMSs take, and their performance gains (Oracle, SQL 
Server, HANA, Cassandra, etc.)
The same DBMS should take different approaches depending on the file type: 
Oracle recommends different things to data files and REDO logs.

* The storage capabilities of PMEM compared to the fast(est) alternatives such 
as NVMe SSD (read/write IOPS, latency, throughput, concurrency, which may be 
posted on websites like Tom's Hardware or SNIA)

* What's the situnation like on Windows?


Regards
Takayuki Tsunakawa

Re: [HACKERS] Custom compression methods

[Dilip Kumar]

On Mon, Feb 15, 2021 at 1:58 AM Justin Pryzby  wrote:
>
> On Sun, Feb 14, 2021 at 12:49:40PM -0600, Justin Pryzby wrote:
> > On Wed, Feb 10, 2021 at 04:56:17PM -0500, Robert Haas wrote:
> > > Small delta patch with a few other suggested changes attached.
> >
> > Robert's fixup patch caused the CI to fail, since it 1) was called *.patch;
> > and, 2) didn't include the previous patches.
> >
> > This includes a couple proposals of mine as separate patches.
>
> CIs failed on BSD and linux due to a test in contrib/, but others passed.
> https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.127551
> https://cirrus-ci.com/task/6087701947482112
> https://cirrus-ci.com/task/6650651900903424
> https://cirrus-ci.com/task/5524751994060800
>
> Resending with fixes to configure.ac and missed autoconf run.  I think this is
> expected to fail on mac, due to missing LZ4.
>
> BTW, compressamapi.h doesn't need to be included in any of these, at least in
> the 0001 patch:
>
>  src/backend/access/common/indextuple.c | 2 +-
>  src/backend/catalog/heap.c | 2 +-
>  src/backend/catalog/index.c| 2 +-
>  src/backend/parser/parse_utilcmd.c | 2 +-
>
> It's pretty unfriendly that this requires quoting the integer to be
> syntactically valid:
>
> |postgres=# create table j(q text compression pglz with (level 1) );
> |2021-01-30 01:26:33.554 CST [31814] ERROR:  syntax error at or near "1" at 
> character 52
> |2021-01-30 01:26:33.554 CST [31814] STATEMENT:  create table j(q text 
> compression pglz with (level 1) );
> |ERROR:  syntax error at or near "1"
> |LINE 1: create table j(q text compression pglz with (level 1) );

Thanks for the review and patch for HIDE_COMPRESSAM,  I will merge
this into the main patch.  And work on other comments after fixing the
issue related to compressed data in composite types.

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

Re: pg_replication_origin_session_setup and superuser

[Michael Paquier]

On Tue, Feb 16, 2021 at 07:54:32AM +, Zohar Gofer wrote:
> Thanks. This seems to be the fix we need.
> Would it be possible to push it to previous versions? 12 or 13?

New features don't go into stable branches, only bug fixes do.  And
this is not a bug fix, but a feature.
--
Michael


signature.asc
Description: PGP signature