Re: Inconsistency in startup process's MyBackendId and procsignal array registration with ProcSignalInit()

[Bharath Rupireddy]

On Mon, Oct 11, 2021 at 8:59 PM Fujii Masao  wrote:
>
> On 2021/10/11 19:46, Bharath Rupireddy wrote:
> > If we do the above, then the problem might arise if somebody calls
> > SICleanupQueue and wants to signal the startup process, the below code
> > (from SICleanupQueue) can't get the startup process backend id. So,
> > the backend id calculation for the startup process can't just be
> > MaxBackends + MyAuxProcType + 1.
> > BackendId his_backendId = (needSig - &segP->procState[0]) + 1;
>
> Attached POC patch illustrates what I'm in mind. ISTM this change
> doesn't prevent SICleanupQueue() from getting right backend ID
> of the startup process. Thought?

The patch looks good to me unless I'm missing something badly.

+ Assert(MyBackendId == InvalidBackendId ||
+MyBackendId <= segP->maxBackends);
+
In the above assertion, we can just do MyBackendId ==
segP->maxBackends, instead of <= as the startup process is the only
process that calls SharedInvalBackendInit with pre-calculated
MyBackendId = MaxBackends + MyAuxProcType + 1; and it will always
occupy the last slot in the procState array.

Otherwise, we could discard defining MyBackendId in auxprocess.c and
define the MyBackendId in the SharedInvalBackendInit itself as this is
the function that defines the MyBackendId for everyone whoever
requires it. I prefer this approach over what's done in PoC patch.

In SharedInvalBackendInit:
Assert(MyBackendId == InvalidBackendId);
/*
* The startup process requires a valid BackendId for the SI message
* buffer and virtual transaction id, so define it here with the value with
* which the procsignal array slot was allocated in AuxiliaryProcessMain.
* All other auxiliary processes don't need it.
*/
if (MyAuxProcType == StartupProcess)
MyBackendId = MaxBackends + MyAuxProcType + 1;

I think this solution, coupled with the one proposed at [1], should
solve this startup process's inconsistency in MyBackendId, procState
and ProcSignal array slot problems.

[1] - 
https://www.postgresql.org/message-id/CALj2ACXZ_o7rcOb7-Rs96P0d%3DEi%2Bnvf_WZ-Meky7Vv%2BnQNFYjQ%40mail.gmail.com

Regards,
Bharath Rupireddy.

Re: Inconsistency in startup process's MyBackendId and procsignal array registration with ProcSignalInit()

[Kyotaro Horiguchi]

At Mon, 11 Oct 2021 16:03:57 +0530, Bharath Rupireddy 
 wrote in 
> On Mon, Oct 11, 2021 at 12:41 PM Kyotaro Horiguchi
>  wrote:
> > (I'm not sure how the trouble happens.)
...
>  If some process wants to send the startup process SIGUSR1 with the
> PGPROC->backendId and SendProcSignal(PGPROC->pid, X,
> PGPROC->backendId) after getting the PGPROC entry from
> AuxiliaryPidGetProc(), then the signal isn't sent. To understand this
> issue, please use a sample patch at [1], have a standby setup, call
> pg_log_backend_memory_contexts with startup process pid on the
> standby, the error "could not send signal to process" is shown, see
> [2].

Thanks, I understand that this doesn't happen on vanilla PG.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Make query ID more portable

[Andrey V. Lepikhov]

Hi,

QueryID is good tool for query analysis. I want to improve core jumbling 
machinery in two ways:
1. QueryID value should survive dump/restore of a database (use fully 
qualified name of table instead of relid).
2. QueryID could represent more general class of queries: for example, 
it can be independent from permutation of tables in a FROM clause.


See the patch in attachment as an POC. Main idea here is to break 
JumbleState down to a 'clocations' part that can be really interested in
a post parse hook and a 'context data', that needed to build query or 
subquery signature (hash) and, I guess, isn't really needed in any 
extensions.


I think, it adds not much complexity and overhead. It still not 
guaranteed equality of queryid on two instances with an equal schema, 
but survives across an instance upgrade and allows to do some query 
analysis on a replica node.


--
regards,
Andrey Lepikhov
Postgres Professional
>From 714111f82569ba827d6387ca3e01e5f364a2c8dd Mon Sep 17 00:00:00 2001
From: Andrey Lepikhov 
Date: Tue, 12 Oct 2021 11:53:50 +0500
Subject: [PATCH] Make queryid more portable. 1. Extract local context from a
 JumbleState. 2. Make a hash value for each range table entry. 3. Make a hash
 signature for each subquery. 4. Use hash instead of rti. 5. Sort hashes of
 range table entries before adding to a context.

TODO:
- Use attnames instead of varattno.
- Use sort of argument hashes at each level of expression jumbling.
---
 contrib/pg_stat_statements/Makefile   |   1 +
 .../t/001_queryid_portability.pl  |  61 
 src/backend/utils/adt/regproc.c   |  25 +-
 src/backend/utils/misc/queryjumble.c  | 319 +++---
 src/include/utils/queryjumble.h   |  20 +-
 src/include/utils/regproc.h   |   1 +
 6 files changed, 299 insertions(+), 128 deletions(-)
 create mode 100644 contrib/pg_stat_statements/t/001_queryid_portability.pl

diff --git a/contrib/pg_stat_statements/Makefile b/contrib/pg_stat_statements/Makefile
index 7fabd96f38..bef304e7d4 100644
--- a/contrib/pg_stat_statements/Makefile
+++ b/contrib/pg_stat_statements/Makefile
@@ -17,6 +17,7 @@ LDFLAGS_SL += $(filter -lm, $(LIBS))
 
 REGRESS_OPTS = --temp-config $(top_srcdir)/contrib/pg_stat_statements/pg_stat_statements.conf
 REGRESS = pg_stat_statements oldextversions
+TAP_TESTS = 1
 # Disabled because these tests require "shared_preload_libraries=pg_stat_statements",
 # which typical installcheck users do not have (e.g. buildfarm clients).
 NO_INSTALLCHECK = 1
diff --git a/contrib/pg_stat_statements/t/001_queryid_portability.pl b/contrib/pg_stat_statements/t/001_queryid_portability.pl
new file mode 100644
index 00..80f8bb4e93
--- /dev/null
+++ b/contrib/pg_stat_statements/t/001_queryid_portability.pl
@@ -0,0 +1,61 @@
+use strict;
+use warnings;
+use PostgresNode;
+use TestLib;
+
+use Test::More tests => 3;
+
+my ($node1, $node2, $result1, $result2);
+
+$node1 = PostgresNode->new('node1');
+$node1->init;
+$node1->append_conf('postgresql.conf', qq{
+	shared_preload_libraries = 'pg_stat_statements'
+	pg_stat_statements.track = 'all'
+});
+$node1->start;
+
+$node2 = PostgresNode->new('node2');
+$node2->init;
+$node2->append_conf('postgresql.conf', qq{
+	shared_preload_libraries = 'pg_stat_statements'
+	pg_stat_statements.track = 'all'
+});
+$node2->start;
+$node2->safe_psql('postgres', qq{CREATE TABLE a(); DROP TABLE a;});
+
+$node1->safe_psql('postgres', q(CREATE EXTENSION pg_stat_statements));
+$node2->safe_psql('postgres', q(CREATE EXTENSION pg_stat_statements));
+
+$node1->safe_psql('postgres', "
+	SELECT pg_stat_statements_reset();
+	CREATE TABLE a (x int, y varchar);
+	CREATE TABLE b (x int);
+	SELECT * FROM a;"
+);
+$node2->safe_psql('postgres', "
+	SELECT pg_stat_statements_reset();
+	CREATE TABLE a (y varchar, x int);
+	CREATE TABLE b (x int);
+	SELECT * FROM a;
+");
+
+$result1 = $node1->safe_psql('postgres', "SELECT queryid FROM pg_stat_statements WHERE query LIKE 'SELECT * FROM a';");
+$result2 = $node2->safe_psql('postgres', "SELECT queryid FROM pg_stat_statements WHERE query LIKE 'SELECT * FROM a';");
+is($result1, $result2);
+
+$node1->safe_psql('postgres', "SELECT x FROM a");
+$node2->safe_psql('postgres', "SELECT x FROM a");
+$result1 = $node1->safe_psql('postgres', "SELECT queryid FROM pg_stat_statements WHERE query LIKE 'SELECT x FROM a';");
+$result2 = $node2->safe_psql('postgres', "SELECT queryid FROM pg_stat_statements WHERE query LIKE 'SELECT x FROM a';");
+is(($result1 != $result2), 1); # TODO
+
+$node1->safe_psql('postgres', "SELECT * FROM a,b WHERE a.x = b.x;");
+$node2->safe_psql('postgres', "SELECT * FROM b,a WHERE a.x = b.x;");
+$result1 = $node1->safe_psql('postgres', "SELECT queryid FROM pg_stat_statements WHERE query LIKE 'SELECT * FROM a,b WHERE a.x = b.x;'");
+$result2 = $node2->safe_psql('postgres', "SELECT queryid FROM pg_stat_statements WHERE query LIKE 'SELECT * FROM b,a WHERE a.x = b.x;'");
+diag("$result1, \n $result2");

Re: Inconsistency in startup process's MyBackendId and procsignal array registration with ProcSignalInit()

[Kyotaro Horiguchi]

At Tue, 12 Oct 2021 13:09:47 +0530, Bharath Rupireddy 
 wrote in 
> On Mon, Oct 11, 2021 at 8:59 PM Fujii Masao  
> wrote:
> >
> > On 2021/10/11 19:46, Bharath Rupireddy wrote:
> > > If we do the above, then the problem might arise if somebody calls
> > > SICleanupQueue and wants to signal the startup process, the below code
> > > (from SICleanupQueue) can't get the startup process backend id. So,
> > > the backend id calculation for the startup process can't just be
> > > MaxBackends + MyAuxProcType + 1.
> > > BackendId his_backendId = (needSig - &segP->procState[0]) + 1;
> >
> > Attached POC patch illustrates what I'm in mind. ISTM this change
> > doesn't prevent SICleanupQueue() from getting right backend ID
> > of the startup process. Thought?
> 
> The patch looks good to me unless I'm missing something badly.
> 
> + Assert(MyBackendId == InvalidBackendId ||
> +MyBackendId <= segP->maxBackends);
> +
> In the above assertion, we can just do MyBackendId ==
> segP->maxBackends, instead of <= as the startup process is the only
> process that calls SharedInvalBackendInit with pre-calculated
> MyBackendId = MaxBackends + MyAuxProcType + 1; and it will always
> occupy the last slot in the procState array.

+1 for not allowing to explicitly specify the "auto-assigned"
backendid range,

> Otherwise, we could discard defining MyBackendId in auxprocess.c and
> define the MyBackendId in the SharedInvalBackendInit itself as this is
> the function that defines the MyBackendId for everyone whoever
> requires it. I prefer this approach over what's done in PoC patch.
> 
> In SharedInvalBackendInit:
> Assert(MyBackendId == InvalidBackendId);
> /*
> * The startup process requires a valid BackendId for the SI message
> * buffer and virtual transaction id, so define it here with the value with
> * which the procsignal array slot was allocated in AuxiliaryProcessMain.
> * All other auxiliary processes don't need it.
> */
> if (MyAuxProcType == StartupProcess)
> MyBackendId = MaxBackends + MyAuxProcType + 1;
> 
> I think this solution, coupled with the one proposed at [1], should
> solve this startup process's inconsistency in MyBackendId, procState
> and ProcSignal array slot problems.
> 
> [1] - 
> https://www.postgresql.org/message-id/CALj2ACXZ_o7rcOb7-Rs96P0d%3DEi%2Bnvf_WZ-Meky7Vv%2BnQNFYjQ%40mail.gmail.com

The patch does this:

case StartupProcess:
+   MyBackendId = MaxBackends + MyAuxProcType + 1;

as well as this:

+   shmInvalBuffer->maxBackends = MaxBackends + 1;

These don't seem to be in the strict correspondence.  I'd prefer
something like the following.

+   /* currently only StartupProcess uses nailed SI slot */
+   shmInvalBuffer->maxBackends = MaxBackends + StartupProcess + 1;

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: Make query ID more portable

[Julien Rouhaud]

Hi,

On Tue, Oct 12, 2021 at 4:12 PM Andrey V. Lepikhov
 wrote:
>
> QueryID is good tool for query analysis. I want to improve core jumbling
> machinery in two ways:
> 1. QueryID value should survive dump/restore of a database (use fully
> qualified name of table instead of relid).
> 2. QueryID could represent more general class of queries: for example,
> it can be independent from permutation of tables in a FROM clause.
>
> See the patch in attachment as an POC. Main idea here is to break
> JumbleState down to a 'clocations' part that can be really interested in
> a post parse hook and a 'context data', that needed to build query or
> subquery signature (hash) and, I guess, isn't really needed in any
> extensions.

There have been quite a lot of threads about that in the past, and
almost every time people wanted to change how the hash was computed.
So it seems to me that extensions would actually be quite interested
in that.  This is even more the case now that an extension can be used
to replace the queryid calculation only and keep the rest of the
extension relying on it as is.

> I think, it adds not much complexity and overhead.

I think the biggest change in your patch is:

  case RTE_RELATION:
- APP_JUMB(rte->relid);
- JumbleExpr(jstate, (Node *) rte->tablesample);
+ {
+ char *relname = regclassout_ext(rte->relid, true);
+
+ APP_JUMB_STRING(relname);
+ JumbleExpr(jstate, (Node *) rte->tablesample, ctx);
  APP_JUMB(rte->inh);
  break;

Have you done any benchmark on OLTP workload?  Adding catalog access
there is likely to add significant overhead.

Also, why only using the fully qualified relation name for stable
hashes?  At least operators and functions should also be treated the
same way.  If you do that you will probably have way too much overhead
to be usable in a busy production environment.  Why not using the new
possibility of 3rd party extension for the queryid calculation that
exactly suits your need?

Re: [RFC] building postgres with meson

[Andres Freund]

Hi,

On 2021-10-12 01:37:21 -0700, Andres Freund wrote:
> non-cached build (world-bin):
> current:40.46s
> ninja:   7.31s

Interestingly this is pretty close to the minimum achievable on my
machine from the buildsystem perspective.

A build with -fuse-ld=lld, which the above didn't use, takes 6.979s. The
critical path is

bison gram.y -> gram.c   4.13s
gcc gram.c -> gram.o 2.05s
gcc postgres 0.317


A very helpful visualization is to transform ninja's build logs into a
tracefile with https://github.com/nico/ninjatracing

I attached an example - the trace.json.gz can be uploaded as-is to
https://ui.perfetto.dev/

It's quite a bit of of fun to look at imo.

There's a few other things quickly apparent:

- genbki prevents build progress due to dependencies on the generated
  headers.
- the absolutely stupid way I implemented the python2->python3
  regression test output conversion uses up a fair bit of resources
- tablecmds.c, pg_dump.c, xlog.c and a few other files are starting to
  big enough to be problematic compile-time wise

Greetings,

Andres Freund


trace.json.gz
Description: application/gzip

Re: Inconsistency in startup process's MyBackendId and procsignal array registration with ProcSignalInit()

[Bharath Rupireddy]

On Tue, Oct 12, 2021 at 2:03 PM Kyotaro Horiguchi
 wrote:
> > [1] - 
> > https://www.postgresql.org/message-id/CALj2ACXZ_o7rcOb7-Rs96P0d%3DEi%2Bnvf_WZ-Meky7Vv%2BnQNFYjQ%40mail.gmail.com
>
> The patch does this:
>
> case StartupProcess:
> +   MyBackendId = MaxBackends + MyAuxProcType + 1;
>
> as well as this:
>
> +   shmInvalBuffer->maxBackends = MaxBackends + 1;
>
> These don't seem to be in the strict correspondence.  I'd prefer
> something like the following.
>
> +   /* currently only StartupProcess uses nailed SI slot */
> +   shmInvalBuffer->maxBackends = MaxBackends + StartupProcess + 1;

I don't think it is a good idea to use macro StartupProcess (because
the macro might get changed to a different value later). What we
essentially need to do for procState array is to extend its size by 1
(for startup process) which is being handled separately in [1]. Once
the patch at [1] gets in, the patch proposed here will not have the
above change at all.

[1] -  
https://www.postgresql.org/message-id/CALj2ACXZ_o7rcOb7-Rs96P0d%3DEi%2Bnvf_WZ-Meky7Vv%2BnQNFYjQ%40mail.gmail.com

Regards,
Bharath Rupireddy.

Re: Make query ID more portable

[Andrey Lepikhov]

On 12/10/21 13:35, Julien Rouhaud wrote:

Hi,

On Tue, Oct 12, 2021 at 4:12 PM Andrey V. Lepikhov
 wrote:

See the patch in attachment as an POC. Main idea here is to break
JumbleState down to a 'clocations' part that can be really interested in
a post parse hook and a 'context data', that needed to build query or
subquery signature (hash) and, I guess, isn't really needed in any
extensions.


There have been quite a lot of threads about that in the past, and
almost every time people wanted to change how the hash was computed.
So it seems to me that extensions would actually be quite interested
in that.  This is even more the case now that an extension can be used
to replace the queryid calculation only and keep the rest of the
extension relying on it as is.
Yes, I know. I have been using such self-made queryID for four years. 
And I will use it further.
But core jumbling code is good, fast and much easier in support. The 
purpose of this work is extending of jumbling to use in more flexible 
way to avoid meaningless copying of this code to an extension.

I think, it adds not much complexity and overhead.


I think the biggest change in your patch is:

   case RTE_RELATION:
- APP_JUMB(rte->relid);
- JumbleExpr(jstate, (Node *) rte->tablesample);
+ {
+ char *relname = regclassout_ext(rte->relid, true);
+
+ APP_JUMB_STRING(relname);
+ JumbleExpr(jstate, (Node *) rte->tablesample, ctx);
   APP_JUMB(rte->inh);
   break;

Have you done any benchmark on OLTP workload?  Adding catalog access
there is likely to add significant overhead.
Yes, I should do benchmarking. But I guess, main goal of Query ID is 
monitoring, that can be switched off, if necessary.

This part made for a demo. It can be replaced by a hook, for example.


Also, why only using the fully qualified relation name for stable
hashes?  At least operators and functions should also be treated the
same way.  If you do that you will probably have way too much overhead
to be usable in a busy production environment.  Why not using the new
possibility of 3rd party extension for the queryid calculation that
exactly suits your need?

I fully agree with these arguments. This code is POC. Main part here is 
breaking down JumbleState, using a local context for subqueries and 
sorting of a range table entries hashes.
I think, we can call one routine (APP_JUMB_OBJECT(), as an example) for 
all oids in this code. It would allow an extension to intercept this 
call and replace oid with an arbitrary value.


--
regards,
Andrey Lepikhov
Postgres Professional

preserve timestamps when installing headers

[Alexander Kuzmenkov]

Hi hackers,

I noticed that `make install` updates modification time for all
installed headers. This leads to recompilation of all dependent
objects, which is inconvenient for example when working on a
third-party extension. A way to solve this would be to pass
`INSTALL="install -p"` to `configure`, to make `install` preserve the
timestamp. After this, a new problem arises -- the
`src/include/Makefile` doesn't use `install` for all headers, but
instead uses `cp`. This patch adds `-p` switch to `cp` invocation in
these files, to make it preserve timestamps. Combined with the
aforementioned install flag, it allows a developer to hack on both
postgres and a third-party extension at the same time, without the
unneeded recompilation.


--
Alexander Kuzmenkov
Timescale
diff --git a/src/include/Makefile b/src/include/Makefile
index 5f257a958c..27242ff910 100644
--- a/src/include/Makefile
+++ b/src/include/Makefile
@@ -48,14 +48,16 @@ install: all installdirs
 	$(INSTALL_DATA) utils/fmgroids.h '$(DESTDIR)$(includedir_server)/utils'
 	$(INSTALL_DATA) utils/fmgrprotos.h '$(DESTDIR)$(includedir_server)/utils'
 # We don't use INSTALL_DATA for performance reasons --- there are a lot of files
-# (in fact, we have to take some pains to avoid overlength shell commands here)
-	cp $(srcdir)/*.h '$(DESTDIR)$(includedir_server)'/
+# (in fact, we have to take some pains to avoid overlength shell commands here).
+# We preserve timestamps while copying to avoid recompiling dependent objects
+# such as third-party extensions.
+	cp -p $(srcdir)/*.h '$(DESTDIR)$(includedir_server)'/
 	for dir in $(SUBDIRS); do \
-	  cp $(srcdir)/$$dir/*.h '$(DESTDIR)$(includedir_server)'/$$dir/ || exit; \
+	  cp -p $(srcdir)/$$dir/*.h '$(DESTDIR)$(includedir_server)'/$$dir/ || exit; \
 	done
 ifeq ($(vpath_build),yes)
 	for file in catalog/schemapg.h catalog/system_fk_info.h catalog/pg_*_d.h parser/gram.h storage/lwlocknames.h utils/probes.h; do \
-	  cp $$file '$(DESTDIR)$(includedir_server)'/$$file || exit; \
+	  cp -p $$file '$(DESTDIR)$(includedir_server)'/$$file || exit; \
 	done
 endif
 	cd '$(DESTDIR)$(includedir_server)' && chmod $(INSTALL_DATA_MODE) *.h

Re: Improve the HINT message of the ALTER command for postgres_fdw

[bt21masumurak]

Hi
Thank you for your comments.

It seems like the change proposed for postgres_fdw_validator is 
similar to what
file_fdw is doing in file_fdw_validator.  I think we also need to do 
the same

change in dblink_fdw_validator and postgresql_fdw_validator as well.


Agreed.


While on this, it's better to add test cases for the error message
"There are no valid options in this context." for all the three fdws
i.e. file_fdw, postgres_fdw and dblink_fdw may be in their respective
contrib modules test files, and for postgresql_fdw_validator in
src/test/regress/sql/foreign_data.sql.


+1.


I made new patch based on those comments.

Regards,
Kosei Masumura


2021-10-08 17:13 に Daniel Gustafsson さんは書きました:
On 8 Oct 2021, at 09:38, Bharath Rupireddy 
 wrote:


On Fri, Oct 8, 2021 at 12:48 PM bt21masumurak
 wrote:


Hi

When 'ALTER FOREIGN DATA WRAPPER OPTIONS' is executed against
postgres_fdw, the HINT message is printed as shown below, even though
there are no valid options in this context.

=# ALTER FOREIGN DATA WRAPPER postgres_fdw OPTIONS (format 'csv');
ERROR: invalid option "format"
HINT: Valid options in this context are:

I made a patch for this problem.


Good catch.


+1

It seems like the change proposed for postgres_fdw_validator is 
similar to what
file_fdw is doing in file_fdw_validator.  I think we also need to do 
the same

change in dblink_fdw_validator and postgresql_fdw_validator as well.


Agreed.


While on this, it's better to add test cases for the error message
"There are no valid options in this context." for all the three fdws
i.e. file_fdw, postgres_fdw and dblink_fdw may be in their respective
contrib modules test files, and for postgresql_fdw_validator in
src/test/regress/sql/foreign_data.sql.


+1.

--
Daniel Gustafsson   https://vmware.com/
diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
index 3a0beaa88e..c8b0b4ff3f 100644
--- a/contrib/dblink/dblink.c
+++ b/contrib/dblink/dblink.c
@@ -2054,8 +2054,10 @@ dblink_fdw_validator(PG_FUNCTION_ARGS)
 			ereport(ERROR,
 	(errcode(ERRCODE_FDW_OPTION_NAME_NOT_FOUND),
 	 errmsg("invalid option \"%s\"", def->defname),
-	 errhint("Valid options in this context are: %s",
-			 buf.data)));
+	 buf.len > 0
+	 ? errhint("Valid options in this context are: %s",
+			   buf.data)
+	 : errhint("There are no valid options in this context.")));
 		}
 	}
 
diff --git a/contrib/dblink/expected/dblink.out b/contrib/dblink/expected/dblink.out
index 6516d4f131..48a4d5ea99 100644
--- a/contrib/dblink/expected/dblink.out
+++ b/contrib/dblink/expected/dblink.out
@@ -1,4 +1,8 @@
 CREATE EXTENSION dblink;
+-- HINT test
+ALTER FOREIGN DATA WRAPPER dblink_fdw OPTIONS (format 'csv');
+ERROR:  invalid option "format"
+HINT:  There are no valid options in this context.
 -- want context for notices
 \set SHOW_CONTEXT always
 CREATE TABLE foo(f1 int, f2 text, f3 text[], primary key (f1,f2));
diff --git a/contrib/dblink/sql/dblink.sql b/contrib/dblink/sql/dblink.sql
index 3e96b98571..fa0a307c73 100644
--- a/contrib/dblink/sql/dblink.sql
+++ b/contrib/dblink/sql/dblink.sql
@@ -1,5 +1,6 @@
 CREATE EXTENSION dblink;
-
+-- HINT test
+ALTER FOREIGN DATA WRAPPER dblink_fdw OPTIONS (format 'csv');
 -- want context for notices
 \set SHOW_CONTEXT always
 
diff --git a/contrib/file_fdw/input/file_fdw.source b/contrib/file_fdw/input/file_fdw.source
index 45b728eeb3..d634f70ebf 100644
--- a/contrib/file_fdw/input/file_fdw.source
+++ b/contrib/file_fdw/input/file_fdw.source
@@ -14,6 +14,9 @@ CREATE ROLE regress_no_priv_user LOGIN; -- has priv but no user
 -- Install file_fdw
 CREATE EXTENSION file_fdw;
 
+--HINT test
+ALTER FOREIGN DATA WRAPPER file_fdw OPTIONS (format 'csv');
+
 -- regress_file_fdw_superuser owns fdw-related objects
 SET ROLE regress_file_fdw_superuser;
 CREATE SERVER file_server FOREIGN DATA WRAPPER file_fdw;
diff --git a/contrib/file_fdw/output/file_fdw.source b/contrib/file_fdw/output/file_fdw.source
index 52b4d5f1df..7bcd8a1e9b 100644
--- a/contrib/file_fdw/output/file_fdw.source
+++ b/contrib/file_fdw/output/file_fdw.source
@@ -10,6 +10,10 @@ CREATE ROLE regress_file_fdw_user LOGIN;-- has priv and user map
 CREATE ROLE regress_no_priv_user LOGIN; -- has priv but no user mapping
 -- Install file_fdw
 CREATE EXTENSION file_fdw;
+--HINT test
+ALTER FOREIGN DATA WRAPPER file_fdw OPTIONS (format 'csv');
+ERROR:  invalid option "format"
+HINT:  There are no valid options in this context.
 -- regress_file_fdw_superuser owns fdw-related objects
 SET ROLE regress_file_fdw_superuser;
 CREATE SERVER file_server FOREIGN DATA WRAPPER file_fdw;
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index c7b7db8065..857ed56d62 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -2,6 +2,10 @@
 -- create FDW objects
 -- 

Re: Skipping logical replication transactions on subscriber side

[Greg Nancarrow]

On Tue, Oct 12, 2021 at 4:00 PM Masahiko Sawada  wrote:
>
> I've attached updated patches.
>

Some comments for the v16-0003 patch:

(1) doc/src/sgml/logical-replication.sgml

The output from "SELECT * FROM pg_stat_subscription_errors;" still
shows "last_failed_time" instead of "last_error_time".

doc/src/sgml/ref/alter_subscription.sgml
(2)

Suggested update (and fix typo: restrited -> restricted):

BEFORE:
+  Setting and resetting of skip_xid option is
+  restrited to superusers.
AFTER:
+  The setting and resetting of the
skip_xid option is
+  restricted to superusers.

(3)
Suggested improvement to the wording:

BEFORE:
+  incoming change or by skipping the whole transaction.  This option
+  specifies transaction ID that logical replication worker skips to
+  apply.  The logical replication worker skips all data modification
AFTER:
+  incoming changes or by skipping the whole transaction.  This option
+  specifies the ID of the transaction whose application is to
be skipped
+  by the logical replication worker. The logical replication worker
+  skips all data modification

(4) src/backend/replication/logical/worker.c

Suggested improvement to the comment wording:

BEFORE:
+ * Stop the skipping transaction if enabled. Otherwise, commit the changes
AFTER:
+ * Stop skipping the transaction changes, if enabled. Otherwise,
commit the changes


(5) skip_xid value validation

The validation of the specified skip_xid XID value isn't great.
For example, the following value are accepted:

ALTER SUBSCRIPTION sub SET (skip_xid='123abcz');
ALTER SUBSCRIPTION sub SET (skip_xid='99$@*');


Regards,
Greg Nancarrow
Fujitsu Australia

Re: storing an explicit nonce

[Bruce Momjian]

On Tue, Oct 12, 2021 at 08:40:17AM +0300, Ants Aasma wrote:
> On Mon, 11 Oct 2021 at 22:15, Bruce Momjian  wrote:
> 
> > Yes, that's the direction that I was thinking also and specifically with
> > XTS as the encryption algorithm to allow us to exclude the LSN but keep
> > everything else, and to address the concern around the nonce/tweak/etc
> > being the same sometimes across multiple writes.  Another thing to
> > consider is if we want to encrypt zero'd page.  There was a point
> > brought up that if we do then we are encrypting a fair bit of very
> > predictable bytes and that's not great (though there's a fair bit about
> > our pages that someone could quite possibly predict anyway based on
> > table structures and such...).  I would think that if it's easy enough
> > to not encrypt zero'd pages that we should avoid doing so.  Don't recall
> > offhand which way zero'd pages were being handled already but thought it
> > made sense to mention that as part of this discussion.
> 
> Yeah, I wanted to mention that.  I don't see any security difference
> between fully-zero pages, pages with headers and no tuples, and pages
> with headers and only a few tuples.  If any of those are insecure, they
> all are.  Therefore, I don't see any reason to treat them differently.
> 
> 
> We had to special case zero pages and not encrypt them because as far as I can
> tell, there is no atomic way to extend a file and initialize it to Enc(zero) 
> in
> the same step.

Oh, good point.  Yeah, we will need to handle that.

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

  If only the physical world exists, free will is an illusion.

Re: dfmgr additional ABI version fields

[Peter Eisentraut]

So here is a patch.  This does what I had in mind as a use case. 
Obviously, the naming and wording can be tuned.  Input from other 
vendors is welcome.
From 837680c2195a04bf1f1ecf567fadc3a3d69087fa Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Tue, 12 Oct 2021 14:08:10 +0200
Subject: [PATCH v1] Add ABI extra field to fmgr magic block

This allows derived products to intentionally make their fmgr ABI
incompatible, with a clean error message.

Discussion: 
https://www.postgresql.org/message-id/flat/55215fda-db31-a045-d6b7-d6f2d2dc9920%40enterprisedb.com
---
 src/backend/utils/fmgr/dfmgr.c | 15 +++
 src/include/fmgr.h |  7 ++-
 src/include/pg_config_manual.h | 17 +
 3 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/src/backend/utils/fmgr/dfmgr.c b/src/backend/utils/fmgr/dfmgr.c
index 96fd9d2268..cc1a520070 100644
--- a/src/backend/utils/fmgr/dfmgr.c
+++ b/src/backend/utils/fmgr/dfmgr.c
@@ -330,6 +330,21 @@ incompatible_module_error(const char *libname,
   magic_data.version / 100, 
library_version)));
}
 
+   /*
+* Similarly, if the ABI extra field doesn't match, error out.  Other
+* fields below might also mismatch, but that isn't useful information 
if
+* you're using the wrong product altogether.
+*/
+   if (strcmp(module_magic_data->abi_extra, magic_data.abi_extra) != 0)
+   {
+   ereport(ERROR,
+   (errmsg("incompatible library \"%s\": ABI 
mismatch",
+   libname),
+errdetail("Server has ABI \"%s\", library has 
\"%s\".",
+  magic_data.abi_extra,
+  
module_magic_data->abi_extra)));
+   }
+
/*
 * Otherwise, spell out which fields don't agree.
 *
diff --git a/src/include/fmgr.h b/src/include/fmgr.h
index ab7b85c86e..cec663bdff 100644
--- a/src/include/fmgr.h
+++ b/src/include/fmgr.h
@@ -458,6 +458,7 @@ typedef struct
int indexmaxkeys;   /* INDEX_MAX_KEYS */
int namedatalen;/* NAMEDATALEN */
int float8byval;/* FLOAT8PASSBYVAL */
+   charabi_extra[32];  /* see pg_config_manual.h */
 } Pg_magic_struct;
 
 /* The actual data block contents */
@@ -468,9 +469,13 @@ typedef struct
FUNC_MAX_ARGS, \
INDEX_MAX_KEYS, \
NAMEDATALEN, \
-   FLOAT8PASSBYVAL \
+   FLOAT8PASSBYVAL, \
+   FMGR_ABI_EXTRA, \
 }
 
+StaticAssertDecl(sizeof(FMGR_ABI_EXTRA) <= 
sizeof(((Pg_magic_struct*)0)->abi_extra),
+"FMGR_ABI_EXTRA too long");
+
 /*
  * Declare the module magic function.  It needs to be a function as the dlsym
  * in the backend is only guaranteed to work on functions, not data
diff --git a/src/include/pg_config_manual.h b/src/include/pg_config_manual.h
index 614035e215..225c5b87cc 100644
--- a/src/include/pg_config_manual.h
+++ b/src/include/pg_config_manual.h
@@ -42,6 +42,23 @@
  */
 #define FUNC_MAX_ARGS  100
 
+/*
+ * When creating a product derived from PostgreSQL with changes that cause
+ * incompatibilities for loadable modules, it is recommended to change this
+ * string so that dfmgr.c can refuse to load incompatible modules with a clean
+ * error message.  Typical examples that cause incompatibilities are any
+ * changes to node tags or node structures.  (Note that dfmgr.c already
+ * detects common sources of incompatibilities due to major version
+ * differences and due to some changed compile-time constants.  This setting
+ * is for catching anything that cannot be detected in a straightforward way.)
+ *
+ * There is no prescribed format for the string.  The suggestion is to include
+ * product or company name, and optionally any internally-relevant ABI
+ * version.  Example: "ACME Postgres/1.2".  Note that the string will appear
+ * in a user-facing error message if an ABI mismatch is detected.
+ */
+#define FMGR_ABI_EXTRA "PostgreSQL"
+
 /*
  * Maximum number of columns in an index.  There is little point in making
  * this anything but a multiple of 32, because the main cost is associated
-- 
2.33.0

Re: [Patch] ALTER SYSTEM READ ONLY

[Amul Sul]

On Thu, Oct 7, 2021 at 6:21 PM Amul Sul  wrote:
>
> On Thu, Oct 7, 2021 at 5:56 AM Jaime Casanova
>  wrote:
> >
> > On Tue, Oct 05, 2021 at 04:11:58PM +0530, Amul Sul wrote:
> > >On Mon, Oct 4, 2021 at 1:57 PM Rushabh Lathia
> > >  wrote:
> > > >
> > > > I tried to apply the patch on the master branch head and it's failing
> > > > with conflicts.
> > > >
> > >
> > > Thanks, Rushabh, for the quick check, I have attached a rebased version 
> > > for the
> > > latest master head commit # f6b5d05ba9a.
> > >
> >
> > Hi,
> >
> > I got this error while executing "make check" on src/test/recovery:
> >
> > """
> > t/026_overwrite_contrecord.pl  1/3 # poll_query_until timed out 
> > executing this query:
> > # SELECT '0/201D4D8'::pg_lsn <= pg_last_wal_replay_lsn()
> > # expecting this output:
> > # t
> > # last actual query output:
> > # f
> > # with stderr:
> > # Looks like your test exited with 29 just after 1.
> > t/026_overwrite_contrecord.pl  Dubious, test returned 29 (wstat 
> > 7424, 0x1d00)
> > Failed 2/3 subtests
> >
> > Test Summary Report
> > ---
> > t/026_overwrite_contrecord.pl  (Wstat: 7424 Tests: 1 Failed: 0)
> >   Non-zero exit status: 29
> >   Parse errors: Bad plan.  You planned 3 tests but ran 1.
> > Files=26, Tests=279, 400 wallclock secs ( 0.27 usr  0.10 sys + 73.78 cusr 
> > 59.66 csys = 133.81 CPU)
> > Result: FAIL
> > make: *** [Makefile:23: check] Error 1
> > """
> >
>
> Thanks for the reporting problem, I am working on it. The cause of
> failure is that v37_0004 patch clearing the missingContrecPtr global
> variable before CreateOverwriteContrecordRecord() execution, which it
> shouldn't.
>

In the attached version I have fixed this issue by restoring missingContrecPtr.

To handle abortedRecPtr and missingContrecPtr newly added global
variables thought the commit # ff9f111bce24, we don't need to store
them in the shared memory separately, instead, we need a flag that
indicates a broken record found previously, at the end of recovery, so
that we can overwrite contrecord.

The missingContrecPtr is assigned to the EndOfLog, and we have handled
EndOfLog previously in the 0004 patch, and the abortedRecPtr is the
same as the lastReplayedEndRecPtr, AFAICS.  I have added an assert to
ensure that the lastReplayedEndRecPtr value is the same as the
abortedRecPtr, but I think that is not needed, we can go ahead and
write an overwrite-contrecord starting at lastReplayedEndRecPtr.

Regards,
Amul
From 5bf021226d9742a6fefbcb33e54f7ef044d8fbcc Mon Sep 17 00:00:00 2001
From: Amul Sul 
Date: Thu, 30 Sep 2021 06:29:06 -0400
Subject: [PATCH v38 4/4] Remove dependencies on startup-process specifical
 variables.

To make XLogAcceptWrites(), need to dependency on few global and local
variable spcific to startup process.

Global variables are abortedRecPtr, missingContrecPtr,
ArchiveRecoveryRequested and LocalPromoteIsTriggered, whereas
LocalPromoteIsTriggered can be accessed in any other process using
existing PromoteIsTriggered().  ArchiveRecoveryRequested is made
accessible by copying into shared memory.  abortedRecPtr and
missingContrecPtr can get from the existing shared memory values but
for that, we need a flag indicating that broken records was found
previously and OVERWRITE_CONTRECORD message needs to be written when
WAL writes permitted, added a flag for the same.

XLogAcceptWrites() accepts two argument as EndOfLogTLI and EndOfLog
which are local to StartupXLOG(). Instead of passing as an argument
XLogCtl->replayEndTLI and XLogCtl->lastSegSwitchLSN from the shared
memory can be used as an replacement to EndOfLogTLI and EndOfLog
respectively.  XLogCtl->lastSegSwitchLSN is not going to change until
we use it. That changes only when the current WAL segment gets full
which never going to happen because of two reasons, first WAL writes
are disabled for other processes until XLogAcceptWrites() finishes and
other reasons before use of lastSegSwitchLSN, XLogAcceptWrites() is
writes fix size wal records as full-page write and record for either
recovery end or checkpoint which not going to fill up the 16MB wal
segment.

EndOfLogTLI in the StartupXLOG() is the timeline ID of the last record
that xlogreader reads, but this xlogreader was simply re-fetching the
last record which we have replied in redo loop if it was in recovery,
if not in recovery, we don't need to worry since this value is needed
only in case of ArchiveRecoveryRequested = true, which implicitly
forces redo and sets XLogCtl->replayEndTLI value.
---
 src/backend/access/transam/xlog.c | 90 ---
 1 file changed, 72 insertions(+), 18 deletions(-)

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index cdfec248081..b9596ca005c 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -668,6 +668,13 @@ typedef struct XLogCtlData
 	 */
 	bool		SharedPromoteIsTriggered;
 
+	/*
+	 * SharedArchiveRecoveryRequested exports the va

Re: storing an explicit nonce

[Stephen Frost]

Greetings,

* Bruce Momjian (br...@momjian.us) wrote:
> On Tue, Oct 12, 2021 at 08:40:17AM +0300, Ants Aasma wrote:
> > On Mon, 11 Oct 2021 at 22:15, Bruce Momjian  wrote:
> > 
> > > Yes, that's the direction that I was thinking also and specifically 
> > with
> > > XTS as the encryption algorithm to allow us to exclude the LSN but 
> > keep
> > > everything else, and to address the concern around the nonce/tweak/etc
> > > being the same sometimes across multiple writes.  Another thing to
> > > consider is if we want to encrypt zero'd page.  There was a point
> > > brought up that if we do then we are encrypting a fair bit of very
> > > predictable bytes and that's not great (though there's a fair bit 
> > about
> > > our pages that someone could quite possibly predict anyway based on
> > > table structures and such...).  I would think that if it's easy enough
> > > to not encrypt zero'd pages that we should avoid doing so.  Don't 
> > recall
> > > offhand which way zero'd pages were being handled already but thought 
> > it
> > > made sense to mention that as part of this discussion.
> > 
> > Yeah, I wanted to mention that.  I don't see any security difference
> > between fully-zero pages, pages with headers and no tuples, and pages
> > with headers and only a few tuples.  If any of those are insecure, they
> > all are.  Therefore, I don't see any reason to treat them differently.
> > 
> > 
> > We had to special case zero pages and not encrypt them because as far as I 
> > can
> > tell, there is no atomic way to extend a file and initialize it to 
> > Enc(zero) in
> > the same step.
> 
> Oh, good point.  Yeah, we will need to handle that.

Not sure what's meant here by 'handle that', but I don't see any
particular reason to avoid doing exactly the same for zero pages with
TDE in core..?  I don't think there's any reason we need to make things
complicated to ensure that we encrypt entirely empty pages.

Thanks,

Stephen


signature.asc
Description: PGP signature

Re: storing an explicit nonce

[Bruce Momjian]

On Tue, Oct 12, 2021 at 08:25:52AM -0400, Stephen Frost wrote:
> Greetings,
> 
> * Bruce Momjian (br...@momjian.us) wrote:
> > On Tue, Oct 12, 2021 at 08:40:17AM +0300, Ants Aasma wrote:
> > > On Mon, 11 Oct 2021 at 22:15, Bruce Momjian  wrote:
> > > 
> > > > Yes, that's the direction that I was thinking also and specifically 
> > > with
> > > > XTS as the encryption algorithm to allow us to exclude the LSN but 
> > > keep
> > > > everything else, and to address the concern around the 
> > > nonce/tweak/etc
> > > > being the same sometimes across multiple writes.  Another thing to
> > > > consider is if we want to encrypt zero'd page.  There was a point
> > > > brought up that if we do then we are encrypting a fair bit of very
> > > > predictable bytes and that's not great (though there's a fair bit 
> > > about
> > > > our pages that someone could quite possibly predict anyway based on
> > > > table structures and such...).  I would think that if it's easy 
> > > enough
> > > > to not encrypt zero'd pages that we should avoid doing so.  Don't 
> > > recall
> > > > offhand which way zero'd pages were being handled already but 
> > > thought it
> > > > made sense to mention that as part of this discussion.
> > > 
> > > Yeah, I wanted to mention that.  I don't see any security difference
> > > between fully-zero pages, pages with headers and no tuples, and pages
> > > with headers and only a few tuples.  If any of those are insecure, 
> > > they
> > > all are.  Therefore, I don't see any reason to treat them differently.
> > > 
> > > 
> > > We had to special case zero pages and not encrypt them because as far as 
> > > I can
> > > tell, there is no atomic way to extend a file and initialize it to 
> > > Enc(zero) in
> > > the same step.
> > 
> > Oh, good point.  Yeah, we will need to handle that.
> 
> Not sure what's meant here by 'handle that', but I don't see any
> particular reason to avoid doing exactly the same for zero pages with
> TDE in core..?  I don't think there's any reason we need to make things
> complicated to ensure that we encrypt entirely empty pages.

I thought he was saying that when you extend a file, you might have to
extend it with all zeros, rather than being able to extend it with
an actual encrypted page of zeros.  For example, I think when a page is
corrupt in storage, it reads back as a fully zero page, and we would
need to handle that.  Are you saying we already have logic to handle
that so we don't need to change anything?

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

  If only the physical world exists, free will is an illusion.

Re: storing an explicit nonce

[Stephen Frost]

Greetings,

* Bruce Momjian (br...@momjian.us) wrote:
> On Tue, Oct 12, 2021 at 08:25:52AM -0400, Stephen Frost wrote:
> > * Bruce Momjian (br...@momjian.us) wrote:
> > > On Tue, Oct 12, 2021 at 08:40:17AM +0300, Ants Aasma wrote:
> > > > On Mon, 11 Oct 2021 at 22:15, Bruce Momjian  wrote:
> > > > 
> > > > > Yes, that's the direction that I was thinking also and 
> > > > specifically with
> > > > > XTS as the encryption algorithm to allow us to exclude the LSN 
> > > > but keep
> > > > > everything else, and to address the concern around the 
> > > > nonce/tweak/etc
> > > > > being the same sometimes across multiple writes.  Another thing to
> > > > > consider is if we want to encrypt zero'd page.  There was a point
> > > > > brought up that if we do then we are encrypting a fair bit of very
> > > > > predictable bytes and that's not great (though there's a fair bit 
> > > > about
> > > > > our pages that someone could quite possibly predict anyway based 
> > > > on
> > > > > table structures and such...).  I would think that if it's easy 
> > > > enough
> > > > > to not encrypt zero'd pages that we should avoid doing so.  Don't 
> > > > recall
> > > > > offhand which way zero'd pages were being handled already but 
> > > > thought it
> > > > > made sense to mention that as part of this discussion.
> > > > 
> > > > Yeah, I wanted to mention that.  I don't see any security difference
> > > > between fully-zero pages, pages with headers and no tuples, and 
> > > > pages
> > > > with headers and only a few tuples.  If any of those are insecure, 
> > > > they
> > > > all are.  Therefore, I don't see any reason to treat them 
> > > > differently.
> > > > 
> > > > 
> > > > We had to special case zero pages and not encrypt them because as far 
> > > > as I can
> > > > tell, there is no atomic way to extend a file and initialize it to 
> > > > Enc(zero) in
> > > > the same step.
> > > 
> > > Oh, good point.  Yeah, we will need to handle that.
> > 
> > Not sure what's meant here by 'handle that', but I don't see any
> > particular reason to avoid doing exactly the same for zero pages with
> > TDE in core..?  I don't think there's any reason we need to make things
> > complicated to ensure that we encrypt entirely empty pages.
> 
> I thought he was saying that when you extend a file, you might have to
> extend it with all zeros, rather than being able to extend it with
> an actual encrypted page of zeros.  For example, I think when a page is
> corrupt in storage, it reads back as a fully zero page, and we would
> need to handle that.  Are you saying we already have logic to handle
> that so we don't need to change anything?

When we extend a file, it gets extended with all zeros.  PG already
handles that case, PG w/ TDE would need to also recognize that case
(which is what Ants was saying their patch does) and handle it.  In
other words, we just need to realize when a page is all zeros and not
try to decrypt it when we're reading it.  Ants' patch does that and my
recollection is that it wasn't very complicated to do, and that seems
much simpler than trying to figure out a way to ensure we do encrypt a
zero'd page as part of extending a file.

Thanks,

Stephen


signature.asc
Description: PGP signature

Question about building an exportable snapshop

[Dilip Kumar]

While working on the issue [1], I realize that if a subtransaction
hasn't done any catalog change then we don't add this in the commit
xid list even if we are building a full snapshot [2].  That means when
we will convert this to the MVCC snapshot we will add this to a
running xid list.  If my understanding is correct then how visibility
will work? Because if I look at the code in XidInMVCCSnapshot(), then
if the suboverflowed is not set then first we are going to look into
the snapshot->subxip array and if we don't find it there then we look
into the snapshot->xip array,  and now we will be finding even
committed subxips in the snapshot->xip array.  Am I missing something?

[2]
/*
* Add subtransaction to base snapshot if catalog modifying, we don't
* distinguish to toplevel transactions there.
*/
if (ReorderBufferXidHasCatalogChanges(builder->reorder, subxid))
{
sub_needs_timetravel = true;
needs_snapshot = true;

[3]

[1] 
https://www.postgresql.org/message-id/CAFiTN-tqopqpfS6HHug2nnOGieJJ_nm-Nvy0WBZ%3DZpo-LqtSJA%40mail.gmail.com

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

Re: automatically generating node support functions

[Peter Eisentraut]

On 12.10.21 03:06, Corey Huinker wrote:

build support and made the Perl code more portable, so that the cfbot
doesn't have to be sad.


Was this also the reason for doing the output with print statements 
rather than using one of the templating libraries? I'm mostly just 
curious, and certainly don't want it to get in the way of working code.


Unless there is a templating library that ships with Perl (>= 5.8.3, 
apparently now), this seems impractical.

Re: storing an explicit nonce

[Bruce Momjian]

On Tue, Oct 12, 2021 at 08:49:28AM -0400, Stephen Frost wrote:
> * Bruce Momjian (br...@momjian.us) wrote:
> > I thought he was saying that when you extend a file, you might have to
> > extend it with all zeros, rather than being able to extend it with
> > an actual encrypted page of zeros.  For example, I think when a page is
> > corrupt in storage, it reads back as a fully zero page, and we would
> > need to handle that.  Are you saying we already have logic to handle
> > that so we don't need to change anything?
> 
> When we extend a file, it gets extended with all zeros.  PG already
> handles that case, PG w/ TDE would need to also recognize that case
> (which is what Ants was saying their patch does) and handle it.  In
> other words, we just need to realize when a page is all zeros and not
> try to decrypt it when we're reading it.  Ants' patch does that and my
> recollection is that it wasn't very complicated to do, and that seems
> much simpler than trying to figure out a way to ensure we do encrypt a
> zero'd page as part of extending a file.

Well, how do you detect an all-zero page vs a page that encrypted to all
zeros?  I am thinking a zero LSN (which is not encrypted) would be the
only sure way, but we then have to make sure unlogged relations always
get a fake LSN.

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

  If only the physical world exists, free will is an illusion.

Re: [RFC] building postgres with meson

[Peter Eisentraut]

On 12.10.21 10:37, Andres Freund wrote:

For the last year or so I've on and off tinkered with $subject.  I think
it's in a state worth sharing now.  First, let's look at a little
comparison.


I played with $subject a few years ago and liked it.  I think, like you 
said, meson is the best way forward.  I support this project.


One problem I noticed back then was that some choices that we currently 
determine ourselves in configure or the makefiles are hardcoded in 
meson.  For example, at the time, gcc on macOS was not supported.  Meson 
thought, if you are on macOS, you are surely using the Apple compiler, 
and it supports these options.  Fixing that required patches deep in the 
bowels of the meson source code (and, in practice, waiting for a new 
release etc.).  I strongly suspect this isn't the only such problem. 
For example, the shared library build behavior has been carefully tuned 
in opinionated ways.  With the autotools chain, one can override 
anything with enough violence; so we have always felt free to do that. 
I haven't followed it in a while, so I don't know what the situation is 
now; but it is a concern, because we have always felt free to try new 
and unusual build tools (Sun compiler, Intel compiler, 
clang-when-it-was-new) early without waiting for anyone else.

Re: Make query ID more portable

[Tom Lane]

Andrey Lepikhov  writes:
> But core jumbling code is good, fast and much easier in support.

It won't be fast once you stick a bunch of catalog lookups into it.
I think this is fine as an extension, but it has no chance of being
accepted in core, just on performance grounds.

(I'm also not sure that the query ID calculation code is always/only
invoked in contexts where it's safe to do catalog accesses.)

A bigger issue is that query ID stability isn't something we are going
to promise on a large scale --- for example, what if a new release adds
some new fields to struct Query?  So I'm not sure that "query IDs should
survive dump/reload" is a useful goal to consider.  It's certainly not
something that could be reached by anything even remotely like the
existing code.

regards, tom lane

Re: Make query ID more portable

[Bruce Momjian]

On Tue, Oct 12, 2021 at 09:40:47AM -0400, Tom Lane wrote:
> Andrey Lepikhov  writes:
> > But core jumbling code is good, fast and much easier in support.
> 
> It won't be fast once you stick a bunch of catalog lookups into it.
> I think this is fine as an extension, but it has no chance of being
> accepted in core, just on performance grounds.
> 
> (I'm also not sure that the query ID calculation code is always/only
> invoked in contexts where it's safe to do catalog accesses.)
> 
> A bigger issue is that query ID stability isn't something we are going
> to promise on a large scale --- for example, what if a new release adds
> some new fields to struct Query?  So I'm not sure that "query IDs should
> survive dump/reload" is a useful goal to consider.  It's certainly not
> something that could be reached by anything even remotely like the
> existing code.

Also, the current code handles renames of schemas and objects, but this
would not.

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

  If only the physical world exists, free will is an illusion.

Re: automatically generating node support functions

[Andrew Dunstan]

On 10/11/21 10:22 AM, Peter Eisentraut wrote:
>
> On 15.09.21 21:01, Peter Eisentraut wrote:
>> On 17.08.21 16:36, Peter Eisentraut wrote:
>>> Here is another set of preparatory patches that clean up various
>>> special cases and similar in the node support.
>>
>> This set of patches has been committed.  I'll close this commit fest
>> entry and come back with the main patch series in the future.
>
> Here is an updated version of my original patch, so we have something
> to continue the discussion around.  This takes into account all the
> preparatory patches that have been committed in the meantime.  I have
> also changed it so that the array size of a pointer is now explicitly
> declared using pg_node_attr(array_size(N)) instead of picking the most
> recent scalar field, which was admittedly hacky.  I have also added
> MSVC build support and made the Perl code more portable, so that the
> cfbot doesn't have to be sad.



I haven't been through the whole thing, but I did notice this: the
comment stripping code looks rather fragile. I think it would blow up if
there were a continuation line not starting with  qr/\s*\*/. It's a lot
simpler and more robust to do this if you slurp the file in whole.
Here's what we do in the buildfarm code:

    my $src = file_contents($_);
# strip C comments
    # We used to use the recipe in perlfaq6 but there is actually no point.
    # We don't need to keep the quoted string values anyway, and
    # on some platforms the complex regex causes perl to barf and crash.
    $src =~ s{/\*.*?\*/}{}gs;

After you've done that splitting it into lines is pretty simple.

cheers


andrew

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

Re: Gather performance analysis

[Dilip Kumar]

On Tue, Oct 12, 2021 at 6:41 PM Tomas Vondra
 wrote:
>
> On 9/28/21 14:00, Dilip Kumar wrote:
> >
> > I think that would be great, can we just test this specific target
> > where we are seeing a huge dip with the patch, e.g.
> > with 1000 rows, 10 columns and 4 threads, and queue size 64k.  In
> > my performance machine, I tried to run this test multiple times but on
> > the head, it is taking ~2000 ms whereas with the patch it is ~1500 ms,
> > so I am not able to reproduce this.  So it would be good if you can
> > run only this specific test and repeat it a couple of times on your
> > performance machine.
> >
>
> I ran the benchmark again, with 10 runs instead of 5, the results and
> scripts are attached. It seems the worst case got much better and is now
> in line with the rest of the results, so it probably was a coincidence.

Thanks, yeah now it looks in line with other results.

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

Re: pg14 psql broke \d datname.nspname.relname

[Robert Haas]

On Mon, Oct 11, 2021 at 10:33 PM Peter Geoghegan  wrote:
> On Mon, Oct 11, 2021 at 7:09 PM Mark Dilger
>  wrote:
> > I was just wondering when it might be time to stop being lenient in psql 
> > and instead reject malformed identifiers.
>
> I suppose that I probably wouldn't have chosen this behavior in a
> green field situation. But Hyrum's law tells us that there are bound
> to be some number of users relying on it. I don't think that it's
> worth inconveniencing those people without getting a clear benefit in
> return.
>
> Being lenient here just doesn't have much downside in practice, as
> evidenced by the total lack of complaints about that lenience.

I find it kind of surprising to find everyone agreeing with this
argument. I mean, PostgreSQL users are often quick to criticize MySQL
for accepting -00-00 as a date, because it isn't, and you
shouldn't accept garbage and do stuff with it as if it were valid
data. But by the same argument, accepting a database name that we know
is not correct as a request to show data in the current database seems
wrong to me.

I completely agree that somebody might be relying on the fact that \d
thisdb.someschema.sometable does something sensible when logged into
thisdb, but surely no user is relying on \d
jgldslghksdghjsgkhsdgjhskg.someschema.sometable is going to just
ignore the leading gibberish. Nor do I understand why we'd want to
ignore the leading gibberish. Saying, as Tom did, that nobody has
complained about that behavior is just another way of saying that
nobody tested it. Surely if someone had, it wouldn't be like this.

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

Re: storing an explicit nonce

[Robert Haas]

On Thu, Oct 7, 2021 at 11:05 PM Stephen Frost  wrote:
> Sure, I get that.  Would be awesome if all these things were clearly
> documented somewhere but I've never been able to find it quite as
> explicitly laid out as one would like.

:-(

> specifically: Appendix C: Tweaks
>
> Quoting a couple of paragraphs from that appendix:
>
> """
> In general, if there is information that is available and statically
> associated with a plaintext, it is recommended to use that information
> as a tweak for the plaintext. Ideally, the non-secret tweak associated
> with a plaintext is associated only with that plaintext.
>
> Extensive tweaking means that fewer plaintexts are encrypted under any
> given tweak. This corresponds, in the security model that is described
> in [1], to fewer queries to the target instance of the encryption.
> """
>
> The gist of this being- the more diverse the tweaking being used, the
> better.  That's where I was going with my "limit the risk" comment.  If
> we can make the tweak vary more for a given encryption invokation,
> that's going to be better, pretty much by definition, and as explained
> in publications by NIST.

I mean I don't have anything against that appendix, but I think we
need to understand - with confidence - what the expectations are
specifically around XTS, and that appendix seems much more general
than that.

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

Re: pg14 psql broke \d datname.nspname.relname

[Tom Lane]

Robert Haas  writes:
> On Mon, Oct 11, 2021 at 10:33 PM Peter Geoghegan  wrote:
>> Being lenient here just doesn't have much downside in practice, as
>> evidenced by the total lack of complaints about that lenience.

> I find it kind of surprising to find everyone agreeing with this
> argument.

If the behavior v14 had implemented were "throw an error if the
first word doesn't match the current database name", perhaps nobody
would have questioned it.  But that's not what we have.  It's fairly
clear that neither you nor Mark thought very much about this case,
let alone tested it.  Given that, I am not very pleased that you
are retroactively trying to justify breaking it by claiming that
it was already broken.  It's been that way since 7.3 implemented
schemas, more or less, and nobody's complained about it.  Therefore
I see little argument for changing that behavior.  Changing it in
an already-released branch is especially suspect.

regards, tom lane

Re: storing an explicit nonce

[Robert Haas]

On Mon, Oct 11, 2021 at 1:30 PM Stephen Frost  wrote:
> Regarding unlogged LSNs at least, I would think that we'd want to
> actually use GetFakeLSNForUnloggedRel() instead of just having it zero'd
> out.  The fixed value for GiST index pages is just during the index
> build process, as I recall, and that's perhaps less of a concern.  Part
> of the point of using XTS is to avoid the issue of the LSN not being
> changed when hint bits are, or more generally not being unique in
> various cases.

I don't believe there's anything to prevent the fake-LSN counter from
overtaking the real end-of-WAL, and if that should happen, then the
buffer manager would get confused. Maybe that can be fixed by doing
some sort of surgery on the buffer manager, but it doesn't seem to be
a trivial or ignorable problem.

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

Re: pg14 psql broke \d datname.nspname.relname

[Mark Dilger]

> On Oct 12, 2021, at 7:30 AM, Tom Lane  wrote:
> 
> If the behavior v14 had implemented were "throw an error if the
> first word doesn't match the current database name", perhaps nobody
> would have questioned it.  But that's not what we have.  It's fairly
> clear that neither you nor Mark thought very much about this case,
> let alone tested it.  Given that, I am not very pleased that you
> are retroactively trying to justify breaking it by claiming that
> it was already broken.  It's been that way since 7.3 implemented
> schemas, more or less, and nobody's complained about it.  Therefore
> I see little argument for changing that behavior.  Changing it in
> an already-released branch is especially suspect.

I completely agree that we need to fix this.  My question was only whether 
"fix" means to make it accept database.schema.table or whether it means to 
accept any.prefix.at.all.schema.table.  It sounds like more people like the 
latter, so I'll go with that unless this debate rages on and a different 
conclusion is reached.

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

Re: storing an explicit nonce

[Stephen Frost]

Greetings,

* Robert Haas (robertmh...@gmail.com) wrote:
> On Mon, Oct 11, 2021 at 1:30 PM Stephen Frost  wrote:
> > Regarding unlogged LSNs at least, I would think that we'd want to
> > actually use GetFakeLSNForUnloggedRel() instead of just having it zero'd
> > out.  The fixed value for GiST index pages is just during the index
> > build process, as I recall, and that's perhaps less of a concern.  Part
> > of the point of using XTS is to avoid the issue of the LSN not being
> > changed when hint bits are, or more generally not being unique in
> > various cases.
> 
> I don't believe there's anything to prevent the fake-LSN counter from
> overtaking the real end-of-WAL, and if that should happen, then the
> buffer manager would get confused. Maybe that can be fixed by doing
> some sort of surgery on the buffer manager, but it doesn't seem to be
> a trivial or ignorable problem.

Using fake LSNs isn't new..  how is this not a concern already then?

Also wondering why the buffer manager would care about the LSN on pages
which are not BM_PERMANENT..?

I'll admit that I might certainly be missing something here.

Thanks,

Stephen


signature.asc
Description: PGP signature

Re: pg14 psql broke \d datname.nspname.relname

[Robert Haas]

On Tue, Oct 12, 2021 at 10:31 AM Tom Lane  wrote:
> If the behavior v14 had implemented were "throw an error if the
> first word doesn't match the current database name", perhaps nobody
> would have questioned it.  But that's not what we have.  It's fairly
> clear that neither you nor Mark thought very much about this case,
> let alone tested it.  Given that, I am not very pleased that you
> are retroactively trying to justify breaking it by claiming that
> it was already broken.  It's been that way since 7.3 implemented
> schemas, more or less, and nobody's complained about it.  Therefore
> I see little argument for changing that behavior.  Changing it in
> an already-released branch is especially suspect.

Oh, give me a break. The previous behavior obviously hasn't been
tested either, and is broken on its face. If someone *had* complained
about it, I imagine you would have promptly fixed it and likely
back-patched the fix, probably in under 24 hours from the time of the
report. I find it difficult to take seriously the contention that
anyone is expecting \d dlsgjdsghj.sdhg.l.dsg.jkhsdg.foo.bar to work
like \d foo.bar, or that they would even prefer that behavior over an
error message. You're carefully avoiding addressing that question in
favor of having a discussion of backward compatibility, but a better
term for what we're talking about here would be bug-compatibility.

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

Re: storing an explicit nonce

[Robert Haas]

On Tue, Oct 12, 2021 at 10:39 AM Stephen Frost  wrote:
> Using fake LSNs isn't new..  how is this not a concern already then?
>
> Also wondering why the buffer manager would care about the LSN on pages
> which are not BM_PERMANENT..?
>
> I'll admit that I might certainly be missing something here.

Oh, FlushBuffer has a guard against this case in it. I hadn't realized that.

Sorry for the noise.

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

Re: [RFC] building postgres with meson

[Robert Haas]

On Tue, Oct 12, 2021 at 9:31 AM Peter Eisentraut
 wrote:
> One problem I noticed back then was that some choices that we currently
> determine ourselves in configure or the makefiles are hardcoded in
> meson.  For example, at the time, gcc on macOS was not supported.  Meson
> thought, if you are on macOS, you are surely using the Apple compiler,
> and it supports these options.  Fixing that required patches deep in the
> bowels of the meson source code (and, in practice, waiting for a new
> release etc.).  I strongly suspect this isn't the only such problem.
> For example, the shared library build behavior has been carefully tuned
> in opinionated ways.  With the autotools chain, one can override
> anything with enough violence; so we have always felt free to do that.
> I haven't followed it in a while, so I don't know what the situation is
> now; but it is a concern, because we have always felt free to try new
> and unusual build tools (Sun compiler, Intel compiler,
> clang-when-it-was-new) early without waiting for anyone else.

I think we're going to need some solution to this problem. We have too
many people here with strong opinions about questions like this for me
to feel good about the idea that we're going to collectively be OK
with leaving these sorts of decisions up to some other project.

>From my point of view, the time it takes to run configure is annoying,
but the build time is pretty fine. On my system, configure takes about
33 seconds, and a full rebuild with 'make -j8' takes 14.5 seconds (I
am using ccache). Moreover, most of the time when I run make, I'm only
doing a partial rebuild, so it's near-instantaneous.

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

Re: [RFC] building postgres with meson

[Andrew Dunstan]

On 10/12/21 4:37 AM, Andres Freund wrote:
> git remote add andres g...@github.com:anarazel/postgres.git

ITYM:

git remote add andres git://github.com/anarazel/postgres.git

cheers

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

Re: pg14 psql broke \d datname.nspname.relname

[Stephen Frost]

Greetings,

* Robert Haas (robertmh...@gmail.com) wrote:
> On Tue, Oct 12, 2021 at 10:31 AM Tom Lane  wrote:
> > If the behavior v14 had implemented were "throw an error if the
> > first word doesn't match the current database name", perhaps nobody
> > would have questioned it.  But that's not what we have.  It's fairly
> > clear that neither you nor Mark thought very much about this case,
> > let alone tested it.  Given that, I am not very pleased that you
> > are retroactively trying to justify breaking it by claiming that
> > it was already broken.  It's been that way since 7.3 implemented
> > schemas, more or less, and nobody's complained about it.  Therefore
> > I see little argument for changing that behavior.  Changing it in
> > an already-released branch is especially suspect.
> 
> Oh, give me a break. The previous behavior obviously hasn't been
> tested either, and is broken on its face. If someone *had* complained
> about it, I imagine you would have promptly fixed it and likely
> back-patched the fix, probably in under 24 hours from the time of the
> report. I find it difficult to take seriously the contention that
> anyone is expecting \d dlsgjdsghj.sdhg.l.dsg.jkhsdg.foo.bar to work
> like \d foo.bar, or that they would even prefer that behavior over an
> error message. You're carefully avoiding addressing that question in
> favor of having a discussion of backward compatibility, but a better
> term for what we're talking about here would be bug-compatibility.

I tend to agree with Robert on this particular case.  Accepting random
nonsense there isn't a feature or something which really needs to be
preserved.  For my 2c, I would hope that one day we will be able to
accept other database names there and if that happens, what then?  We'd
"break" these cases anyway.  Better to be clear that such nonsense isn't
intended to be accepted and clean that up.  I do think it'd be good to
accept the current database name there as that's reasonable.

Thanks,

Stephen


signature.asc
Description: PGP signature

Re: [RFC] building postgres with meson

[Josef Šimánek]

.

út 12. 10. 2021 v 10:37 odesílatel Andres Freund  napsal:
>
> Hi,
>
> For the last year or so I've on and off tinkered with $subject.  I think
> it's in a state worth sharing now.  First, let's look at a little
> comparison.
>
> My workstation:
>
> non-cached configure:
> current:11.80s
> meson:   6.67s
>
> non-cached build (world-bin):
> current:40.46s
> ninja:   7.31s
>
> no-change build:
> current: 1.17s
> ninja:   0.06s
>
> test world:
> current:  105s
> meson: 63s
>
>
> What actually started to motivate me however were the long times windows
> builds took to come back with testsresults. On CI, with the same machine
> config:
>
> build:
> current:  202s (doesn't include genbki etc)
> meson+ninja:  140s
> meson+msbuild:206s
>
>
> test:
> current: 1323s (many commands)
> meson:903s (single command)
>
> (note that the test comparison isn't quite fair - there's a few tests
> missing, but it's just small contrib ones afaik)
>
>
> The biggest difference to me however is not the speed, but how readable
> the output is.
>
> Running the tests with meson in a terminal, shows the number of tests
> that completed out of how many total, how much time has passed, how long
> the currently running tests already have been running.
>
> At the end of a testrun a count of tests is shown:
>
> 188/189 postgresql:tap+pg_basebackup / pg_basebackup/t/010_pg_basebackup.pl   
>   OK   39.51s   110 subtests passed
> 189/189 postgresql:isolation+snapshot_too_old / snapshot_too_old/isolation
>   OK   62.93s
>
>
> Ok: 188
> Expected Fail:  0
> Fail:   1
> Unexpected Pass:0
> Skipped:0
> Timeout:0
>
> Full log written to /tmp/meson/meson-logs/testlog.txt
>
>
> The log has the output of the tests and ends with:
>
> Summary of Failures:
> 120/189 postgresql:tap+recovery / recovery/t/007_sync_rep.pl  
>  ERROR 7.16s   (exit status 255 or signal 127 
> SIGinvalid)
>
>
> Quite the difference to make check-world -jnn output.
>
>
> So, now that the teasing is done, let me explain a bit what lead me down
> this path:
>
> Autoconf + make is not being actively developed. Especially autoconf is
> *barely* in maintenance mode - despite many shortcomings and bugs. It's
> also technology that very few want to use - autoconf m4 is scary, and
> it's scarier for people that started more recently than a lot of us
> committers for example.
>
> Recursive make as we use it is hard to get right. One reason the clean
> make build is so slow compared to meson is that we had to resort to
> .NOTPARALLEL to handle dependencies in a bunch of places. And despite
> that, I quite regularly see incremental build failures that can be
> resolved by retrying the build.
>
> While we have incremental build via --enable-depend, they don't work
> that reliable (i.e. misses necessary rebuilds) and yet is often too
> aggressive. More modern build system can keep track of the precise
> command used to build a target and rebuild it when that command changes.
>
>
> We also don't just have the autoconf / make buildsystem, there's also
> the msvc project generator - something most of us unix-y folks do not
> like to touch. I think that, combined with there being no easy way to
> run all tests, and it being just different, really hurt our windows
> developer appeal (and subsequently the quality of postgres on
> windows). I'm not saying this to ding the project generator - that was
> well before there were decent "meta" buildsystems out there (and in some
> ways it is a small one itself).
>
>
> The last big issue I have with the current situation is that there's no
> good test integration. make check-world output is essentially unreadable
> / not automatically parseable. Which led to the buildfarm having a
> separate list of things it needs to test, so that failures can be
> pinpointed and paired with appropriate logs. That approach unfortunately
> doesn't scale well to multi-core CPUs, slowing down the buildfarm by a
> fair bit.
>
>
> This all led to me to experiment with improvements. I tried a few
> somewhat crazy but incremental things like converting our buildsystem to
> non-recursive make (I got it to build the backend, but it's too hard to
> do manually I think), or to not run tests during the recursive make
> check-world, but to append commands to a list of tests, that then is run
> by a helper (can kinda be made to work).  In the end I concluded that
> the amount of time we'd need to invest to maintain our more-and-more
> custom buildsystem going forward doesn't make sense.
>
>
> Which lead me to look around and analyze which other buildsystems there
> are that could make some sense for us. The halfway decent list includes,
> I think:
> 1) cmake
> 2) bazel
> 3) meson
>
>
> cmake would be a decent choic

Re: [RFC] building postgres with meson

[Andrew Dunstan]

On 10/12/21 4:37 AM, Andres Freund wrote:
> # setup build directory
> meson setup build --buildtype debug

I took this for an outing on msys2 and it just seems to hang. If it's not 
hanging it's unbelievably slow.


cheers


andrew


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

Re: [RFC] building postgres with meson

[Tom Lane]

Robert Haas  writes:
> I think we're going to need some solution to this problem. We have too
> many people here with strong opinions about questions like this for me
> to feel good about the idea that we're going to collectively be OK
> with leaving these sorts of decisions up to some other project.

Agreed.  I'm willing to put up with the costs of moving to some
other build system, but not if it dictates choices we don't want to
make about the end products.

> From my point of view, the time it takes to run configure is annoying,
> but the build time is pretty fine. On my system, configure takes about
> 33 seconds, and a full rebuild with 'make -j8' takes 14.5 seconds (I
> am using ccache). Moreover, most of the time when I run make, I'm only
> doing a partial rebuild, so it's near-instantaneous.

Read about Autoconf's --cache-file option.  That and ccache are
absolutely essential tools IMO.

regards, tom lane

Re: [RFC] building postgres with meson

[Andrew Dunstan]

On 10/12/21 11:28 AM, Andrew Dunstan wrote:
> On 10/12/21 4:37 AM, Andres Freund wrote:
>> # setup build directory
>> meson setup build --buildtype debug
> I took this for an outing on msys2 and it just seems to hang. If it's not 
> hanging it's unbelievably slow.
>
>

It hung because it expected the compiler to be 'ccache cc'. Hanging in
such a case is kinda unforgivable. I remedied that by setting 'CC=gcc'
but it then errored out looking for perl libs. I think msys2 is going to
be a bit difficult here :-(


cheers


andrew


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

Re: [RFC] building postgres with meson

[Andres Freund]

Hi,

On 2021-10-12 15:30:57 +0200, Peter Eisentraut wrote:
> I played with $subject a few years ago and liked it.  I think, like you
> said, meson is the best way forward.  I support this project.

Cool.


> One problem I noticed back then was that some choices that we currently
> determine ourselves in configure or the makefiles are hardcoded in meson.

Yea, there's some of that. I think some degree of reduction in flexibility is
needed to realistically target multiple "backend" build-system like visual
studio project files etc. but I wish there were a bit less of that
nonetheless.


> For example, at the time, gcc on macOS was not supported.  Meson thought, if
> you are on macOS, you are surely using the Apple compiler, and it supports
> these options.

I'm pretty sure this one now can just be overridden with CC=gcc. It can on
linux and windows, but I don't have ready interactive access with a mac
(leaving cirrus asside, which now has a "start a terminal" option...).


> For example, the shared library build behavior has been carefully tuned in
> opinionated ways.  With the autotools chain, one can override anything with
> enough violence; so we have always felt free to do that. I haven't followed
> it in a while, so I don't know what the situation is now; but it is a
> concern, because we have always felt free to try new and unusual build tools
> (Sun compiler, Intel compiler, clang-when-it-was-new) early without waiting
> for anyone else.

It's possible to just take over building e.g. shared libraries ourselves with
custom targets. Although it'd be a bit annoying to do. The bigger problem is
that that e.g. wouldn't play that nicely with generating visual studio
projects, which require to generate link steps in a certain way. It'd build,
but the GUI might loose some of its options. Etc.

Greetings,

Andres Freund

Re: pg14 psql broke \d datname.nspname.relname

[Peter Geoghegan]

On Tue, Oct 12, 2021 at 7:41 AM Robert Haas  wrote:
> Oh, give me a break. The previous behavior obviously hasn't been
> tested either, and is broken on its face. If someone *had* complained
> about it, I imagine you would have promptly fixed it and likely
> back-patched the fix, probably in under 24 hours from the time of the
> report.

You're asking us to imagine a counterfactual. But this counterfactual
bug report would have to describe a real practical problem. The
details would matter. It's reasonable to suppose that we haven't seen
such a bug report for a reason.

I can't speak for Tom. My position on this is that it's better to
leave it alone at this time, given the history, and the lack of
complaints from users.

> I find it difficult to take seriously the contention that
> anyone is expecting \d dlsgjdsghj.sdhg.l.dsg.jkhsdg.foo.bar to work
> like \d foo.bar, or that they would even prefer that behavior over an
> error message. You're carefully avoiding addressing that question in
> favor of having a discussion of backward compatibility, but a better
> term for what we're talking about here would be bug-compatibility.

Let's assume that it is bug compatibility. Is that intrinsically a bad thing?

-- 
Peter Geoghegan

Re: pg14 psql broke \d datname.nspname.relname

[Vik Fearing]

On 10/12/21 5:19 PM, Stephen Frost wrote:
> Greetings,
> 
> * Robert Haas (robertmh...@gmail.com) wrote:
>> On Tue, Oct 12, 2021 at 10:31 AM Tom Lane  wrote:
>>> If the behavior v14 had implemented were "throw an error if the
>>> first word doesn't match the current database name", perhaps nobody
>>> would have questioned it.  But that's not what we have.  It's fairly
>>> clear that neither you nor Mark thought very much about this case,
>>> let alone tested it.  Given that, I am not very pleased that you
>>> are retroactively trying to justify breaking it by claiming that
>>> it was already broken.  It's been that way since 7.3 implemented
>>> schemas, more or less, and nobody's complained about it.  Therefore
>>> I see little argument for changing that behavior.  Changing it in
>>> an already-released branch is especially suspect.
>>
>> Oh, give me a break. The previous behavior obviously hasn't been
>> tested either, and is broken on its face. If someone *had* complained
>> about it, I imagine you would have promptly fixed it and likely
>> back-patched the fix, probably in under 24 hours from the time of the
>> report. I find it difficult to take seriously the contention that
>> anyone is expecting \d dlsgjdsghj.sdhg.l.dsg.jkhsdg.foo.bar to work
>> like \d foo.bar, or that they would even prefer that behavior over an
>> error message. You're carefully avoiding addressing that question in
>> favor of having a discussion of backward compatibility, but a better
>> term for what we're talking about here would be bug-compatibility.
> 
> I tend to agree with Robert on this particular case.  Accepting random
> nonsense there isn't a feature or something which really needs to be
> preserved.  For my 2c, I would hope that one day we will be able to
> accept other database names there and if that happens, what then?  We'd
> "break" these cases anyway.  Better to be clear that such nonsense isn't
> intended to be accepted and clean that up.  I do think it'd be good to
> accept the current database name there as that's reasonable.

I am going to throw my hat in with Robert and Stephen, too.  At least
for 15 if we don't want to change this behavior in back branches.
-- 
Vik Fearing

Re: pg14 psql broke \d datname.nspname.relname

[Justin Pryzby]

I understand Tom's position to be that the behavior should be changed back,
since it was 1) unintentional; and 2) breaks legitimate use (when the datname
matches current_database).

I think there's an easy answer here that would satisfy everyone; two patches:
0001 to fix the unintentional behavior change;
0002 to reject garbage input: anything with more than 3 dot-separated
 components, or with 3 components where the first doesn't match
 current_database.

0001 would be backpatched to v14.

If it turns out there's no consensus on 0002, or if it were really hard for
some reason, or (more likely) nobody went to the bother to implement it this
year, then that's okay.

I would prefer if it errored if the datname didn't match the current database.
After all, it would've helped me to avoid making a confusing problem report.

-- 
Justin

Re: [RFC] building postgres with meson

[Andres Freund]

Hi,

On 2021-10-12 11:50:03 -0400, Andrew Dunstan wrote:
> It hung because it expected the compiler to be 'ccache cc'. Hanging in
> such a case is kinda unforgivable. I remedied that by setting 'CC=gcc'
> but it then errored out looking for perl libs. I think msys2 is going to
> be a bit difficult here :-(

Hm. Yea, the perl thing is my fault - you should be able to get past it with
-Dperl=disabled, and I'll take a look at fixing the perl detection. (*)

I can't reproduce the hanging though. I needed to install bison, flex and
ninja and disable perl as described above, but then it built just fine.

It does seems to crash somewhere in the main regression tests though, I think
I don't do the "set stack depth" dance correctly for msys.


If you repro the hanging, what's the last bit in meson-logs/meson-log.txt?


(*) I've for now made most dependencies autodetected, unless you pass
--auto-features disabled to collectively disable all the auto-detected
features. Initially I had mirrored the autoconf behaviour, but I got sick of
forgetting to turn off readline or zlib on windows. And then it was useful to
test on multiple operating systems...

For working on windows meson's wraps are quite useful. I've not added that to
the git branch, but if you manually do
  mkdir subprojects
  meson wrap install lz4
  meson wrap install zlib
building with -Dzlib=enabled -Dlz4=enabled will fall back to building lz4,
zlib as-needed.

I was wondering about adding a binary wrap for e.g. bison, flex on windows, so
that the process of getting a build going isn't as arduous.

Greetings,

Andres Freund

Re: pg14 psql broke \d datname.nspname.relname

[Robert Haas]

On Tue, Oct 12, 2021 at 12:44 PM Peter Geoghegan  wrote:
> You're asking us to imagine a counterfactual. But this counterfactual
> bug report would have to describe a real practical problem.

Yes. And I think this one should be held to the same standard: \d
mydb.myschema.mytable not working is potentially a real, practical
problem. \d sdlgkjdss.dsgkjsk.sdgskldjgds.myschema.mytable not working
isn't.

> Let's assume that it is bug compatibility. Is that intrinsically a bad thing?

Well my view is that having the same bugs is better than having
different ones, but fixing the bugs is superior to either.

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

Re: pg14 psql broke \d datname.nspname.relname

[Robert Haas]

On Tue, Oct 12, 2021 at 12:57 PM Justin Pryzby  wrote:
> I think there's an easy answer here that would satisfy everyone; two patches:
> 0001 to fix the unintentional behavior change;
> 0002 to reject garbage input: anything with more than 3 dot-separated
>  components, or with 3 components where the first doesn't match
>  current_database.
>
> 0001 would be backpatched to v14.
>
> If it turns out there's no consensus on 0002, or if it were really hard for
> some reason, or (more likely) nobody went to the bother to implement it this
> year, then that's okay.

This might work, but I fear that 0001 would end up being substantially
more complicated than a combined patch that solves both problems
together.

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

Re: [RFC] building postgres with meson

[Andres Freund]

Hi,

On 2021-10-12 17:21:50 +0200, Josef Šimánek wrote:
> > # build (uses automatically as many cores as available)
> > ninja
> 
> I'm getting errors at this step. You can find my output at
> https://pastebin.com/Ar5VqfFG. Setup went well without errors. Is that
> expected for now?

Thanks, that's helpful. And no, that's not expected (*), it should be fixed.

What OS / distribution / version is this?

Can you build postgres "normally" with --with-gss? Seems like we're ending up
with a version of gssapi that we're not compatible with.

You should be able to get past this by disabling gss using meson configure
-Dgssapi=disabled.

Greetings,

Andres Freund

* except kinda, in the sense that I'd expect it to be buggy, given that I've
  run it only on a few machines and it's very, uh, bleeding edge

Re: pg14 psql broke \d datname.nspname.relname

[Mark Dilger]

> On Oct 12, 2021, at 10:03 AM, Robert Haas  wrote:
> 
> On Tue, Oct 12, 2021 at 12:57 PM Justin Pryzby  wrote:
>> I think there's an easy answer here that would satisfy everyone; two patches:
>> 0001 to fix the unintentional behavior change;
>> 0002 to reject garbage input: anything with more than 3 dot-separated
>> components, or with 3 components where the first doesn't match
>> current_database.
>> 
>> 0001 would be backpatched to v14.
>> 
>> If it turns out there's no consensus on 0002, or if it were really hard for
>> some reason, or (more likely) nobody went to the bother to implement it this
>> year, then that's okay.
> 
> This might work, but I fear that 0001 would end up being substantially
> more complicated than a combined patch that solves both problems
> together.

Here is a WIP patch that restores the old behavior, just so you can eyeball how 
large it is.  (It passes check-world and I've read it over once, but I'm not 
ready to stand by this as correct quite yet.)  I need to add a regression test 
to make sure this behavior is not accidentally changed in the future, and will 
repost after doing so.



string_utils.patch.WIP
Description: Binary data

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

Re: pg14 psql broke \d datname.nspname.relname

[Mark Dilger]

> On Oct 12, 2021, at 10:01 AM, Robert Haas  wrote:
> 
> On Tue, Oct 12, 2021 at 12:44 PM Peter Geoghegan  wrote:
>> You're asking us to imagine a counterfactual. But this counterfactual
>> bug report would have to describe a real practical problem.
> 
> Yes. And I think this one should be held to the same standard: \d
> mydb.myschema.mytable not working is potentially a real, practical
> problem. \d sdlgkjdss.dsgkjsk.sdgskldjgds.myschema.mytable not working
> isn't.

I favor restoring the v13 behavior, but I don't think \d mydb.myschema.mytable 
was ever legitimate.  You got exactly the same results with \d 
nosuchdb.myschema.mytable, meaning the user was given a false sense of security 
that the database name was being used to fetch the definition from the database 
they specified.

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

Re: refactoring basebackup.c

[Robert Haas]

On Tue, Oct 5, 2021 at 5:51 AM Jeevan Ladhe
 wrote:
> I have fixed the autoFlush issue. Basically, I was wrongly initializing
> the lz4 preferences in bbsink_lz4_begin_archive() instead of
> bbsink_lz4_begin_backup(). I have fixed the issue in the attached
> patch, please have a look at it.

Thanks for the new patch. Seems like this is getting closer, but:

+/*
+ * Read the input buffer in CHUNK_SIZE length in each iteration and pass it to
+ * the lz4 compression. Defined as 8k, since the input buffer is multiple of
+ * BLCKSZ i.e. multiple of 8k.
+ */
+#define CHUNK_SIZE 8192

BLCKSZ does not have to be 8kB.

+ size_t compressedSize;
+ int nextChunkLen = CHUNK_SIZE;
+
+ /* Last chunk to be read from the input. */
+ if (avail_in < CHUNK_SIZE)
+ nextChunkLen = avail_in;

This is the only place where CHUNK_SIZE gets used, and I don't think I
see any point to it. I think the 5th argument to LZ4F_compressUpdate
could just be avail_in. And as soon as you do that then I think
bbsink_lz4_archive_contents() no longer needs to be a loop. For gzip,
the output buffer isn't guaranteed to be big enough to write all the
data, so the compression step can fail to compress all the data. But
LZ4 forces us to make the output buffer big enough that no such
failure can happen. Therefore, that can't happen here except if you
artificially limit the amount of data that you pass to
LZ4F_compressUpdate() to something less than the size of the input
buffer. And I don't see any reason to do that.

+ /* First of all write the frame header to destination buffer. */
+ headerSize = LZ4F_compressBegin(mysink->ctx,
+ mysink->base.bbs_next->bbs_buffer,
+ mysink->base.bbs_next->bbs_buffer_length,
+ &mysink->prefs);

+ compressedSize = LZ4F_compressEnd(mysink->ctx,
+ mysink->base.bbs_next->bbs_buffer + mysink->bytes_written,
+ mysink->base.bbs_next->bbs_buffer_length - mysink->bytes_written,
+ NULL);

I think there's some issue with these two chunks of code. What happens
if one of these functions wants to write more data than will fit in
the output buffer? It seems like either there needs to be some code
someplace that ensures adequate space in the output buffer at the time
of these calls, or else there needs to be a retry loop that writes as
much of the data as possible, flushes the output buffer, and then
loops to generate more output data. But there's clearly no retry loop
here, and I don't see any code that guarantees that the output buffer
has to be large enough (and in the case of LZ4F_compressEnd, have
enough remaining space) either. In other words, all the same concerns
that apply to LZ4F_compressUpdate() also apply here ... but in
LZ4F_compressUpdate() you seem to BOTH have a retry loop and ALSO code
to make sure that the buffer is certain to be large enough (which is
more than you need, you only need one of those) and here you seem to
have NEITHER of those things (which is not enough, you need one or the
other).

+ /* Initialize compressor object. */
+ prefs->frameInfo.blockSizeID = LZ4F_max256KB;
+ prefs->frameInfo.blockMode = LZ4F_blockLinked;
+ prefs->frameInfo.contentChecksumFlag = LZ4F_noContentChecksum;
+ prefs->frameInfo.frameType = LZ4F_frame;
+ prefs->frameInfo.contentSize = 0;
+ prefs->frameInfo.dictID = 0;
+ prefs->frameInfo.blockChecksumFlag = LZ4F_noBlockChecksum;
+ prefs->compressionLevel = 0;
+ prefs->autoFlush = 0;
+ prefs->favorDecSpeed = 0;
+ prefs->reserved[0] = 0;
+ prefs->reserved[1] = 0;
+ prefs->reserved[2] = 0;

How about instead using memset() to zero the whole thing and then
omitting the zero initializations? That seems like it would be less
fragile, if the upstream structure definition ever changes.

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

Re: Improve the HINT message of the ALTER command for postgres_fdw

[Fujii Masao]

On 2021/10/12 19:57, bt21masumurak wrote:

I made new patch based on those comments.


Thanks for updating the patch!

-errhint("HOGEHOGEValid options in this 
context are: %s",
-buf.data)));

The patch contains the garbage "HOGEHOGE" in the above,
which causes the compiler to fail. Attached is the updated version
of the patch. I got rid of the garbage.


+--HINT test
+ALTER FOREIGN DATA WRAPPER file_fdw OPTIONS (format 'csv');

file_fdw already has the test for ALTER FOREIGN DATA WRAPPER .. OPTIONS,
so you don't need to add new test for the command into file_fdw.
I removed that test from file_fdw.


Also I moved the tests for ALTER FOREIGN DATA WRAPPER .. OPTIONS,
in the tests of postgres_fdw, dblink, and foreign data, into more proper
places.


BTW, I found file_fdw.c, dblink.c, postgres_fdw/option.c and foreign.c
use different error codes for the same error message as follows.
They should use the same error code? If yes, ISTM that
ERRCODE_FDW_INVALID_OPTION_NAME is better because
the error message is "invalid option ...".

- ERRCODE_FDW_INVALID_OPTION_NAME (file_fdw.c)
- ERRCODE_FDW_OPTION_NAME_NOT_FOUND (dblink.c)
- ERRCODE_FDW_INVALID_OPTION_NAME (postgres_fdw/option.c)
- ERRCODE_SYNTAX_ERROR (foreign.c)

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
index 3a0beaa88e..c8b0b4ff3f 100644
--- a/contrib/dblink/dblink.c
+++ b/contrib/dblink/dblink.c
@@ -2054,8 +2054,10 @@ dblink_fdw_validator(PG_FUNCTION_ARGS)
ereport(ERROR,

(errcode(ERRCODE_FDW_OPTION_NAME_NOT_FOUND),
 errmsg("invalid option \"%s\"", 
def->defname),
-errhint("Valid options in this context 
are: %s",
-buf.data)));
+buf.len > 0
+? errhint("Valid options in this 
context are: %s",
+  buf.data)
+: errhint("There are no valid options 
in this context.")));
}
}
 
diff --git a/contrib/dblink/expected/dblink.out 
b/contrib/dblink/expected/dblink.out
index 6516d4f131..91cbd744a9 100644
--- a/contrib/dblink/expected/dblink.out
+++ b/contrib/dblink/expected/dblink.out
@@ -917,6 +917,10 @@ REVOKE EXECUTE ON FUNCTION dblink_connect_u(text, text) 
FROM regress_dblink_user
 DROP USER regress_dblink_user;
 DROP USER MAPPING FOR public SERVER fdtest;
 DROP SERVER fdtest;
+-- should fail
+ALTER FOREIGN DATA WRAPPER dblink_fdw OPTIONS (nonexistent 'fdw');
+ERROR:  invalid option "nonexistent"
+HINT:  There are no valid options in this context.
 -- test asynchronous notifications
 SELECT dblink_connect(connection_parameters());
  dblink_connect 
diff --git a/contrib/dblink/sql/dblink.sql b/contrib/dblink/sql/dblink.sql
index 3e96b98571..7a71817d65 100644
--- a/contrib/dblink/sql/dblink.sql
+++ b/contrib/dblink/sql/dblink.sql
@@ -463,6 +463,9 @@ DROP USER regress_dblink_user;
 DROP USER MAPPING FOR public SERVER fdtest;
 DROP SERVER fdtest;
 
+-- should fail
+ALTER FOREIGN DATA WRAPPER dblink_fdw OPTIONS (nonexistent 'fdw');
+
 -- test asynchronous notifications
 SELECT dblink_connect(connection_parameters());
 
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out 
b/contrib/postgres_fdw/expected/postgres_fdw.out
index 44c4367b8f..fd141a0fa5 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -10746,7 +10746,7 @@ DROP TABLE join_tbl;
 ALTER SERVER loopback OPTIONS (DROP async_capable);
 ALTER SERVER loopback2 OPTIONS (DROP async_capable);
 -- ===
--- test invalid server and foreign table options
+-- test invalid server, foreign table and foreign data wrapper options
 -- ===
 -- Invalid fdw_startup_cost option
 CREATE SERVER inv_scst FOREIGN DATA WRAPPER postgres_fdw
@@ -10764,3 +10764,7 @@ ERROR:  invalid value for integer option "fetch_size": 
100$%$#$#
 CREATE FOREIGN TABLE inv_bsz (c1 int )
SERVER loopback OPTIONS (batch_size '100$%$#$#');
 ERROR:  invalid value for integer option "batch_size": 100$%$#$#
+-- No option is allowed to be specified at foreign data wrapper level
+ALTER FOREIGN DATA WRAPPER postgres_fdw OPTIONS (nonexistent 'fdw');
+ERROR:  invalid option "nonexistent"
+HINT:  There are no valid options in this context.
diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c
index 5bb1af4084..48c7417e6e 100644
--- a/contrib/postgres_fdw/option.c
+++ b/contrib/postgres_fdw/option.c
@@ -107,8 +107,10

Re: pg14 psql broke \d datname.nspname.relname

[Robert Haas]

On Tue, Oct 12, 2021 at 1:18 PM Mark Dilger
 wrote:
> Here is a WIP patch that restores the old behavior, just so you can eyeball 
> how large it is.

I guess that's not that bad. Why did we end up with the behavior that
the current comment describes this way?

"(Additional dots in the name portion are not treated as special.)"

I thought there was some reason why it needed to work that way.

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

Re: [RFC] building postgres with meson

[Andres Freund]

Hi,

On 2021-10-12 09:59:26 -0700, Andres Freund wrote:
> On 2021-10-12 11:50:03 -0400, Andrew Dunstan wrote:
> > It hung because it expected the compiler to be 'ccache cc'. Hanging in
> > such a case is kinda unforgivable. I remedied that by setting 'CC=gcc'
> > but it then errored out looking for perl libs. I think msys2 is going to
> > be a bit difficult here :-(
> 
> Hm. Yea, the perl thing is my fault - you should be able to get past it with
> -Dperl=disabled, and I'll take a look at fixing the perl detection. (*)

This is a weird one. I don't know much about msys, so it's probably related to
that. Perl spits out /usr/lib/perl5/core_perl/ as its archlibexp. According to
shell commands that exists, but not according to msys's own python

$ /mingw64/bin/python -c "import os; p = '/usr/lib/perl5/core_perl/CORE'; 
print(f'does {p} exist:', os.path.exists(p))"
does /usr/lib/perl5/core_perl/CORE exist: False

$ ls -ld /usr/lib/perl5/core_perl/CORE
drwxr-xr-x 1 anfreund anfreund 0 Oct 10 10:19 /usr/lib/perl5/core_perl/CORE

So it's not too surprising that that doesn't work out. It's easy enough to
work around, but still pretty weird.

I pushed a workaround for the config-time error, but it doesn't yet recognize
msys perl correctly. But at least it's not alone in that - configure doesn't
seem to either, so I'm probably doing something wrong :)


> I can't reproduce the hanging though. I needed to install bison, flex and
> ninja and disable perl as described above, but then it built just fine.
> 
> It does seems to crash somewhere in the main regression tests though, I think
> I don't do the "set stack depth" dance correctly for msys.

That was it - just hadn't ported setting -Wl,--stack=... for !msvc
windows. Pushed the fix for that out.


I guess I should figure out how to commandline install msys and add it to CI.

Greetings,

Andres Freund

Re: Inconsistency in startup process's MyBackendId and procsignal array registration with ProcSignalInit()

[Fujii Masao]

On 2021/10/12 16:39, Bharath Rupireddy wrote:

Otherwise, we could discard defining MyBackendId in auxprocess.c and
define the MyBackendId in the SharedInvalBackendInit itself as this is
the function that defines the MyBackendId for everyone whoever
requires it. I prefer this approach over what's done in PoC patch.

In SharedInvalBackendInit:
Assert(MyBackendId == InvalidBackendId);
/*
* The startup process requires a valid BackendId for the SI message
* buffer and virtual transaction id, so define it here with the value with
* which the procsignal array slot was allocated in AuxiliaryProcessMain.
* All other auxiliary processes don't need it.
*/
if (MyAuxProcType == StartupProcess)
 MyBackendId = MaxBackends + MyAuxProcType + 1;


Yes, this is an option.

But, at [1], you're proposing to enhance pg_log_backend_memory_contexts()
so that it can send the request to even auxiliary processes. If we need to
assign a backend ID to an auxiliary process other than the startup process
and use it to send the signal promptly to those auxiliary processes,
this design might not be good. Since those auxiliary processes don't call
SharedInvalBackendInit(), backend IDs for them might need to be assigned
outside SharedInvalBackendInit(). Thought?


[1]
https://postgr.es/m/CALj2ACU1nBzpacOK2q=a65s_4+oaz_rltsu1ri0gf7yumnm...@mail.gmail.com

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

Re: [RFC] building postgres with meson

[Andrew Dunstan]

On 10/12/21 12:59 PM, Andres Freund wrote:
>
>
> If you repro the hanging, what's the last bit in meson-logs/meson-log.txt?



Here's the entire thing


# cat 
C:/tools/msys64/home/Administrator/postgresql/build/meson-logs/meson-log.txt
Build started at 2021-10-12T18:08:34.387568
Main binary: C:/tools/msys64/mingw64/bin/python.exe
Build Options: -Dbuildtype=debug
Python system: Windows
The Meson build system
Version: 0.59.1
Source dir: C:/tools/msys64/home/Administrator/postgresql
Build dir: C:/tools/msys64/home/Administrator/postgresql/build
Build type: native build
Project name: postgresql
Project version: 15devel
Sanity testing C compiler: ccache cc
Is cross compiler: False.
Sanity check compiler command line: ccache cc sanitycheckc.c -o
sanitycheckc.exe -D_FILE_OFFSET_BITS=64
Sanity check compile stdout:

-
Sanity check compile stderr:

-

meson.build:1:0: ERROR: Compiler ccache cc can not compile programs.


cheers


andrew

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

Re: [RFC] building postgres with meson

[Andres Freund]

Hi,

On 2021-10-12 14:11:39 -0400, Andrew Dunstan wrote:
> On 10/12/21 12:59 PM, Andres Freund wrote:
> > If you repro the hanging, what's the last bit in meson-logs/meson-log.txt?

> Here's the entire thing

> Sanity check compiler command line: ccache cc sanitycheckc.c -o
> sanitycheckc.exe -D_FILE_OFFSET_BITS=64
> Sanity check compile stdout:
> 
> -
> Sanity check compile stderr:
> 
> -
> 
> meson.build:1:0: ERROR: Compiler ccache cc can not compile programs.

Huh, it's not a question of gcc vs cc, it's that meson automatically uses
ccache. And it looks like msys's ccache is broken at the moment (installed
yesterday):

$ ccache --version
ccache version 4.4.1
...

$ echo > test.c
$ ccache cc -c test.c
Segmentation fault (core dumped)
..

not sure how that leads to hanging, but it's not too surprising that things
don't work out after that...

Greetings,

Andres Freund

Re: [RFC] building postgres with meson

[Andrew Dunstan]

On 10/12/21 2:09 PM, Andres Freund wrote:
> Hi,
>
> On 2021-10-12 09:59:26 -0700, Andres Freund wrote:
>> On 2021-10-12 11:50:03 -0400, Andrew Dunstan wrote:
>>> It hung because it expected the compiler to be 'ccache cc'. Hanging in
>>> such a case is kinda unforgivable. I remedied that by setting 'CC=gcc'
>>> but it then errored out looking for perl libs. I think msys2 is going to
>>> be a bit difficult here :-(
>> Hm. Yea, the perl thing is my fault - you should be able to get past it with
>> -Dperl=disabled, and I'll take a look at fixing the perl detection. (*)
> This is a weird one. I don't know much about msys, so it's probably related to
> that. Perl spits out /usr/lib/perl5/core_perl/ as its archlibexp. According to
> shell commands that exists, but not according to msys's own python
>
> $ /mingw64/bin/python -c "import os; p = '/usr/lib/perl5/core_perl/CORE'; 
> print(f'does {p} exist:', os.path.exists(p))"
> does /usr/lib/perl5/core_perl/CORE exist: False
>
> $ ls -ld /usr/lib/perl5/core_perl/CORE
> drwxr-xr-x 1 anfreund anfreund 0 Oct 10 10:19 /usr/lib/perl5/core_perl/CORE


Looks to me like a python issue:


# perl -e 'my $p = "/usr/lib/perl5/core_perl/CORE"; print qq(does $p
exist: ), -e $p, qq{\n};'
does /usr/lib/perl5/core_perl/CORE exist: 1

# python -c "import os; p = '/usr/lib/perl5/core_perl/CORE';
print(f'does {p} exist:', os.path.exists(p))"
does /usr/lib/perl5/core_perl/CORE exist: False

# cygpath -m /usr/lib/perl5/core_perl/CORE
C:/tools/msys64/usr/lib/perl5/core_perl/CORE

# python -c "import os; p =
'C:/tools/msys64/usr/lib/perl5/core_perl/CORE'; print(f'does {p}
exist:', os.path.exists(p))"
does C:/tools/msys64/usr/lib/perl5/core_perl/CORE exist: True


Clearly python is not understanding msys virtualized paths.


>
>
> I guess I should figure out how to commandline install msys and add it to CI.
>


here's what I do:


# msys2 outputs esc-[3J which clears the screen's scroll buffer. Nasty.
# so we redirect the output
# find the log in c:\Windows\System32 if needed
choco install -y --no-progress --limit-output msys2 > msys2inst.log
c:\tools\msys64\usr\bin\bash -l
'/c/vfiles/windows-uploads/msys2-packages.sh'

Here's what's in msys-packages.sh:


pacman -S --needed --noconfirm \
    base-devel \
    msys/git \
    msys/ccache \
    msys/vim  \
    msys/perl-Crypt-SSLeay \
    mingw-w64-clang-x86_64-toolchain \
    mingw-w64-x86_64-toolchain

# could do: pacman -S --needed --noconfirm development
# this is more economical. These should cover most of the things you
might
# want to configure with

pacman -S --needed --noconfirm \
   msys/gettext-devel \
   msys/icu-devel \
   msys/libiconv-devel \
   msys/libreadline-devel \
   msys/libxml2-devel \
   msys/libxslt-devel \
   msys/openssl-devel \
   msys/zlib-devel


cheers


andrew


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

Re: [RFC] building postgres with meson

[Andrew Dunstan]

On 10/12/21 2:23 PM, Andres Freund wrote:
> Hi,
>
> On 2021-10-12 14:11:39 -0400, Andrew Dunstan wrote:
>> On 10/12/21 12:59 PM, Andres Freund wrote:
>>> If you repro the hanging, what's the last bit in meson-logs/meson-log.txt?
>> Here's the entire thing
>> Sanity check compiler command line: ccache cc sanitycheckc.c -o
>> sanitycheckc.exe -D_FILE_OFFSET_BITS=64
>> Sanity check compile stdout:
>>
>> -
>> Sanity check compile stderr:
>>
>> -
>>
>> meson.build:1:0: ERROR: Compiler ccache cc can not compile programs.
> Huh, it's not a question of gcc vs cc, it's that meson automatically uses
> ccache. And it looks like msys's ccache is broken at the moment (installed
> yesterday):
>
> $ ccache --version
> ccache version 4.4.1
> ...
>
> $ echo > test.c
> $ ccache cc -c test.c
> Segmentation fault (core dumped)
> ..
>
> not sure how that leads to hanging, but it's not too surprising that things
> don't work out after that...
>

Yes, I've had to disable ccache on fairywren.


cheers


andrew

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

Re: [RFC] building postgres with meson

[Andres Freund]

Hi,

On 2021-10-12 09:15:41 -0700, Andres Freund wrote:
> > For example, at the time, gcc on macOS was not supported.  Meson thought, if
> > you are on macOS, you are surely using the Apple compiler, and it supports
> > these options.
>
> I'm pretty sure this one now can just be overridden with CC=gcc. It can on
> linux and windows, but I don't have ready interactive access with a mac
> (leaving cirrus asside, which now has a "start a terminal" option...).

It was a tad more complicated. But only because it took me a while to figure
out how to make gcc on macos actually work, independent of meson. Initially
gcc was always failing with errors about not finding the linker, and
installing binutils was a dead end.

Turns out just using a gcc at a specific path doesn't work, it ends up using
wrong internal binaries or something like that.

Once I got to that, the meson part was easy:

$ export PATH="/usr/local/opt/gcc/bin:$PATH"
$ CC=gcc-11 meson setup build-gcc
  ...
  C compiler for the host machine: gcc-11 (gcc 11.2.0 "gcc-11 (Homebrew GCC 
11.2.0) 11.2.0")
  ...
$ cd build-gcc
$ ninja test
...

  181/181 postgresql:tap+subscription / subscription/t/100_bugs.pl  
   OK   17.83s   5 subtests passed


  Ok: 180
  Expected Fail:  0
  Fail:   0
  Unexpected Pass:0
  Skipped:1
  Timeout:0



One thing that is nice with meson's testrunner is that it can parse the output
of tap tests and recognizes the number of completed / failed subtests. I
wonder whether we could make pg_regress' output tap compliant without the
output quality suffering too much.

Greetings,

Andres Freund

Re: [RFC] building postgres with meson

[Andrew Dunstan]

On 10/12/21 2:37 PM, Andrew Dunstan wrote:
> On 10/12/21 2:09 PM, Andres Freund wrote:
>> Hi,
>>
>> On 2021-10-12 09:59:26 -0700, Andres Freund wrote:
>>> On 2021-10-12 11:50:03 -0400, Andrew Dunstan wrote:
 It hung because it expected the compiler to be 'ccache cc'. Hanging in
 such a case is kinda unforgivable. I remedied that by setting 'CC=gcc'
 but it then errored out looking for perl libs. I think msys2 is going to
 be a bit difficult here :-(
>>> Hm. Yea, the perl thing is my fault - you should be able to get past it with
>>> -Dperl=disabled, and I'll take a look at fixing the perl detection. (*)
>> This is a weird one. I don't know much about msys, so it's probably related 
>> to
>> that. Perl spits out /usr/lib/perl5/core_perl/ as its archlibexp. According 
>> to
>> shell commands that exists, but not according to msys's own python
>>
>> $ /mingw64/bin/python -c "import os; p = '/usr/lib/perl5/core_perl/CORE'; 
>> print(f'does {p} exist:', os.path.exists(p))"
>> does /usr/lib/perl5/core_perl/CORE exist: False
>>
>> $ ls -ld /usr/lib/perl5/core_perl/CORE
>> drwxr-xr-x 1 anfreund anfreund 0 Oct 10 10:19 /usr/lib/perl5/core_perl/CORE
>
> Looks to me like a python issue:
>
>
> # perl -e 'my $p = "/usr/lib/perl5/core_perl/CORE"; print qq(does $p
> exist: ), -e $p, qq{\n};'
> does /usr/lib/perl5/core_perl/CORE exist: 1
>
> # python -c "import os; p = '/usr/lib/perl5/core_perl/CORE';
> print(f'does {p} exist:', os.path.exists(p))"
> does /usr/lib/perl5/core_perl/CORE exist: False
>
> # cygpath -m /usr/lib/perl5/core_perl/CORE
> C:/tools/msys64/usr/lib/perl5/core_perl/CORE
>
> # python -c "import os; p =
> 'C:/tools/msys64/usr/lib/perl5/core_perl/CORE'; print(f'does {p}
> exist:', os.path.exists(p))"
> does C:/tools/msys64/usr/lib/perl5/core_perl/CORE exist: True
>
>
> Clearly python is not understanding msys virtualized paths.


It's a matter of which python you use. The one that understands msys
paths is msys/python. The mingw64 packages are normally pure native
windows and so don't understand msys paths. I know it's confusing :-(


# /usr/bin/python -c "import os; p = '/usr/lib/perl5/core_perl/CORE';
print(f'does {p} exist:', os.path.exists(p))"
does /usr/lib/perl5/core_perl/CORE exist: True


cheers


andrew

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

Re: pg14 psql broke \d datname.nspname.relname

[Mark Dilger]

> On Oct 12, 2021, at 10:54 AM, Robert Haas  wrote:
> 
> On Tue, Oct 12, 2021 at 1:18 PM Mark Dilger
>  wrote:
>> Here is a WIP patch that restores the old behavior, just so you can eyeball 
>> how large it is.
> 
> I guess that's not that bad. Why did we end up with the behavior that
> the current comment describes this way?
> 
> "(Additional dots in the name portion are not treated as special.)"
> 
> I thought there was some reason why it needed to work that way.

We're not talking about the parsing of string literals, but rather about the 
parsing of shell-style patterns.  The primary caller of this logic is 
processSQLNamePattern(), which expects only a relname or a (schema,relname) 
pair, not database names nor anything else.

The pattern myschema.my.*table is not a three-part pattern, but a two part 
pattern, with a literal schema name and a relation name pattern.  In v14 it can 
be seen to work as follows:

\d pg_toast.pg_.oast_2619
TOAST table "pg_toast.pg_toast_2619"
   Column   |  Type
+-
 chunk_id   | oid
 chunk_seq  | integer
 chunk_data | bytea
Owning table: "pg_catalog.pg_statistic"
Indexes:
"pg_toast_2619_index" PRIMARY KEY, btree (chunk_id, chunk_seq)

\d pg_toast.pg_.*_2619
TOAST table "pg_toast.pg_toast_2619"
   Column   |  Type
+-
 chunk_id   | oid
 chunk_seq  | integer
 chunk_data | bytea
Owning table: "pg_catalog.pg_statistic"
Indexes:
"pg_toast_2619_index" PRIMARY KEY, btree (chunk_id, chunk_seq)

In v13, neither of those matched anything (which is defensible, I guess) but 
the following did match, which is really nuts:

+CREATE SCHEMA g_;
+CREATE TABLE g_.oast_2619 (i integer);
+\d pg_toast..g_.oast_2619
+   Table "g_.oast_2619"
+ Column |  Type   | Collation | Nullable | Default 
++-+---+--+-
+ i  | integer |   |  | 


The behavior Justin reported in the original complaint was \d 
regresion.public.bit_defaults, which gets handled as schema =~ /^(regresion)$/ 
and relname =~ /^(public.bit_defaults)$/.  That gives no results for him, but I 
tend to think no results is defensible.

Apparently, this behavior breaks an old bug, and we need to restore the old bug 
and then debate this behavioral change for v15.  I'd rather people had engaged 
in the discussion about this feature during the v14 cycle, since this patch was 
posted and reviewed on -hackers, and I don't recall anybody complaining about 
it.


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

Re: [RFC] building postgres with meson

[Andres Freund]

Hi,

On 2021-10-12 14:37:04 -0400, Andrew Dunstan wrote:
> On 10/12/21 2:09 PM, Andres Freund wrote:
> >> Hm. Yea, the perl thing is my fault - you should be able to get past it 
> >> with
> >> -Dperl=disabled, and I'll take a look at fixing the perl detection. (*)
> > This is a weird one. I don't know much about msys, so it's probably related 
> > to
> > that. Perl spits out /usr/lib/perl5/core_perl/ as its archlibexp. According 
> > to
> > shell commands that exists, but not according to msys's own python
> >
> > $ /mingw64/bin/python -c "import os; p = '/usr/lib/perl5/core_perl/CORE'; 
> > print(f'does {p} exist:', os.path.exists(p))"
> > does /usr/lib/perl5/core_perl/CORE exist: False
> >
> > $ ls -ld /usr/lib/perl5/core_perl/CORE
> > drwxr-xr-x 1 anfreund anfreund 0 Oct 10 10:19 /usr/lib/perl5/core_perl/CORE

> Looks to me like a python issue:

> Clearly python is not understanding msys virtualized paths.

Ah, it's a question of the *wrong* python being used :/. I somehow ended up
with both a mingw and an msys python, with the mingw python taking preference
over the msys one. The latter one does understand such paths.



> > I guess I should figure out how to commandline install msys and add it to 
> > CI.

> here's what I do:

Thanks!


Does that recipe get you to a build where ./configure --with-perl succeeds?

I see this here:

checking for Perl archlibexp... /usr/lib/perl5/core_perl
checking for Perl privlibexp... /usr/share/perl5/core_perl
checking for Perl useshrplib... true
checking for CFLAGS recommended by Perl... -DPERL_USE_SAFE_PUTENV 
-U__STRICT_ANSI__ -D_GNU_SOURCE -march=x86-64 -mtune=generic -O2 -pipe -fwrapv 
-fno-strict-aliasing -fstack-protector-strong
checking for CFLAGS to compile embedded Perl... -DPERL_USE_SAFE_PUTENV
checking for flags to link embedded Perl... no
configure: error: could not determine flags for linking embedded Perl.
This probably means that ExtUtils::Embed or ExtUtils::MakeMaker is not
installed.

If I just include perl.h from a test file with gcc using the above flags it
fails to compile:
$ echo '#include ' > test.c
$ gcc -DPERL_USE_SAFE_PUTENV -U__STRICT_ANSI__ -D_GNU_SOURCE -march=x86-64 
-mtune=generic -O2 -pipe -fwrapv -fno-strict-aliasing -fstack-protector-strong 
test.c -c -I /c/dev/msys64/usr/lib/perl5/core_perl/CORE
In file included from test.c:1:
C:/dev/msys64/usr/lib/perl5/core_perl/CORE/perl.h:1003:13: fatal error: 
sys/wait.h: No such file or directory
 1003 | #   include 

and ldopts bleats

$ perl -MExtUtils::Embed -e ldopts
Warning (mostly harmless): No library found for -lpthread
Warning (mostly harmless): No library found for -ldl
   -Wl,--enable-auto-import -Wl,--export-all-symbols 
-Wl,--enable-auto-image-base -fstack-protector-strong  
-L/usr/lib/perl5/core_perl/CORE -lperl -lcrypt

Greetings,

Andres Freund

Re: [RFC] building postgres with meson

[John Naylor]

On Tue, Oct 12, 2021 at 4:37 AM Andres Freund  wrote:

[Meson prototype]

The build code looks pretty approachable for someone with no prior
exposure, and feels pretty nice when running it (I couldn't get a build
working but I'll leave that aside for now).

> As far as I can tell the only OS that postgres currently supports that
> meson doesn't support is HPUX. It'd likely be fairly easy to add
> gcc-on-hpux support, a chunk more to add support for the proprietary
> ones.

That would also have to work for all the dependencies, which were displayed
to me as:

ninja, gdbm, ca-certificates, openssl@1.1, readline, sqlite and python@3.9

Also, could utility makefile targets be made to work? I'm thinking in
particular of update-unicode and reformat-dat-files, for example.

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

Re: [RFC] building postgres with meson

[Andrew Dunstan]

On 10/12/21 3:29 PM, Andres Freund wrote:
>
> Does that recipe get you to a build where ./configure --with-perl succeeds?
>
> I see this here:
>
> checking for Perl archlibexp... /usr/lib/perl5/core_perl
> checking for Perl privlibexp... /usr/share/perl5/core_perl
> checking for Perl useshrplib... true
> checking for CFLAGS recommended by Perl... -DPERL_USE_SAFE_PUTENV 
> -U__STRICT_ANSI__ -D_GNU_SOURCE -march=x86-64 -mtune=generic -O2 -pipe 
> -fwrapv -fno-strict-aliasing -fstack-protector-strong
> checking for CFLAGS to compile embedded Perl... -DPERL_USE_SAFE_PUTENV
> checking for flags to link embedded Perl... no
> configure: error: could not determine flags for linking embedded Perl.
> This probably means that ExtUtils::Embed or ExtUtils::MakeMaker is not
> installed.
>
> If I just include perl.h from a test file with gcc using the above flags it
> fails to compile:


You need to build against a native perl, like Strawberry or ActiveState.
(I have had mixed success with Strawberry) You do that by putting a path
to it at the start of the PATH. The wrinkle in this is that you need
prove to point to one that understands virtual paths. So you do
something like this:


PATH="/c/perl/bin:$PATH" PROVE=/bin/core_perl/prove configure ...



cheers


andrew


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

Re: storing an explicit nonce

[Ants Aasma]

On Tue, 12 Oct 2021 at 16:14, Bruce Momjian  wrote:

> Well, how do you detect an all-zero page vs a page that encrypted to all
> zeros?
>
Page encrypting to all zeros is for all practical purposes impossible to
hit. Basically an attacker would have to be able to arbitrarily set the
whole contents of the page and they would then achieve that this page gets
ignored.

-- 

Ants Aasma
Senior Database Engineerwww.cybertec-postgresql.com

Re: [RFC] building postgres with meson

[Andres Freund]

Hi,

On 2021-10-12 16:02:14 -0400, Andrew Dunstan wrote:
> You need to build against a native perl, like Strawberry or ActiveState.
> (I have had mixed success with Strawberry)

Do you understand why that is needed?


> You do that by putting a path to it at the start of the PATH. The wrinkle in
> this is that you need prove to point to one that understands virtual
> paths. So you do something like this:
> 
> 
> PATH="/c/perl/bin:$PATH" PROVE=/bin/core_perl/prove configure ...

Oh my.

I'll try that later... I wonder if we could make this easier from our side?
This is a lot of magic to know.

Greetings,

Andres Freund

Re: [RFC] building postgres with meson

[Andres Freund]

Hi,

On 2021-10-12 15:55:22 -0400, John Naylor wrote:
> On Tue, Oct 12, 2021 at 4:37 AM Andres Freund  wrote:
> The build code looks pretty approachable for someone with no prior
> exposure, and feels pretty nice when running it

That's part of what attracted me...


> (I couldn't get a build working but I'll leave that aside for now).

If you want to do that separately, I'll try to fix it.


> > As far as I can tell the only OS that postgres currently supports that
> > meson doesn't support is HPUX. It'd likely be fairly easy to add
> > gcc-on-hpux support, a chunk more to add support for the proprietary
> > ones.
> 
> That would also have to work for all the dependencies, which were displayed
> to me as:
> 
> ninja, gdbm, ca-certificates, openssl@1.1, readline, sqlite and python@3.9

meson does depend on ninja (to execute the build) and of course python. But
the rest should be optional dependencies. ninja builds without any
dependencies as long as you don't change its parser sources. python builds on
aix, hpux etc.

Not sure what way gdbm openssl@1.1 and sqlite are pulled in? I assume readline
is for python...


> Also, could utility makefile targets be made to work? I'm thinking in
> particular of update-unicode and reformat-dat-files, for example.

Yes, that shouldn't be a problem. You can run arbitrary code in targets
(there's plenty need for that already in what I have so far).

Greetings,

Andres Freund

Re: pg14 psql broke \d datname.nspname.relname

[Mark Dilger]

> On Oct 12, 2021, at 10:18 AM, Mark Dilger  
> wrote:
> 
> Here is a WIP patch that restores the old behavior, just so you can eyeball 
> how large it is.  (It passes check-world and I've read it over once, but I'm 
> not ready to stand by this as correct quite yet.)  I need to add a regression 
> test to make sure this behavior is not accidentally changed in the future, 
> and will repost after doing so.

I wasn't thinking critically enough about how psql handles \d when I accepted 
Justin's initial characterization of the bug.  The psql client has never 
thought about the stuff to the left of the schema name as a database name, even 
if some users thought about it that way.  It also doesn't think about the 
pattern as a literal string.

The psql client's interpretation of the pattern is a bit of a chimera, 
following shell glob patterns for some things and POSIX regex rules for others. 
 The reason for that is shell glob stuff gets transliterated into the 
corresponding POSIX syntax, but non-shell-glob stuff is left in tact, with the 
one outlier being dots, which have a very special interpretation.  The 
interpretation of a dot as meaning "match one character" is not a shell glob 
rule but a regex one, and one that psql never supported because it split the 
pattern on all dots and threw away stuff to the left.  There was therefore 
never an opportunity for an unquoted dot to make it through to the POSIX 
regular expression for processing.  For other regex type stuff, it happily 
passed it through to the POSIX regex, so that the following examples work even 
though they contain non-shell-glob regex stuff:

v13=# create table ababab (i integer);
CREATE TABLE

v13=# \dt (ab){3}
   List of relations
 Schema |  Name  | Type  |Owner
++---+-
 public | ababab | table | mark.dilger
(1 row)

v13=# \dt pg_catalog.pg_clas{1,2}
  List of relations
   Schema   |   Name   | Type  |Owner
+--+---+-
 pg_catalog | pg_class | table | mark.dilger

v13=# \dt pg_catalog.pg_[am]{1,3}
List of relations
   Schema   | Name  | Type  |Owner
+---+---+-
 pg_catalog | pg_am | table | mark.dilger
(1 row)

Splitting the pattern on all the dots and throwing away any additional leftmost 
fields is a bug, and when you stop doing that, passing additional dots through 
to the POSIX regular expression for processing is the most natural thing to do. 
 This is, in fact, how v14 works.  It is a bit debatable whether treating the 
first dot as a separator and the additional dots as stuff to be passed through 
is the right thing, so we could call the v14 behavior a mis-feature, but it's 
not as clearcut as the discussion upthread suggested.  Reverting to v13 
behavior seems wrong, but I'm now uncertain how to proceed.

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

Re: storing an explicit nonce

[Bruce Momjian]

On Tue, Oct 12, 2021 at 11:21:28PM +0300, Ants Aasma wrote:
> On Tue, 12 Oct 2021 at 16:14, Bruce Momjian  wrote:
> 
> Well, how do you detect an all-zero page vs a page that encrypted to all
> zeros?
> 
> Page encrypting to all zeros is for all practical purposes impossible to hit.
> Basically an attacker would have to be able to arbitrarily set the whole
> contents of the page and they would then achieve that this page gets ignored.

Uh, how do we know that valid data can't produce an encrypted all-zero
page?

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

  If only the physical world exists, free will is an illusion.

Re: storing an explicit nonce

[Ants Aasma]

On Wed, 13 Oct 2021 at 00:25, Bruce Momjian  wrote:

> On Tue, Oct 12, 2021 at 11:21:28PM +0300, Ants Aasma wrote:
> > On Tue, 12 Oct 2021 at 16:14, Bruce Momjian  wrote:
> >
> > Well, how do you detect an all-zero page vs a page that encrypted to
> all
> > zeros?
> >
> > Page encrypting to all zeros is for all practical purposes impossible to
> hit.
> > Basically an attacker would have to be able to arbitrarily set the whole
> > contents of the page and they would then achieve that this page gets
> ignored.
>
> Uh, how do we know that valid data can't produce an encrypted all-zero
> page?
>

Because the chances of that happening by accident are equivalent to making
a series of commits to postgres and ending up with the same git commit hash
400 times in a row.

--

Ants Aasma
Senior Database Engineerwww.cybertec-postgresql.com

Re: storing an explicit nonce

[Stephen Frost]

Greetings,

On Tue, Oct 12, 2021 at 17:49 Ants Aasma  wrote:

>
> On Wed, 13 Oct 2021 at 00:25, Bruce Momjian  wrote:
>
>> On Tue, Oct 12, 2021 at 11:21:28PM +0300, Ants Aasma wrote:
>> > On Tue, 12 Oct 2021 at 16:14, Bruce Momjian  wrote:
>> >
>> > Well, how do you detect an all-zero page vs a page that encrypted
>> to all
>> > zeros?
>> >
>> > Page encrypting to all zeros is for all practical purposes impossible
>> to hit.
>> > Basically an attacker would have to be able to arbitrarily set the whole
>> > contents of the page and they would then achieve that this page gets
>> ignored.
>>
>> Uh, how do we know that valid data can't produce an encrypted all-zero
>> page?
>>
>
> Because the chances of that happening by accident are equivalent to making
> a series of commits to postgres and ending up with the same git commit hash
> 400 times in a row.
>

And to then have a valid checksum … seems next to impossible.

Thanks,

Stephen

>

Re: Temporary tables versus wraparound... again

[Greg Stark]

Here's an updated patch. I added some warning messages to autovacuum.

One thing I learned trying to debug this situation in production is
that it's nigh impossible to find the pid of the session using a
temporary schema. The number in the schema refers to the backendId in
the sinval stuff for which there's no external way to look up the
corresponding pid. It would have been very helpful if autovacuum had
just told me which backend pid to kill.

I still have the regression test in the patch and as before I think
it's probably not worth committing. I'm leaning to reverting that
section of the patch before comitting.

Incidentally there's still a hole here where a new session can attach
to an existing temporary schema where a table was never truncated due
to a session dieing abnormally. That new session could be a long-lived
session but never use the temporary schema causing the old table to
just sit there. Autovacuum has no way to tell it's not actually in
use. I tend to think the optimization to defer cleaning the temporary
schema until it's used might not really be an optimization. It still
needs to be cleaned someday so it's just moving the work around. Just
removing that optimization might be the easiest way to close this
hole. The only alternative I see is adding a flag to PROC or somewhere
where autovacuum can see if the backend has actually initialized the
temporary schema yet or not.
From 1315daf80e668b03cf2aab04106fe53ad606c9d0 Mon Sep 17 00:00:00 2001
From: Greg Stark 
Date: Tue, 12 Oct 2021 17:17:51 -0400
Subject: [PATCH v2] Update relfrozenxmin when truncating temp tables

Make ON COMMIT DELETE ROWS reset relfrozenxmin and other table stats
like normal truncate. Otherwise even typical short-lived transactions
using temporary tables can easily cause them to reach relfrozenxid.

Also add warnings when old temporary tables are found to still be in
use during autovacuum. Long lived sessions using temporary tables are
required to vacuum them themselves.

For the warning to be useful modify checkTempNamespaceStatus to return
the backend pid using it so that we can inform super-user which pid to
terminate. Otherwise it's quite tricky to determine as a user.
---
 src/backend/access/transam/varsup.c | 12 ++---
 src/backend/catalog/heap.c  | 53 +
 src/backend/catalog/namespace.c |  9 +--
 src/backend/postmaster/autovacuum.c | 48 -
 src/include/catalog/namespace.h |  2 +-
 src/test/regress/expected/temp.out  | 21 +++
 src/test/regress/parallel_schedule  |  9 ---
 src/test/regress/sql/temp.sql   | 19 +
 8 files changed, 151 insertions(+), 22 deletions(-)

diff --git a/src/backend/access/transam/varsup.c b/src/backend/access/transam/varsup.c
index a6e98e7..84ccd2f 100644
--- a/src/backend/access/transam/varsup.c
+++ b/src/backend/access/transam/varsup.c
@@ -129,14 +129,16 @@ GetNewTransactionId(bool isSubXact)
 		 errmsg("database is not accepting commands to avoid wraparound data loss in database \"%s\"",
 oldest_datname),
 		 errhint("Stop the postmaster and vacuum that database in single-user mode.\n"
- "You might also need to commit or roll back old prepared transactions, or drop stale replication slots.")));
+ "You might also need to commit or roll back old prepared transactions,\n"
+ "drop temporary tables, or drop stale replication slots.")));
 			else
 ereport(ERROR,
 		(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
 		 errmsg("database is not accepting commands to avoid wraparound data loss in database with OID %u",
 oldest_datoid),
 		 errhint("Stop the postmaster and vacuum that database in single-user mode.\n"
- "You might also need to commit or roll back old prepared transactions, or drop stale replication slots.")));
+ "You might also need to commit or roll back old prepared transactions,\n"
+ "drop temporary tables, or drop stale replication slots.")));
 		}
 		else if (TransactionIdFollowsOrEquals(xid, xidWarnLimit))
 		{
@@ -149,14 +151,16 @@ GetNewTransactionId(bool isSubXact)
 oldest_datname,
 xidWrapLimit - xid),
 		 errhint("To avoid a database shutdown, execute a database-wide VACUUM in that database.\n"
- "You might also need to commit or roll back old prepared transactions, or drop stale replication slots.")));
+ "You might also need to commit or roll back old prepared transactions,\n"
+ "drop temporary tables, or drop stale replication slots.")));
 			else
 ereport(WARNING,
 		(errmsg("database with OID %u must be vacuumed within %u transactions",
 oldest_datoid,
 xidWrapLimit - xid),
 		 errhint("To avoid a database shutdown, execute a database-wide VACUUM in that database.\n"
- "You might also need to commit or roll back old prepared transactions, or drop stale replication slots.")));
+ "You 

Re: Inconsistency in startup process's MyBackendId and procsignal array registration with ProcSignalInit()

[Andres Freund]

Hi,

On 2021-10-11 15:24:46 +0900, Fujii Masao wrote:
> How about modifying SharedInvalBackendInit() so that it accepts
> BackendId as an argument and allocates the ProcState entry of
> the specified BackendId? That is, the startup process determines
> that its BackendId is "MaxBackends + MyAuxProcType (=StartupProcess) + 1"
> in AuxiliaryProcessMain(), and then it passes that BackendId to
> SharedInvalBackendInit() in InitRecoveryTransactionEnvironment().

If I understand correctly what you're proposing, I think that's going in the
wrong direction. We should work towards getting rid of BackendIds
instead. This whole complication vanishes if we make sinvaladt use pgprocno.

See https://postgr.es/m/20210802171255.k4yv5cfqaqbuuy6f%40alap3.anarazel.de
for some discussion of this.

Greetings,

Andres Freund

Re: [BUG] Logical replication failure "ERROR: could not map filenode "base/13237/442428" to relation OID" with catalog modifying txns

[Jeremy Schneider]

On 10/10/21 23:27, Masahiko Sawada wrote:
> 
> After more thought, given DDLs are not likely to happen than DML in
> practice, ...

I haven't looked closely at the patch, but I'd be careful about
workloads where people create and drop "temporary tables". I've seen
this pattern used a few times, especially by developers who came from a
SQL server background, for some reason.

I certainly don't think we need to optimize for this workload - which is
not a best practice on PostreSQL. I'd just want to be careful not to
make PostgreSQL logical replication crumble underneath it, if PG was
previously keeping up with difficulty. That would be a sad upgrade
experience!

-Jeremy

-- 
http://about.me/jeremy_schneider

Re: [RFC] building postgres with meson

[Josef Šimánek]

út 12. 10. 2021 v 19:17 odesílatel Andres Freund  napsal:
>
> Hi,
>
> On 2021-10-12 17:21:50 +0200, Josef Šimánek wrote:
> > > # build (uses automatically as many cores as available)
> > > ninja
> >
> > I'm getting errors at this step. You can find my output at
> > https://pastebin.com/Ar5VqfFG. Setup went well without errors. Is that
> > expected for now?
>
> Thanks, that's helpful. And no, that's not expected (*), it should be fixed.
>
> What OS / distribution / version is this?

Fedora 34 (64 bit)

> Can you build postgres "normally" with --with-gss? Seems like we're ending up
> with a version of gssapi that we're not compatible with.

Yes, I can.

> You should be able to get past this by disabling gss using meson configure
> -Dgssapi=disabled.

I tried to clean and start from scratch, but I'm getting different
error probably related to wrongly configured JIT (LLVM wasn't found
during meson setup). I'll debug on my side to provide more info.

Whole build error could be found at https://pastebin.com/hCFqcPvZ.
Setup log could be found at https://pastebin.com/wjbE1w56.

> Greetings,
>
> Andres Freund
>
> * except kinda, in the sense that I'd expect it to be buggy, given that I've
>   run it only on a few machines and it's very, uh, bleeding edge

Re: storing an explicit nonce

[Bruce Momjian]

On Wed, Oct 13, 2021 at 12:48:51AM +0300, Ants Aasma wrote:
> On Wed, 13 Oct 2021 at 00:25, Bruce Momjian  wrote:
> 
> On Tue, Oct 12, 2021 at 11:21:28PM +0300, Ants Aasma wrote:
> > Page encrypting to all zeros is for all practical purposes impossible to
> hit.
> > Basically an attacker would have to be able to arbitrarily set the whole
> > contents of the page and they would then achieve that this page gets
> ignored.
> 
> Uh, how do we know that valid data can't produce an encrypted all-zero
> page?
> 
> 
> Because the chances of that happening by accident are equivalent to making a
> series of commits to postgres and ending up with the same git commit hash 400
> times in a row.

Yes, 256^8192 is 1e+19728, but why not just assume a page LSN=0 is an
empty page, and if not, an error?  Seems easier than checking if each
page contains all zeros every time.

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

  If only the physical world exists, free will is an illusion.

Re: [RFC] building postgres with meson

[Andres Freund]

Hi,

On 2021-10-13 01:19:27 +0200, Josef Šimánek wrote:
> I tried to clean and start from scratch, but I'm getting different
> error probably related to wrongly configured JIT (LLVM wasn't found
> during meson setup). I'll debug on my side to provide more info.

../src/backend/jit/jit.c:91:73: error: ‘DLSUFFIX’ undeclared (first use in this 
function)
   91 | snprintf(path, MAXPGPATH, "%s/%s%s", pkglib_path, jit_provider, 
DLSUFFIX);
  | 
^~~~

This *very* likely is related to building in a source tree that also contains
a "non-meson" build "in place". The problem is that the meson build picks up
the pg_config.h generated by ./configure in the "normal" build, rather than
the one meson generated itself.

You'd need to execute make distclean or such, or use a separate git checkout.

I forgot about this issue because I only ever build postgres from outside the
source-tree (by invoking configure from a separate directory), so there's
never build products in it. I think at least I need to make the build emit a
warning / error if there's a pg_config.h in the source tree...


This is the part of the jit code that's built regardless of llvm availability
- you'd get the same error in a few other places unrelated to jit.

Greetings,

Andres Freund

Re: Inconsistency in startup process's MyBackendId and procsignal array registration with ProcSignalInit()

[Kyotaro Horiguchi]

At Tue, 12 Oct 2021 14:57:58 +0530, Bharath Rupireddy 
 wrote in 
> On Tue, Oct 12, 2021 at 2:03 PM Kyotaro Horiguchi
>  wrote:
> > > [1] - 
> > > https://www.postgresql.org/message-id/CALj2ACXZ_o7rcOb7-Rs96P0d%3DEi%2Bnvf_WZ-Meky7Vv%2BnQNFYjQ%40mail.gmail.com
> >
> > The patch does this:
> >
> > case StartupProcess:
> > +   MyBackendId = MaxBackends + MyAuxProcType + 1;
> >
> > as well as this:
> >
> > +   shmInvalBuffer->maxBackends = MaxBackends + 1;
> >
> > These don't seem to be in the strict correspondence.  I'd prefer
> > something like the following.
> >
> > +   /* currently only StartupProcess uses nailed SI slot */
> > +   shmInvalBuffer->maxBackends = MaxBackends + StartupProcess + 1;
> 
> I don't think it is a good idea to use macro StartupProcess (because
> the macro might get changed to a different value later). What we

If wo, we shouldn't use MyAuxProcType at the "case StartupProcess".

> essentially need to do for procState array is to extend its size by 1
> (for startup process) which is being handled separately in [1]. Once
> the patch at [1] gets in, the patch proposed here will not have the
> above change at all.
> 
> [1] -  
> https://www.postgresql.org/message-id/CALj2ACXZ_o7rcOb7-Rs96P0d%3DEi%2Bnvf_WZ-Meky7Vv%2BnQNFYjQ%40mail.gmail.com

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: should we allow users with a predefined role to access pg_backend_memory_contexts view and pg_log_backend_memory_contexts function?

[Bossart, Nathan]

On 10/9/21, 2:12 AM, "Bharath Rupireddy" 
 wrote:
> Here's the v1, please review it further.

Thanks for the patch.

-   /* Only allow superusers to log memory contexts. */
-   if (!superuser())
+   /*
+* Only superusers or members of pg_read_all_stats can log memory 
contexts.
+*/
+   if (!is_member_of_role(GetUserId(), ROLE_PG_READ_ALL_STATS))

I personally think pg_log_backend_memory_contexts() should remain
restricted to superusers since it directly impacts the server log.
However, if we really did want to open it up to others, couldn't we
add GRANT/REVOKE statements in system_functions.sql and remove the
hard-coded superuser check?  I think that provides a bit more
flexibility (e.g., permission to execute it can be granted to others
without giving them pg_read_all_stats).

Nathan

Re: BUG #17220: ALTER INDEX ALTER COLUMN SET (..) with an optionless opclass makes index and table unusable

[Vik Fearing]

On 10/13/21 2:06 AM, Bossart, Nathan wrote:
> On 10/11/21, 11:03 AM, "Vik Fearing"  wrote:
>> On 10/11/21 5:25 PM, PG Bug reporting form wrote:
>>>
>>> User 'musttu' on IRC reported the following bug: After running "ALTER INDEX
>>> some_idx ALTER COLUMN expr SET (n_distinct=100)", the index and table become
>>> unusable. All further statements involving the table result in: "ERROR:
>>> operator class text_ops has no options".
>>>
>>> They reported this on the RDS version of 13.3, but I've been able to
>>> reproduce this on Debian with 13.4 and 14.0. It does not reproduce on 12.8,
>>> all statements succeed on that version.
>>
>> This was broken by 911e702077 (Implement operator class parameters).
> 
> Moving to pgsql-hackers@.
> 
> At first glance, it looks like ALTER INDEX .. ALTER COLUMN ... SET
> uses the wrong validation function.  I've attached a patch where I've
> attempted to fix that and added some tests.

Ah, thank you.  I was in the (slow) process of writing basically this
exact patch.  So I'll stop now and endorse yours.
-- 
Vik Fearing

Re: should we allow users with a predefined role to access pg_backend_memory_contexts view and pg_log_backend_memory_contexts function?

[Stephen Frost]

Greetings,

On Tue, Oct 12, 2021 at 20:26 Bossart, Nathan  wrote:

> On 10/9/21, 2:12 AM, "Bharath Rupireddy" <
> bharath.rupireddyforpostg...@gmail.com> wrote:
> > Here's the v1, please review it further.
>
> Thanks for the patch.
>
> -   /* Only allow superusers to log memory contexts. */
> -   if (!superuser())
> +   /*
> +* Only superusers or members of pg_read_all_stats can log memory
> contexts.
> +*/
> +   if (!is_member_of_role(GetUserId(), ROLE_PG_READ_ALL_STATS))
>
> I personally think pg_log_backend_memory_contexts() should remain
> restricted to superusers since it directly impacts the server log.
> However, if we really did want to open it up to others, couldn't we
> add GRANT/REVOKE statements in system_functions.sql and remove the
> hard-coded superuser check?  I think that provides a bit more
> flexibility (e.g., permission to execute it can be granted to others
> without giving them pg_read_all_stats).


I would think we would do both…. That is- move to using GRANT/REVOKE, and
then just include a GRANT to pg_read_all_stats.

Or not. I can see the argument that, because it just goes into the log,
that it doesn’t make sense to grant to a predefined role, since that role
wouldn’t be able to see the results even if it had access.

Thanks,

Stephen

>

Re: [RFC] building postgres with meson

[Andres Freund]

Hi,

On 2021-10-12 13:42:56 -0700, Andres Freund wrote:
> On 2021-10-12 16:02:14 -0400, Andrew Dunstan wrote:
> > You do that by putting a path to it at the start of the PATH. The wrinkle in
> > this is that you need prove to point to one that understands virtual
> > paths. So you do something like this:
> > 
> > 
> > PATH="/c/perl/bin:$PATH" PROVE=/bin/core_perl/prove configure ...
> 
> Oh my.
> 
> I'll try that later... I wonder if we could make this easier from our side?
> This is a lot of magic to know.

I managed to get this working. At first it failed because I don't have
pexports - it's not available inside msys as far as I could tell. And seems to
be unmaintained. But replacing pexports with gendef fixed that.

There's this comment in src/pl/plperl/GNUmakefile

# Perl on win32 ships with import libraries only for Microsoft Visual C++,
# which are not compatible with mingw gcc. Therefore we need to build a
# new import library to link with.

but I seem to be able to link fine without going through that song-and-dance?

Greetings,

Andres Freund

Re: should we allow users with a predefined role to access pg_backend_memory_contexts view and pg_log_backend_memory_contexts function?gr

[Michael Paquier]

On Tue, Oct 12, 2021 at 08:33:19PM -0400, Stephen Frost wrote:
> I would think we would do both…. That is- move to using GRANT/REVOKE, and
> then just include a GRANT to pg_read_all_stats.
> 
> Or not. I can see the argument that, because it just goes into the log,
> that it doesn’t make sense to grant to a predefined role, since that role
> wouldn’t be able to see the results even if it had access.

I don't think that this is a bad thing to remove the superuser() check
and replace it with a REVOKE FROM PUBLIC in this case, but linking the
logging of memory contexts with pg_read_all_stats does not seem right
to me.
--
Michael


signature.asc
Description: PGP signature

Re: pg_upgrade test for binary compatibility of core data types

[Michael Paquier]

On Mon, Oct 11, 2021 at 02:38:12PM +0900, Michael Paquier wrote:
> For now, attached is a patch to address the issues with test.sh that I
> am planning to backpatch.  This fixes the facility on HEAD, while
> minimizing the diffs between the dumps.  We could do more, like a
> s/PROCEDURE/FUNCTION/ but that does not make the diffs really
> unreadable either.  I have only tested that on HEAD as new version
> down to 11 as the oldest version per the business with --wal-segsize.
> This still needs tests with 12~ as new version though, which is boring
> but not complicated at all :)

Okay, tested and done as of fa66b6d.
--
Michael


signature.asc
Description: PGP signature

Feature Request: Allow additional special characters at the beginning of the name.

[Mark Simon]

I am very new to this list, so I don’t know whether this is the right place.

Microsoft SQL and MySQL both use the @ sign at the beginning of their 
variables. The most obvious benefit of this is that it is very easy to 
distinguish between variable names and column names.


I’m not asking for a change in how PostgreSQL manages variables, but 
whether it’s possible to allow the @ sign, and possibly the $ sign to 
start a variable name. I am aware that the _ can start a variable name, 
but the other characters are a little more eye-catching.


Does that make sense?


--


 Mark Simon

Manngo Net Pty Ltd

mobile:0411 246 672

email:m...@manngo.net 
web:http://www.manngo.net

Resume:http://mark.manngo.net

Re: Skipping logical replication transactions on subscriber side

[Greg Nancarrow]

On Tue, Oct 12, 2021 at 4:00 PM Masahiko Sawada  wrote:
>
> I've attached updated patches.
>

Some comments for the v16-0001 patch:


src/backend/postmaster/pgstat.c

(1) pgstat_vacuum_subworker_stat()

Remove "the" from beginning of the following comment line:

+ * the all the dead subscription worker statistics.


(2) pgstat_reset_subscription_error_stats()

This function would be better named "pgstat_reset_subscription_subworker_error".


(3) pgstat_report_subworker_purge()

Improve comment:

BEFORE:
+ * Tell the collector about dead subscriptions.
AFTER:
+ * Tell the collector to remove dead subscriptions.


(4) pgstat_get_subworker_entry()

I notice that in the following code:

+ if (create && !found)
+ pgstat_reset_subworker_error(wentry, 0);

The newly-created PgStat_StatSubWorkerEntry doesn't get the "dbid"
member set, so I think it's a junk value in this case, yet the caller
of pgstat_get_subworker_entry(..., true) is referencing it:

+ /* Get the subscription worker stats */
+ wentry = pgstat_get_subworker_entry(msg->m_subid, msg->m_subrelid, true);
+ Assert(wentry);
+
+ /*
+ * Update only the counter and timestamp if we received the same error
+ * again
+ */
+ if (wentry->dbid == msg->m_dbid &&
+ wentry->relid == msg->m_relid &&
+ wentry->command == msg->m_command &&
+ wentry->xid == msg->m_xid &&
+ strcmp(wentry->message, msg->m_message) == 0)
+ {
+ wentry->count++;
+ wentry->timestamp = msg->m_timestamp;
+ return;
+ }

Maybe the cheapest solution is to just set dbid in
pgstat_reset_subworker_error()?


src/backend/replication/logical/worker.c

(5) Fix typo

synchroniztion -> synchronization

+ * type for table synchroniztion.


Regards,
Greg Nancarrow
Fujitsu Australia

Re: should we allow users with a predefined role to access pg_backend_memory_contexts view and pg_log_backend_memory_contexts function?gr

[Bossart, Nathan]

On 10/12/21, 6:26 PM, "Michael Paquier"  wrote:
> On Tue, Oct 12, 2021 at 08:33:19PM -0400, Stephen Frost wrote:
>> I would think we would do both…. That is- move to using GRANT/REVOKE, and
>> then just include a GRANT to pg_read_all_stats.
>> 
>> Or not. I can see the argument that, because it just goes into the log,
>> that it doesn’t make sense to grant to a predefined role, since that role
>> wouldn’t be able to see the results even if it had access.
>
> I don't think that this is a bad thing to remove the superuser() check
> and replace it with a REVOKE FROM PUBLIC in this case, but linking the
> logging of memory contexts with pg_read_all_stats does not seem right
> to me.

+1

Nathan

Re: BUG #17220: ALTER INDEX ALTER COLUMN SET (..) with an optionless opclass makes index and table unusable

[Bossart, Nathan]

On 10/12/21, 5:31 PM, "Vik Fearing"  wrote:
> On 10/13/21 2:06 AM, Bossart, Nathan wrote:
>> Moving to pgsql-hackers@.
>>
>> At first glance, it looks like ALTER INDEX .. ALTER COLUMN ... SET
>> uses the wrong validation function.  I've attached a patch where I've
>> attempted to fix that and added some tests.
>
> Ah, thank you.  I was in the (slow) process of writing basically this
> exact patch.  So I'll stop now and endorse yours.

Oops, sorry about that.  Thanks for the endorsement.

Nathan

RE: Added schema level support for publication.

[houzj.f...@fujitsu.com]

On Tuesday, October 12, 2021 9:15 PM vignesh C 
> Attached v40 patch has the fix for the above comments.

Thanks for the update, I have some minor issues about partition related 
behavior.

1)

Tang tested and discussed this issue with me.
The testcase is:
We publish a schema and there is a partition in the published schema. If
publish_via_partition_root is on and the partition's parent table is not in the
published schema, neither the change on the partition nor the parent table will
not be published.

But if we publish by FOR TABLE partition and set publish_via_partition_root to
on, the change on the partition will be published. So, I think it'd be better to
publish the change on partition for FOR ALL TABLES IN SCHEMA case if its parent 
table
is not published in the same publication.

It seems we should pass publication oid to the GetSchemaPublicationRelations()
and add some check like the following:

if (is_publishable_class(relid, relForm) &&
!(relForm->relispartition && pub_partopt == 
PUBLICATION_PART_ROOT))
result = lappend_oid(result, relid);
+   if (relForm->relispartition && pub_partopt == 
PUBLICATION_PART_ROOT)
+   {
+   bool skip = false;
+   List *ancestors = get_partition_ancestors(relid);
+   List *schemas = GetPublicationSchemas(pubid);
+   ListCell   *lc;
+   foreach(lc, ancestors)
+   {
+   if (list_member_oid(schemas, 
get_rel_namespace(lfirst_oid(lc
+   {
+   skip = true;
+   break;
+   }
+   }
+   if (!skip)
+   result = lappend_oid(result, relid);
+   }



2)
+   /*
+* It is quite possible that some of the partitions are in a different
+* schema than the parent table, so we need to get such partitions
+* separately.
+*/
+   scan = table_beginscan_catalog(classRel, keycount, key);
+   while ((tuple = heap_getnext(scan, ForwardScanDirection)) != NULL)
+   {
+   Form_pg_class relForm = (Form_pg_class) GETSTRUCT(tuple);
+
+   if (is_publishable_class(relForm->oid, relForm))
+   {
+   List   *partitionrels = NIL;
+
+   partitionrels = 
GetPubPartitionOptionRelations(partitionrels,
+   
   pub_partopt,
+   
   relForm->oid);

I think a partitioned table could also be a partition which should not be
appended to the list. I think we should also filter these cases here by same
check in 1).
Thoughts ?

Best regards,
Hou zj

Re: Feature Request: Allow additional special characters at the beginning of the name.

[Tom Lane]

Mark Simon  writes:
> I’m not asking for a change in how PostgreSQL manages variables, but 
> whether it’s possible to allow the @ sign, and possibly the $ sign to 
> start a variable name.

@ is allowed in operator names, and indeed is used in (mumble select
count(*) ...) 59 built-in operators.  So we could not support that
without breaking a lot of applications.  Is "a<@b" to be parsed as
"a <@ b" or "a < @b"?  For that matter, is "@a" a name or an invocation
of the built-in prefix operator "@" on variable "a"?

As for allowing $ to start a name, there are also issues:

* It'd be rather ambiguous with the $id$ string delimiter syntax [1],
which is a Postgres-ism for sure, but a lot of people use it.

* It'd not be entirely clear whether $1 is a variable name
or a parameter reference.

* I think there are client interfaces that allow $name to be
a parameter symbol, so we'd also be breaking anything that
works that way.

Maybe we could have done this twenty years ago, but I think
compatibility considerations preclude it now.

regards, tom lane

[1] 
https://www.postgresql.org/docs/current/sql-syntax-lexical.html#SQL-SYNTAX-DOLLAR-QUOTING

Re: [BUG] Unexpected action when publishing partition tables

[Amit Langote]

Hi Amit,

On Fri, Oct 8, 2021 at 12:47 PM Amit Kapila  wrote:
> On Thu, Oct 7, 2021 at 12:39 PM Amit Langote  wrote:
> > Sorry that I didn't comment on this earlier, but I think either
> > GetPubPartitionOptionRelations() or InvalidatePublicationRels()
> > introduced in the commit 4548c76738b should lock the partitions, just
> > like to the parent partitioned table would be, before invalidating
> > them.  There may be some hazards to invalidating a relation without
> > locking it.
>
> I see your point but then on the same lines didn't the existing code
> "for all tables" case (where we call CacheInvalidateRelcacheAll()
> without locking all relations) have a similar problem.

There might be.  I checked to see how other callers/modules use
CacheInvalidateRelcacheAll(), though it seems that only the functions
in publicationcmds.c use it or really was invented in 665d1fad99e for
use by publication commands.

Maybe I need to look harder than I've done for any examples of hazard.

> Also, in your
> patch, you are assuming that the callers of GetPublicationRelations()
> will lock all the relations but what when it gets called from
> AlterPublicationOptions()?

Ah, my bad.  I hadn't noticed that one for some reason.

Now that you mention it, I do find it somewhat concerning (on the
similar grounds as what prompted my previous email) that
AlterPublicationOptions() does away with any locking on the affected
relations.

Anyway, I'll think a bit more about the possible hazards of not doing
the locking and will reply again if there's indeed a problem(s) that
needs to be fixed.

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

Re: BUG #17220: ALTER INDEX ALTER COLUMN SET (..) with an optionless opclass makes index and table unusable

[Michael Paquier]

On Wed, Oct 13, 2021 at 12:06:32AM +, Bossart, Nathan wrote:
> At first glance, it looks like ALTER INDEX .. ALTER COLUMN ... SET
> uses the wrong validation function.  I've attached a patch where I've
> attempted to fix that and added some tests.

The gap is larger than than, because ALTER INDEX .. ALTER COLUMN
.. SET is supported by the parser but we don't document it.  The only
thing we document now is SET STATISTICS that applies to a column
*number*.

Anyway, specifying a column name for an ALTER INDEX is not right, no?
Just take for example the case of an expression which has a hardcoded
column name in pg_attribute.  So these are not specific to indexes,
which is why we apply column numbers for the statistics case.  I think
that we'd better just reject those cases until there is a proper
design done here.  As far as I can see, I guess that we should do
things  similarly to what we do for SET STATISTICS with column
numbers when it comes to indexes.
--
Michael


signature.asc
Description: PGP signature

Re: Bug in DefineRange() with multiranges

[Michael Paquier]

On Tue, Oct 12, 2021 at 08:52:29AM +0300, Sergey Shinderuk wrote:
> Thanks, here is a patch.

Looks fine seen from here, so I'll apply shortly.  I was initially
tempted to do pstrdup() on the object name returned by
QualifiedNameGetCreationNamespace(), but just removing the pfree() is
simpler.   

I got to wonder about similar mistakes from the other callers of
QualifiedNameGetCreationNamespace(), so I have double-checked but
nothing looks wrong.
--
Michael


signature.asc
Description: PGP signature

Re: Reset snapshot export state on the transaction abort

[Michael Paquier]

On Mon, Oct 11, 2021 at 08:46:32PM +0530, Dilip Kumar wrote:
> As reported at [1], if the transaction is aborted during export
> snapshot then ExportInProgress and SavedResourceOwnerDuringExport are
> not getting reset and that is throwing an error
> "clearing exported snapshot in wrong transaction state" while
> executing the next command.  The attached patch clears this state if
> the transaction is aborted.

Injecting an error is enough to reproduce the failure in a second
command after the first one failed.  This could happen on OOM for the
palloc() done at the beginning of SnapBuildInitialSnapshot().

@@ -2698,6 +2698,9 @@ AbortTransaction(void)
/* Reset logical streaming state. */
ResetLogicalStreamingState();

+   /* Reset snapshot export state. */
+   ResetSnapBuildExportSnapshotState();
Shouldn't we care about the case of a sub-transaction abort as well?
See AbortSubTransaction().
--
Michael


signature.asc
Description: PGP signature

lastOverflowedXid does not handle transaction ID wraparound

[Stan Hu]

In a blog post 
(https://about.gitlab.com/blog/2021/09/29/why-we-spent-the-last-month-eliminating-postgresql-subtransactions/),
I described how PostgreSQL can enter into a suboverflow condition on
the replica under a number of conditions:

1. A long transaction starts.
2. A single SAVEPOINT is issued.
3. Many rows are updated on the primary, and the same rows are read
from the replica.

This can cause a significant performance degradation with a replica
due to SubtransSLRU wait events since the replica needs to perform a
parent lookup on an ever-growing range of XIDs. Full details on how to
replicate this: https://gitlab.com/-/snippets/2187338.

The main two lines of code that cause the replica to enter in the
suboverflowed state are here
(https://github.com/postgres/postgres/blob/317632f3073fc06047a42075eb5e28a9577a4f96/src/backend/storage/ipc/procarray.c#L2431-L2432):

if (TransactionIdPrecedesOrEquals(xmin, procArray->lastOverflowedXid))
   suboverflowed = true;

I noticed that lastOverflowedXid doesn't get cleared even after all
subtransactions have been completed. On a replica, it only seems to be
updated via a XLOG_XACT_ASSIGNMENT, but no such message will be sent
if subtransactions halt. If the XID wraps around again and a long
transaction starts before lastOverflowedXid, the replica might
unnecessarily enter in the suboverflow condition again.

I've validated this by issuing a SAVEPOINT, running the read/write
test, logging lastOverflowedXid to stderr, and then using pg_bench to
advance XID with SELECT txid_current(). After many hours, I validated
that lastOverflowedXid remained the same, and I could induce a high
degree of SubtransSLRU wait events without issuing a new SAVEPOINT.

I'm wondering a few things:

1. Should lastOverflowedXid be reset to 0 at some point? I'm not sure
if there's a good way at the moment for the replica to know that all
subtransactions have completed.
2. Alternatively, should the epoch number be used to compare xmin and
lastOverflowedXid?

To mitigate this issue, we've considered:

1. Restarting the replicas. This isn't great, and if another SAVEPOINT
comes along, we'd have to do this again. It would be nice to be able
to monitor the exact value of lastOverflowedXid.
2. Raise the NUM_SUBTRANS_BUFFERS as a workaround until the scalable
SLRU patches are available
(https://commitfest.postgresql.org/34/2627/).
3. Issue SAVEPOINTs periodically to "run away" from this wraparound issue.

Re: Improve the HINT message of the ALTER command for postgres_fdw

[Bharath Rupireddy]

On Tue, Oct 12, 2021 at 11:11 PM Fujii Masao
 wrote:
> BTW, I found file_fdw.c, dblink.c, postgres_fdw/option.c and foreign.c
> use different error codes for the same error message as follows.
> They should use the same error code? If yes, ISTM that
> ERRCODE_FDW_INVALID_OPTION_NAME is better because
> the error message is "invalid option ...".
>
> - ERRCODE_FDW_INVALID_OPTION_NAME (file_fdw.c)
> - ERRCODE_FDW_OPTION_NAME_NOT_FOUND (dblink.c)
> - ERRCODE_FDW_INVALID_OPTION_NAME (postgres_fdw/option.c)
> - ERRCODE_SYNTAX_ERROR (foreign.c)

Good catch. ERRCODE_FDW_INVALID_OPTION_NAME seems reasonable to me. I
think we can remove the error code ERRCODE_FDW_OPTION_NAME_NOT_FOUND
(it is being used only by dblink.c), instead use
ERRCODE_FDW_INVALID_OPTION_NAME for all option parsing and
validations.

Regards,
Bharath Rupireddy.

Re: Reset snapshot export state on the transaction abort

[Dilip Kumar]

On Wed, Oct 13, 2021 at 10:17 AM Michael Paquier  wrote:
>
> On Mon, Oct 11, 2021 at 08:46:32PM +0530, Dilip Kumar wrote:
> > As reported at [1], if the transaction is aborted during export
> > snapshot then ExportInProgress and SavedResourceOwnerDuringExport are
> > not getting reset and that is throwing an error
> > "clearing exported snapshot in wrong transaction state" while
> > executing the next command.  The attached patch clears this state if
> > the transaction is aborted.

Correct.

> Injecting an error is enough to reproduce the failure in a second
> command after the first one failed.  This could happen on OOM for the
> palloc() done at the beginning of SnapBuildInitialSnapshot().
>
> @@ -2698,6 +2698,9 @@ AbortTransaction(void)
> /* Reset logical streaming state. */
> ResetLogicalStreamingState();
>
> +   /* Reset snapshot export state. */
> +   ResetSnapBuildExportSnapshotState();
> Shouldn't we care about the case of a sub-transaction abort as well?
> See AbortSubTransaction().


Actually, it is not required because 1) Snapshot export can not be
allowed within a transaction block, basically, it starts its own
transaction block and aborts that while executing any next replication
command see SnapBuildClearExportedSnapshot().  So our problem is only
if the transaction block internally started for exporting, gets
aborted before any next command arrives.  So there is no possibility
of starting any sub transaction.

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

Re: should we allow users with a predefined role to access pg_backend_memory_contexts view and pg_log_backend_memory_contexts function?gr

[Bharath Rupireddy]

On Wed, Oct 13, 2021 at 6:55 AM Michael Paquier  wrote:
>
> On Tue, Oct 12, 2021 at 08:33:19PM -0400, Stephen Frost wrote:
> > I would think we would do both…. That is- move to using GRANT/REVOKE, and
> > then just include a GRANT to pg_read_all_stats.
> >
> > Or not. I can see the argument that, because it just goes into the log,
> > that it doesn’t make sense to grant to a predefined role, since that role
> > wouldn’t be able to see the results even if it had access.
>
> I don't think that this is a bad thing to remove the superuser() check
> and replace it with a REVOKE FROM PUBLIC in this case,

IMO, we can just retain the "if (!superuser())" check in the
pg_log_backend_memory_contexts as is. This would be more meaningful as
the error "must be superuser to use raw page functions" explicitly
says that a superuser is allowed. Whereas if we revoke the permissions
in system_views.sql, then the error we get is not meaningful as the
error "permission denied for function pg_log_backend_memory_contexts"
says that permissions denied and the user will have to look at the
documentation for what permissions this function requires.

And, I see there are a lot of functions in the code base that does "if
(!superuser())" check and emit "must be superuser to XXX" sort of
error.

> but linking the
> logging of memory contexts with pg_read_all_stats does not seem right
> to me.

Agreed. The user with pg_read_all_stats can't see the server logs so
it doesn't make sense to make them call the function.  I will remove
this change from the patch.

Regards,
Bharath Rupireddy.

Re: prevent immature WAL streaming

[Amul Sul]

Hi,

On Thu, Oct 7, 2021 at 6:57 PM Alvaro Herrera  wrote:
>
> On 2021-Oct-07, Amul Sul wrote:
>
> > Make sense, thanks for the explanation.
>
> You're welcome.  Also, I forgot: thank you for taking the time to review
> the code.  Much appreciated.

:)

>
>

I have one more question, regarding the need for other global
variables i.e. abortedRecPtr.  (Sorry for coming back after so long.)

Instead of abortedRecPtr point, isn't enough to write
overwrite-contrecord at XLogCtl->lastReplayedEndRecPtr?  I think both
are pointing to the same location then can't we use
lastReplayedEndRecPtr instead of abortedRecPtr to write
overwrite-contrecord and remove need of extra global variable, like
attached?

You might wonder why I am so concerned about the global variable. The
reason is that I am working on another thread[1] where we are trying
to club all the WAL write operations that happen at the end of
StartupXLOG into a separate function. In the future, we might want to
allow executing this function from other processes (e.g.
Checkpointer). For that, we need to remove the dependency of those WAL
write operations having on the global variables which are mostly valid
in the startup process. The easiest way to do that is simply copy all
the global variables into shared memory but that will not be an
optimised solution, the goal is to try to see if we could leverage the existing
information available into shared memory. I would be grateful if you
could share your thoughts on the same, thanks.

Regards,
Amul

1] 
https://postgr.es/m/caaj_b97kzzdjsffwrk7w0xu5hnxkcgkgtr69t8cozztsyxj...@mail.gmail.com
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 26dcc00ac01..52b93cd80cc 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -200,12 +200,11 @@ static XLogRecPtr flushedUpto = 0;
 static TimeLineID receiveTLI = 0;
 
 /*
- * abortedRecPtr is the start pointer of a broken record at end of WAL when
- * recovery completes; missingContrecPtr is the location of the first
- * contrecord that went missing.  See CreateOverwriteContrecordRecord for
- * details.
+ * missingContrecPtr is the location of the first contrecord that went missing.
+ * XLogCtl->lastReplayedEndRecPtr will be the start pointer of a broken record
+ * at end of WAL when recovery completes. See CreateOverwriteContrecordRecord
+ * for details.
  */
-static XLogRecPtr abortedRecPtr;
 static XLogRecPtr missingContrecPtr;
 
 /*
@@ -4425,11 +4424,8 @@ ReadRecord(XLogReaderState *xlogreader, int emode,
 			 * that portion is to be ignored.
 			 */
 			if (!StandbyMode &&
-!XLogRecPtrIsInvalid(xlogreader->abortedRecPtr))
-			{
-abortedRecPtr = xlogreader->abortedRecPtr;
+!XLogRecPtrIsInvalid(xlogreader->missingContrecPtr))
 missingContrecPtr = xlogreader->missingContrecPtr;
-			}
 
 			if (readFile >= 0)
 			{
@@ -7109,7 +7105,6 @@ StartupXLOG(void)
 	/*
 	 * Start recovery assuming that the final record isn't lost.
 	 */
-	abortedRecPtr = InvalidXLogRecPtr;
 	missingContrecPtr = InvalidXLogRecPtr;
 
 	/* REDO */
@@ -7876,10 +7871,7 @@ StartupXLOG(void)
 	 * we'll do as soon as we're open for writing new WAL.)
 	 */
 	if (!XLogRecPtrIsInvalid(missingContrecPtr))
-	{
-		Assert(!XLogRecPtrIsInvalid(abortedRecPtr));
 		EndOfLog = missingContrecPtr;
-	}
 
 	/*
 	 * Prepare to write WAL starting at EndOfLog location, and init xlog
@@ -7936,11 +7928,10 @@ StartupXLOG(void)
 	LocalSetXLogInsertAllowed();
 
 	/* If necessary, write overwrite-contrecord before doing anything else */
-	if (!XLogRecPtrIsInvalid(abortedRecPtr))
+	if (!XLogRecPtrIsInvalid(missingContrecPtr))
 	{
-		Assert(!XLogRecPtrIsInvalid(missingContrecPtr));
-		CreateOverwriteContrecordRecord(abortedRecPtr);
-		abortedRecPtr = InvalidXLogRecPtr;
+		Assert(!XLogRecPtrIsInvalid(XLogCtl->lastReplayedEndRecPtr));
+		CreateOverwriteContrecordRecord(XLogCtl->lastReplayedEndRecPtr);
 		missingContrecPtr = InvalidXLogRecPtr;
 	}
 
diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
index 4b03577dccd..516124ecd5f 100644
--- a/src/backend/access/transam/xlogreader.c
+++ b/src/backend/access/transam/xlogreader.c
@@ -294,7 +294,6 @@ XLogReadRecord(XLogReaderState *state, char **errormsg)
 	state->errormsg_buf[0] = '\0';
 
 	ResetDecoder(state);
-	state->abortedRecPtr = InvalidXLogRecPtr;
 	state->missingContrecPtr = InvalidXLogRecPtr;
 
 	RecPtr = state->EndRecPtr;
@@ -586,7 +585,6 @@ err:
 		 * in turn signal downstream WAL consumers that the broken WAL record
 		 * is to be ignored.
 		 */
-		state->abortedRecPtr = RecPtr;
 		state->missingContrecPtr = targetPagePtr;
 	}
 
diff --git a/src/include/access/xlogreader.h b/src/include/access/xlogreader.h
index de6fd791fe6..d876e25de7f 100644
--- a/src/include/access/xlogreader.h
+++ b/src/include/access/xlogreader.h
@@ -176,11 +176,9 @@ struct XLogReaderState
 	XLogRecPtr	EndRecPtr;		/* end+1 of last record read */
 
 	/*
-	 * Set at the end