Re: patch: reduce overhead of execution of CALL statement in no atomic mode from PL/pgSQL

[Pavel Stehule]

Hi

only rebase

Regards

Pavel
diff --git a/src/backend/commands/prepare.c b/src/backend/commands/prepare.c
index 89087a7be3..b1d2f1af37 100644
--- a/src/backend/commands/prepare.c
+++ b/src/backend/commands/prepare.c
@@ -230,7 +230,7 @@ ExecuteQuery(ParseState *pstate,
 	   entry->plansource->query_string);
 
 	/* Replan if needed, and increment plan refcount for portal */
-	cplan = GetCachedPlan(entry->plansource, paramLI, false, NULL);
+	cplan = GetCachedPlan(entry->plansource, paramLI, false, NULL, NULL);
 	plan_list = cplan->stmt_list;
 
 	/*
@@ -651,7 +651,7 @@ ExplainExecuteQuery(ExecuteStmt *execstmt, IntoClause *into, ExplainState *es,
 	}
 
 	/* Replan if needed, and acquire a transient refcount */
-	cplan = GetCachedPlan(entry->plansource, paramLI, true, queryEnv);
+	cplan = GetCachedPlan(entry->plansource, paramLI, true, CurrentResourceOwner, queryEnv);
 
 	INSTR_TIME_SET_CURRENT(planduration);
 	INSTR_TIME_SUBTRACT(planduration, planstart);
@@ -687,7 +687,7 @@ ExplainExecuteQuery(ExecuteStmt *execstmt, IntoClause *into, ExplainState *es,
 	if (estate)
 		FreeExecutorState(estate);
 
-	ReleaseCachedPlan(cplan, true);
+	ReleaseCachedPlan(cplan, true, CurrentResourceOwner);
 }
 
 /*
diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index 055ebb77ae..39c19f3b40 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -61,7 +61,7 @@ static void _SPI_prepare_oneshot_plan(const char *src, SPIPlanPtr plan);
 static int	_SPI_execute_plan(SPIPlanPtr plan, ParamListInfo paramLI,
 			  Snapshot snapshot, Snapshot crosscheck_snapshot,
 			  bool read_only, bool fire_triggers, uint64 tcount,
-			  DestReceiver *caller_dest);
+			  DestReceiver *caller_dest, ResourceOwner owner);
 
 static ParamListInfo _SPI_convert_params(int nargs, Oid *argtypes,
 		 Datum *Values, const char *Nulls);
@@ -514,7 +514,8 @@ SPI_execute(const char *src, bool read_only, long tcount)
 
 	res = _SPI_execute_plan(&plan, NULL,
 			InvalidSnapshot, InvalidSnapshot,
-			read_only, true, tcount, NULL);
+			read_only, true, tcount, NULL,
+			CurrentResourceOwner);
 
 	_SPI_end_call(true);
 	return res;
@@ -548,7 +549,8 @@ SPI_execute_plan(SPIPlanPtr plan, Datum *Values, const char *Nulls,
 			_SPI_convert_params(plan->nargs, plan->argtypes,
 Values, Nulls),
 			InvalidSnapshot, InvalidSnapshot,
-			read_only, true, tcount, NULL);
+			read_only, true, tcount, NULL,
+			CurrentResourceOwner);
 
 	_SPI_end_call(true);
 	return res;
@@ -577,7 +579,8 @@ SPI_execute_plan_with_paramlist(SPIPlanPtr plan, ParamListInfo params,
 
 	res = _SPI_execute_plan(plan, params,
 			InvalidSnapshot, InvalidSnapshot,
-			read_only, true, tcount, NULL);
+			read_only, true, tcount, NULL,
+			CurrentResourceOwner);
 
 	_SPI_end_call(true);
 	return res;
@@ -606,7 +609,37 @@ SPI_execute_plan_with_receiver(SPIPlanPtr plan,
 
 	res = _SPI_execute_plan(plan, params,
 			InvalidSnapshot, InvalidSnapshot,
-			read_only, true, tcount, dest);
+			read_only, true, tcount, dest,
+			CurrentResourceOwner);
+
+	_SPI_end_call(true);
+	return res;
+}
+
+
+/*
+ * Execute a previously prepared plan with possibility to
+ * specify resource owner
+ */
+int
+SPI_execute_plan_with_resowner(SPIPlanPtr plan,
+ParamListInfo params,
+bool read_only, long tcount,
+ResourceOwner owner)
+{
+	int			res;
+
+	if (plan == NULL || plan->magic != _SPI_PLAN_MAGIC || tcount < 0)
+		return SPI_ERROR_ARGUMENT;
+
+	res = _SPI_begin_call(true);
+	if (res < 0)
+		return res;
+
+	res = _SPI_execute_plan(plan, params,
+			InvalidSnapshot, InvalidSnapshot,
+			read_only, true, tcount, NULL,
+			owner);
 
 	_SPI_end_call(true);
 	return res;
@@ -647,7 +680,8 @@ SPI_execute_snapshot(SPIPlanPtr plan,
 			_SPI_convert_params(plan->nargs, plan->argtypes,
 Values, Nulls),
 			snapshot, crosscheck_snapshot,
-			read_only, fire_triggers, tcount, NULL);
+			read_only, fire_triggers, tcount, NULL,
+			CurrentResourceOwner);
 
 	_SPI_end_call(true);
 	return res;
@@ -694,7 +728,8 @@ SPI_execute_with_args(const char *src,
 
 	res = _SPI_execute_plan(&plan, paramLI,
 			InvalidSnapshot, InvalidSnapshot,
-			read_only, true, tcount, NULL);
+			read_only, true, tcount, NULL,
+			CurrentResourceOwner);
 
 	_SPI_end_call(true);
 	return res;
@@ -737,7 +772,8 @@ SPI_execute_with_receiver(const char *src,
 
 	res = _SPI_execute_plan(&plan, params,
 			InvalidSnapshot, InvalidSnapshot,
-			read_only, true, tcount, dest);
+			read_only, true, tcount, dest,
+			CurrentResourceOwner);
 
 	_SPI_end_call(true);
 	return res;
@@ -1502,7 +1538,7 @@ SPI_cursor_open_internal(const char *name, SPIPlanPtr plan,
 	 */
 
 	/* Replan if needed, and increment plan refcount for portal */
-	cplan = GetCachedPlan(plansource, paramLI, false, _SPI_current->queryEnv);
+	cplan = GetCachedPlan(pl

Re: Asynchronous Append on postgres_fdw nodes.

[Etsuro Fujita]

On Thu, Dec 31, 2020 at 7:15 PM Etsuro Fujita  wrote:
> * I tweaked comments a bit to address your comments.

I forgot to update some comments.  :-(  Attached is a new version of
the patch updating comments further.  I did a bit of cleanup for the
postgres_fdw part as well.

Best regards,
Etsuro Fujita


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

Re: proposal: schema variables

[Pavel Stehule]

Hi

fresh rebase

Regards

Pavel


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

Re: [PATCH] postgres_fdw connection caching - cause remote sessions linger till the local session exit

[Bharath Rupireddy]

On Thu, Dec 31, 2020 at 8:29 AM Bharath Rupireddy
 wrote:
> Right. I meant the "next use" as the select attempt on a foreign table
> with that foreign server. If no select query is run, then at the end
> of the current txn that connection gets closed. Yes internally such
> connection gets closed in pgfdw_xact_callback.
>
> If the errdetail("Such connections get closed either in the next use
> or at the end of the current transaction.") looks confusing, how about
>
> 1) errdetail("Such connection gets discarded while closing the remote
> transaction.")/errdetail("Such connections get discarded while closing
> the remote transaction.")
> 2) errdetail("Such connection is discarded at the end of remote
> transaction.")/errdetail("Such connections are discarded at the end of
> remote transaction.")
>
> I prefer 2)  Thoughts?
>
> Because we already print a message in pgfdw_xact_callback -
> elog(DEBUG3, "closing remote transaction on connection %p"

I changed the message to "Such connection is discarded at the end of
remote transaction.".

I'm attaching v5 patch set i.e. all the patches 0001 ( for new
functions), 0002 ( for GUC) and 0003 (for server level option). I have
also made the changes for increasing the version of
postgres_fdw--1.0.sql from 1.0 to 1.1.

I have no open points from my end. Please consider the v5 patch set
for further review.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
From 413a8b34f8e1e1faf03aecad926a4b2a7a724aaa Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy 
Date: Fri, 1 Jan 2021 14:09:54 +0530
Subject: [PATCH v5 1/3] postgres_fdw function to discard cached connections

This patch introduces new function postgres_fdw_disconnect() when
called with a foreign server name discards the associated
connections with the server name. When called without any argument,
discards all the existing cached connections.

This patch also adds another function postgres_fdw_get_connections()
to get the list of all cached connections by corresponding foreign
server names.
---
 contrib/postgres_fdw/Makefile |   2 +-
 contrib/postgres_fdw/connection.c | 319 +-
 .../postgres_fdw/expected/postgres_fdw.out| 415 +-
 ...dw--1.0.sql => postgres_fdw--1.0--1.1.sql} |   6 +-
 contrib/postgres_fdw/postgres_fdw--1.1.sql|  33 ++
 contrib/postgres_fdw/postgres_fdw.control |   2 +-
 contrib/postgres_fdw/sql/postgres_fdw.sql | 166 +++
 doc/src/sgml/postgres-fdw.sgml|  57 ++-
 8 files changed, 976 insertions(+), 24 deletions(-)
 rename contrib/postgres_fdw/{postgres_fdw--1.0.sql => postgres_fdw--1.0--1.1.sql} (60%)
 create mode 100644 contrib/postgres_fdw/postgres_fdw--1.1.sql

diff --git a/contrib/postgres_fdw/Makefile b/contrib/postgres_fdw/Makefile
index ee8a80a392..52d3dac0bd 100644
--- a/contrib/postgres_fdw/Makefile
+++ b/contrib/postgres_fdw/Makefile
@@ -14,7 +14,7 @@ PG_CPPFLAGS = -I$(libpq_srcdir)
 SHLIB_LINK_INTERNAL = $(libpq)
 
 EXTENSION = postgres_fdw
-DATA = postgres_fdw--1.0.sql
+DATA = postgres_fdw--1.1.sql postgres_fdw--1.0--1.1.sql
 
 REGRESS = postgres_fdw
 
diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c
index d841cec39b..b9f6050f4b 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -22,6 +22,7 @@
 #include "postgres_fdw.h"
 #include "storage/fd.h"
 #include "storage/latch.h"
+#include "utils/builtins.h"
 #include "utils/datetime.h"
 #include "utils/hsearch.h"
 #include "utils/inval.h"
@@ -57,6 +58,8 @@ typedef struct ConnCacheEntry
 	bool		have_error;		/* have any subxacts aborted in this xact? */
 	bool		changing_xact_state;	/* xact state change in process */
 	bool		invalidated;	/* true if reconnect is pending */
+	/* Server oid to get the associated foreign server name. */
+	Oid			serverid;
 	uint32		server_hashvalue;	/* hash value of foreign server OID */
 	uint32		mapping_hashvalue;	/* hash value of user mapping OID */
 } ConnCacheEntry;
@@ -73,6 +76,12 @@ static unsigned int prep_stmt_number = 0;
 /* tracks whether any work is needed in callback functions */
 static bool xact_got_connection = false;
 
+/*
+ * SQL functions
+ */
+PG_FUNCTION_INFO_V1(postgres_fdw_get_connections);
+PG_FUNCTION_INFO_V1(postgres_fdw_disconnect);
+
 /* prototypes of private functions */
 static void make_new_connection(ConnCacheEntry *entry, UserMapping *user);
 static PGconn *connect_pg_server(ForeignServer *server, UserMapping *user);
@@ -94,6 +103,8 @@ static bool pgfdw_exec_cleanup_query(PGconn *conn, const char *query,
 static bool pgfdw_get_cleanup_result(PGconn *conn, TimestampTz endtime,
 	 PGresult **result);
 static bool UserMappingPasswordRequired(UserMapping *user);
+static bool disconnect_cached_connections(uint32 hashvalue, bool all,
+		  bool *is_in_use);
 
 /*
  * Get a PGconn which can be used to execute queries on the remote PostgreSQL
@@ -273,6 +284,7 @@ make_new_connection(ConnCacheEntry 

Re: [PATCH] Simple progress reporting for COPY command

[Bharath Rupireddy]

On Fri, Jan 1, 2021 at 6:55 AM Josef Šimánek  wrote:
> finally I had some time to revisit patch and all comments from
> https://www.postgresql.org/message-id/CAFp7QwqMGEi4OyyaLEK9DR0%2BE%2BoK3UtA4bEjDVCa4bNkwUY2PQ%40mail.gmail.com
> and I have prepared simple version of COPY command progress reporting.

Thanks for the patch.

> To keep the patch small as possible, I have introduced only a minimum
> set of columns. It could be extended later if needed.
>
> Columns are inspired by CREATE INDEX progress report system view.
>
> pid - integer - PID of backend
> datid - oid - OID of related database
> datname - name - name of related database (this seems redundant, since
> oid should be enough, but it is the same in CREATE INDEX)
> relid - oid - oid of table related to COPY command, when not known
> (for example when copying to file, it is 0)
> bytes_processed - bigint - amount of bytes processed
> bytes_total - bigint - file size in bytes if COPY FROM file (0 if not
> COPY FROM file)

It means that, for COPY FROM STDIN, bytes_total will be 0. IIUC, so
having this parameter as 0 is an indication of the COPY command being
from STDIN? I'm not sure whether it's discussed or not previously, but
I personally prefer to have it as a separate column, say a varchar or
text column with values STDIN, FILE or PROGRAM may be. And also it
will be better if we have the format of the data streaming in, as a
separate column, with possible values may be CSV, TEXT, BINARY.

Since this progress reporting is for both COPY FROM and COPY TO,
wouldn't it be good to have that also as a separate column, say
command type with values FROM and TO?

Thoughts?

I'm sure some of the points would have already been discussed. I just
shared my thoughts here.

I have not looked into the patch in detail, but it's missing tests.
I'm sure we can not add the tests into copy.sql or copy2.sql, can we
think of adding test cases to TAP or under isolation? I'm not sure
whether other progress reporting commands have test cases.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

Re: [PATCH] Simplify permission checking logic in user.c

[Andrey Borodin]

> 31 дек. 2020 г., в 00:37, Paul Martinez  написал(а):
> 
> In Google Cloud SQL we create a role for customers, cloudsqlsuperuser,
> which, confusingly, is not a SUPERUSER, but does have some extra
> permissions. We've modified a lot of "if (!superuser())" checks to
> "if (!superuser() && !cloudsqlsuperuser())".

You use cloudsqlsuperuser.
RDS has rds_superuser.
Aiven has their aiven_extras extension.
Yandex Cloud has mdb_admin and mdb_replication.

Some of us, probably, could do something useful for the project instead of 
rebasing those patches and extensions.
Let's start to work together with community. Let's address issues that 
thread[0] faced and restart it.

Happy New Year :)

Best regards, Andrey Borodin.

[0] 
https://www.postgresql.org/message-id/flat/f41d93b6-69ba-fa05-c91a-045bafa5f832%402ndquadrant.com#636c800abc6f464c388db7f532a389ba

Re: poc - possibility to write window function in PL languages

[Pavel Stehule]

Hi

rebase

Regards

Pavel
diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
index 11246aa653..89a07678ee 100644
--- a/doc/src/sgml/plpgsql.sgml
+++ b/doc/src/sgml/plpgsql.sgml
@@ -4606,6 +4606,99 @@ CREATE EVENT TRIGGER snitch ON ddl_command_start EXECUTE FUNCTION snitch();
 
   
 
+ 
+  Window Functions
+
+  
+   window
+   in PL/pgSQL
+  
+
+  
+   PL/pgSQL can be used to define window
+   functions. A window function is created with the CREATE FUNCTION
+   command with clause WINDOW. The specific feature of
+   this functions is a possibility to two special storages with 
+   sorted values of window function arguments and store with stored
+   one value of any type for currently processed partition (of window
+   function).
+  
+
+  
+   Access to both storages is done with special internal variable
+   WINDOWOBJECT. This variable is declared implicitly,
+   and it is available only in window functions.
+
+
+CREATE OR REPLACE FUNCTION plpgsql_rownum() RETURNS int8
+LANGUAGE plpgsql WINDOW
+AS $$
+DECLARE pos int8
+BEGIN
+pos := get_current_position(WINDOWOBJECT);
+pos := pos + 1;
+PERFORM set_mark_position(WINDOWOBJECT, pos);
+RETURN pos;
+$$;
+
+SELECT plpgsql_rownum() OVER (), * FROM tab;
+
+  
+
+  
+   The arguments of window function cannot be accessed directly. The special
+   functions should be used. With these functions we can choose a scope of
+   buffered arguments, we can choose a wanted position against first, current, or
+   last row. The implementation of lag can looks like
+   (the window functions in plpgsql can use polymorphic types too):
+
+
+CREATE OR REPLACE FUNCTION plpgsql_lag(anyelement) RETURNS anyelement
+LANGUAGE plpgsql WINDOW
+AS $$
+BEGIN
+RETURN
+  get_input_value_in_partition(WINDOWOBJECT,
+   1, -1,
+   'seek_current',
+   false);
+END;
+$$;
+
+SELECT v, plpgsql_lag(v) FROM generate_series(1, 10) g(v);
+
+
+  
+
+  
+   Second buffer that can be used in window function is a buffer for one value
+   assigned to partition. The content of this buffer can be read by function
+   get_partition_context_value or modified by function
+   set_partition_context_value. Next function replaces
+   missing values by previous non NULL value:
+
+
+CREATE OR REPLACE FUNCTION plpgsql_replace_missing(numeric) RETURNS numeric
+LANGUAGE plpgsql WINDOW
+AS $$
+DECLATE
+v numeric;
+BEGIN
+v := get_input_value_for_row(WINDOWOBJECT, 1);
+IF v IS NULL THEN
+v := get_partition_context_value(WINDOWOBJECT, NULL::numeric);
+ELSE
+PERFORM set_partition_context_value(WINDOWOBJECT, v);
+END IF;
+RETURN v;
+END;
+$$;
+
+
+  
+
+  
+
   
PL/pgSQL under the Hood
 
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index b140c210bc..37d38d6e84 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -1442,6 +1442,18 @@ LANGUAGE internal
 STRICT IMMUTABLE PARALLEL SAFE
 AS 'unicode_is_normalized';
 
+CREATE OR REPLACE FUNCTION
+  get_partition_context_value(windowobjectproxy, anyelement, int4 DEFAULT NULL)
+  RETURNS anyelement
+LANGUAGE internal
+AS 'windowobject_get_partition_context_value';
+
+CREATE OR REPLACE FUNCTION
+  set_partition_context_value(windowobjectproxy, anyelement, int4 DEFAULT NULL)
+  RETURNS void
+LANGUAGE internal
+AS 'windowobject_set_partition_context_value';
+
 --
 -- The default permissions for functions mean that anyone can execute them.
 -- A number of functions shouldn't be executable by just anyone, but rather
diff --git a/src/backend/utils/adt/Makefile b/src/backend/utils/adt/Makefile
index 82732146d3..4e10e2bde7 100644
--- a/src/backend/utils/adt/Makefile
+++ b/src/backend/utils/adt/Makefile
@@ -105,6 +105,7 @@ OBJS = \
 	tsvector.o \
 	tsvector_op.o \
 	tsvector_parser.o \
+	typedvalue.o \
 	uuid.o \
 	varbit.o \
 	varchar.o \
diff --git a/src/backend/utils/adt/pseudotypes.c b/src/backend/utils/adt/pseudotypes.c
index 99a93271fe..3745cc6515 100644
--- a/src/backend/utils/adt/pseudotypes.c
+++ b/src/backend/utils/adt/pseudotypes.c
@@ -372,6 +372,17 @@ pg_node_tree_send(PG_FUNCTION_ARGS)
 PSEUDOTYPE_DUMMY_IO_FUNCS(pg_ddl_command);
 PSEUDOTYPE_DUMMY_BINARY_IO_FUNCS(pg_ddl_command);
 
+/*
+ * windowobjectproxy
+ *
+ * This type is pointer to WindowObjectProxyData. It is communication
+ * mechanism between PL environment and WinFuncArgs functions. Due
+ * performance reason I prefer using indirect result processing against
+ * using function returning polymorphic composite value. The indirect
+ * mechanism is implemented with proxy object represented by type
+ * WindowObjectProxyData.
+ */
+PSEUDOTYPE_DUMMY_IO_FUNCS(windowobjectproxy);
 
 /*
  * Dummy I/O functions for various other pseudotypes.
diff --git a/src/backend/utils/adt/typedvalue.c b/src/backend/utils/adt/typedvalue.c
new file mode 100644
index 00..370804a05a
-

Commitfest 2021-01 Now in Progress

[Masahiko Sawada]

Hi,

Happy new year to all!

The January Commitfest for PG14 is now in progress! I'm happy to
volunteer to manage it.

Current state for the Commitfest is:

Needs review: 188
Waiting on Author: 28
Ready for Committer: 22
Committed: 19
Withdrawn: 3
Total: 260

The number of patches waiting on author seems lower (as a percentage)
which I take to be a good sign. I'll be assessing the Waiting on
Author patches over from the next Monday, so if your patch is in this
state get a new version in soon.

Also, the stats of how many Commitfests live the patches are:

1-2 : 149
3-4 : 62
5-6 : 19
7-8 : 9
9-10 : 9
11-12 : 1
13-14 : 7
15-16 : 1
17-18 : 1
19-20 : 2

About 10% of patches are living for more than 10 Commitfests whereas
85% of patches are registered since when Commitfest for PG14 started.

Please, if you have submitted patches in this CF make sure that you
are also reviewing patches of a similar number and complexity. The CF
cannot move forward without patch review.

Also, check the state of your patch at http://cfbot.cputube.org/

Happy hacking!

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

Re: [PATCH] Simple progress reporting for COPY command

[Josef Šimánek]

pá 1. 1. 2021 v 11:16 odesílatel Bharath Rupireddy
 napsal:
>
> On Fri, Jan 1, 2021 at 6:55 AM Josef Šimánek  wrote:
> > finally I had some time to revisit patch and all comments from
> > https://www.postgresql.org/message-id/CAFp7QwqMGEi4OyyaLEK9DR0%2BE%2BoK3UtA4bEjDVCa4bNkwUY2PQ%40mail.gmail.com
> > and I have prepared simple version of COPY command progress reporting.
>
> Thanks for the patch.
>
> > To keep the patch small as possible, I have introduced only a minimum
> > set of columns. It could be extended later if needed.
> >
> > Columns are inspired by CREATE INDEX progress report system view.
> >
> > pid - integer - PID of backend
> > datid - oid - OID of related database
> > datname - name - name of related database (this seems redundant, since
> > oid should be enough, but it is the same in CREATE INDEX)
> > relid - oid - oid of table related to COPY command, when not known
> > (for example when copying to file, it is 0)
> > bytes_processed - bigint - amount of bytes processed
> > bytes_total - bigint - file size in bytes if COPY FROM file (0 if not
> > COPY FROM file)
>
> It means that, for COPY FROM STDIN, bytes_total will be 0. IIUC, so
> having this parameter as 0 is an indication of the COPY command being
> from STDIN? I'm not sure whether it's discussed or not previously, but
> I personally prefer to have it as a separate column, say a varchar or
> text column with values STDIN, FILE or PROGRAM may be. And also it
> will be better if we have the format of the data streaming in, as a
> separate column, with possible values may be CSV, TEXT, BINARY.
>
> Since this progress reporting is for both COPY FROM and COPY TO,
> wouldn't it be good to have that also as a separate column, say
> command type with values FROM and TO?
>
> Thoughts?

My first patch had more columns (direction - FROM/TO, file bool,
program bool), but it was subject of discussion what's the purpose.
You can check the query by looking at pg_stat_activity by pid to get
more info. To keep it simple I decided to go with a minimal set of
columns for now. It can be extended later. I'm ok to continue on
improving this with more feedback once merged.

> I'm sure some of the points would have already been discussed. I just
> shared my thoughts here.
>
> I have not looked into the patch in detail, but it's missing tests.
> I'm sure we can not add the tests into copy.sql or copy2.sql, can we
> think of adding test cases to TAP or under isolation? I'm not sure
> whether other progress reporting commands have test cases.

I have raised the question in an old thread as well since there are no
tests for other progress commands as well. I was told it is ok for now
to keep it untested, since there's no easy way to do that for now.

https://www.postgresql.org/message-id/20200615001828.GA52676%40paquier.xyz

> With Regards,
> Bharath Rupireddy.
> EnterpriseDB: http://www.enterprisedb.com

Re: Let people set host(no)ssl settings from initdb

[Magnus Hagander]

On Wed, Dec 30, 2020 at 9:00 PM Tom Lane  wrote:

> David Fetter  writes:
> > On Wed, Dec 30, 2020 at 08:24:06PM +0100, David Fetter wrote:
> >> On Mon, Sep 07, 2020 at 11:57:58AM +0900, Michael Paquier wrote:
> >>> I have looked at the patch of this thread, and I doubt that it is a
> >>> good idea to put more burden into initdb for that.  I agree that
> >>> being able to reject easily non-SSL connections in pg_hba.conf is a
> >>> bit of a hassle now, but putting more logic into initdb does not seem
> >>> the right course to me.  Perhaps we could consider an idea like
> >>> Peter's to have a sslmode=require on the server side and ease the
> >>> generation of HBA rules..
>
> >> Please find attached the rebased patch.
> >>
> >> Peter's suggestion seems a little more subtle to me than requiring TLS
> >> on the server side in that what people generally want to do is
> >> disallow clear text connections entirely.  In those scenarios, people
> >> would also want to set (and be able to change at runtime) some kind of
> >> cryptographic policy, as SSH and TLS do. While I see this as a worthy
> >> goal, it's a much bigger lift than an optional argument or two to
> >> initdb, and requires a lot more discussion than it's had to date.
>
> FWIW, I still agree with what Michael says above.  I do not think
> that adding more options to initdb is a useful solution here.
> In the first place, it's unlikely that we'll manage to cover many
> people's exact requirements this way.  In the second place, it's
> very unclear where to stop adding options.  In the third place,
> I believe the vast majority of users don't invoke initdb "by hand"
> anymore.  The typical scenario is to go through a packager-provided
> script, which almost certainly won't offer access to these additional
> options.  In the fourth place, many people won't know at initdb time
> exactly what they should do, or they'll change their minds later.
>

AFAIK bot the debian/ubuntu script mentioned by Isaac downthread, and the
RedHat/Fedora ones do allow you to specify inidb options.  That would cover
the majority I'd say...

That said, I agree with not adding it as an option to initdb. You'll
quickly get to the point where you specify the whole pg_hba file on the
commandline to initdb -- and most people today who actually care that much
about it would have their pg_hba.conf file under some sort of configuration
management anyway, whether it's ansible, chef, puppet or something else.


The last two points suggest that what'd be more useful is some sort
> of tool to modify an existing pg_hba.conf file.  Or maybe even just
>

I don't think we need, or indeed want, a tool to *modify* pg_hba.conf. For
people who want that, there are already plenty options out there in the
configuration management space, let's not invent our own.



> audit a file to see if it implements $desired-policy, such as
> "no unencrypted network connections" or "no plaintext passwords".
> (I kind of like the auditing-tool approach; it seems less scary
> than something that actually rewrites the file.)
>

Audiring, however, is a lot more interesting.

For people who actually care about most of this, it's not that important
what the initial one is, if it can trivially be changed to become insecure.
And unfortunately due to the complexity of pg_hba, that can easily happen.
Keeping it under configuration management helps with that, but doesn't
entirely solve the problem.

Another possible approach could be to add global gucs for
"allow_unencrypted_connections" and maybe
"available_authentication_methods". That would override pg_hba. At least in
doing so, there would be *one* spot where one could fairly strictly lock
things down. (Similar to Peters suggestion upthread)

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

faster ETL / bulk data load for heap tables

[Luc Vlaming]

Hi,

In an effort to speed up bulk data loading/transforming I noticed that 
considerable time is spent in the relation extension lock. I know there 
are already many other efforts to increase the chances of using bulk 
loading [1], [2], [3], [4], efforts to make loading more parallel [5], 
and removing some syscalls [6], as well as completely new I/O systems 
[7] and some more extreme measures like disabling wal logging [8].


Whilst they will all help, they will ultimately be stopped by the 
relation extension lock. Also it seems from the tests I've done so far 
that at least for bulk loading using pwrite() actually can carry us 
rather far as long as we are not doing it under a global lock. Moreover, 
the solution provided here might be an alternative to [7] because the 
results are quite promising, even with WAL enabled.


Attached two WIP patches in the hopes of getting feedback.
The first changes the way we do bulk loading: each backend now gets a 
standalone set of blocks allocated that are local to that backend. This 
helps both with reducing contention but also with some OS writeback 
mechanisms.
The second patch reduces the time spent on locking the partition buffers 
by shifting around the logic to make each set of 128 blocks use the same 
buffer partition, and then adding a custom function to get buffer blocks 
specifically for extension, whilst keeping a previous partition lock, 
thereby reducing the amount of time we spent on futexes.


The design:
- add a set of buffers into the BulkInsertState that can be used by the 
backend without any locks.
- add a ReadBufferExtendBulk function which extends the relation with 
BULK_INSERT_BATCH_SIZE blocks at once.
- rework FileWrite to have a parameter to speed up relation extension by 
passing in if we are using filewrite just to extend the file. if 
supported uses ftruncate as this is much faster (also than 
posix_fallocate on my system) and according to the manpages 
(https://linux.die.net/man/2/ftruncate) should read as zeroed space. to 
be cleaned-up later possibly into a special function FileExtend().

- rework mdextend to get a page count.
- make a specialized version of BufferAlloc called BufferAllocExtend 
which keeps around the lock on the last buffer partition and tries to 
reuse this so that there are a lot less futex calls.


Many things that are still to be done; some are:
- reintroduce FSM again, and possibly optimize the lock usage there. in 
other words: this patch currently can only start the server and run COPY 
FROM and read queries.
- look into the wal logging. whilst it seems to be fairly optimal 
already wrt the locking and such i noticed there seems to be an 
alternating pattern between the bgwriter and the workers. whilst setting 
some parameters bigger helped a lot (wal_writer_flush_after, 
wal_writer_delay, wal_buffers)
- work nicely with the code from [6] so that the register_dirty_segment 
is indeed not needed anymore; or alternatively optimize that code so 
that less locks are needed.
- make use of [9] in the fallback case in FileWrite() when 
ftruncate/fallocate is not available so that the buffer size can be reduced.


First results are below; all tests were loading 32 times the same 1G 
lineitem csv into the same table. tests were done both on a nvme and the 
more parallel ones also with a tmpfs disk to see potential disk 
bottlenecks and e.g. potential wrt using NVDIMM.

=
using an unlogged table:
NVME, UNLOGGED table, 4 parallel streams:   HEAD 171s, patched 113s
NVME, UNLOGGED table, 8 parallel streams:   HEAD 113s, patched 67s
NVME, UNLOGGED table, 16 parallel streams:  HEAD 112s, patched 42s
NVME, UNLOGGED table, 32 parallel streams:  HEAD 121s, patched 36s
tmpfs, UNLOGGED table, 16 parallel streams: HEAD 96s, patched 38s
tmpfs, UNLOGGED table, 32 parallel streams: HEAD 104s, patched 25s
=
using a normal table, wal-level=minimal, 16mb wal segments:
NVME, 4 parallel streams:   HEAD 237s, patched 157s
NVME, 8 parallel streams:   HEAD 200s, patched 142s
NVME, 16 parallel streams:  HEAD 171s, patched 145s
NVME, 32 parallel streams:  HEAD 199s, patched 146s
tmpfs, 16 parallel streams: HEAD 131s, patched 89s
tmpfs, 32 parallel streams: HEAD 148s, patched 98s
=
using a normal table, wal-level=minimal, 256mb wal segments,
wal_init_zero = off, wal_buffers = 262143, wal_writer_delay = 1ms,
wal_writer_flush_after = 512MB

NVME, 4 parallel streams:   HEAD 201s, patched 159s
NVME, 8 parallel streams:   HEAD 157s, patched 109s
NVME, 16 parallel streams:  HEAD 150s, patched 78s
NVME, 32 parallel streams:  HEAD 158s, patched 70s
tmpfs, 16 parallel streams: HEAD 106s, patched 54s
tmpfs, 32 parallel streams: HEAD 113s, patched 44s
=

Thoughts?

Cheers,
Luc
Swarm64


[1] 
https://www.postgresql.org/message-id/flat/CAJcOf-f%3DUX1uKbPjDXf%2B8gJOoEPz9VCzh7pKnknfU4sG4LXj0A%40mail.gmail.com#49fe9f2ffcc9916cc

Re: [PATCH] postgres_fdw connection caching - cause remote sessions linger till the local session exit

[Zhihong Yu]

Hi, Bharath:

Happy new year.

+   appendStringInfo(&buf, "(%s, %s)", server->servername,
+entry->invalidated ? "false" : "true");

Is it better to use 'invalidated' than 'false' in the string ?

For the first if block of postgres_fdw_disconnect():

+* Check if the connection associated with the given foreign server
is
+* in use i.e. entry->xact_depth > 0. Since we can not close it, so
+* error out.
+*/
+   if (is_in_use)
+   ereport(WARNING,

since is_in_use is only set in the if (server) block, I think the above
warning can be moved into that block.

Cheers

On Fri, Jan 1, 2021 at 2:04 AM Bharath Rupireddy <
bharath.rupireddyforpostg...@gmail.com> wrote:

> On Thu, Dec 31, 2020 at 8:29 AM Bharath Rupireddy
>  wrote:
> > Right. I meant the "next use" as the select attempt on a foreign table
> > with that foreign server. If no select query is run, then at the end
> > of the current txn that connection gets closed. Yes internally such
> > connection gets closed in pgfdw_xact_callback.
> >
> > If the errdetail("Such connections get closed either in the next use
> > or at the end of the current transaction.") looks confusing, how about
> >
> > 1) errdetail("Such connection gets discarded while closing the remote
> > transaction.")/errdetail("Such connections get discarded while closing
> > the remote transaction.")
> > 2) errdetail("Such connection is discarded at the end of remote
> > transaction.")/errdetail("Such connections are discarded at the end of
> > remote transaction.")
> >
> > I prefer 2)  Thoughts?
> >
> > Because we already print a message in pgfdw_xact_callback -
> > elog(DEBUG3, "closing remote transaction on connection %p"
>
> I changed the message to "Such connection is discarded at the end of
> remote transaction.".
>
> I'm attaching v5 patch set i.e. all the patches 0001 ( for new
> functions), 0002 ( for GUC) and 0003 (for server level option). I have
> also made the changes for increasing the version of
> postgres_fdw--1.0.sql from 1.0 to 1.1.
>
> I have no open points from my end. Please consider the v5 patch set
> for further review.
>
> With Regards,
> Bharath Rupireddy.
> EnterpriseDB: http://www.enterprisedb.com
>

Re: Safety/validity of resetting permissions by updating system tables

[Tom Lane]

Isaac Morland  writes:
> Is it safe and valid to reset to default permissions by doing
> UPDATE pg_namespace/pg_class/pg_type/pg_proc
> SET nspacl/relacl/typacl/proacl = NULL WHERE ... to accomplish this?

Not terribly; the main objection is you'd fail to update pg_shdepend.

> And what do people think, conceptually, of the notion of adding a command
> to do this without resorting to updating system tables directly?

I'm a little skeptical as to the use-case, particularly once you take
ALTER DEFAULT PRIVILEGES into account and try to figure out what that
means.  If it means "apply the current default privileges", you could
easily be "resetting" to a state that never actually prevailed in the
past.

regards, tom lane

Re: poc - possibility to write window function in PL languages

[Zhihong Yu]

Hi, Pavel:
Happy New Year.

+   command with clause WINDOW. The specific feature of
+   this functions is a possibility to two special storages with

this functions -> this function

possibility to two special storages: there is no verb.

'store with stored one value': store is repeated.

+ * Portions Copyright (c) 1996-2020, PostgreSQL Global Development Group

It would be better to change 2020 to 2021 in the new files.

For some functions, such as windowobject_get_func_arg_frame, it would be
better to add comment explaining their purposes.

For estimate_partition_context_size():
+errmsg("size of value is greather than limit (1024
bytes)")));

Please include the value of typlen in the message. There is similar error
message in the else block where value of size should be included.

+   return *realsize;
+   }
+   else

The 'else' is not needed since the if block ends with return.

+   size += size / 3;

Please add a comment for the choice of constant 3.

+   /* by default we allocate 30 bytes */
+   *realsize = 0;

The value 30 may not be accurate - from the caller:

+   if (PG_ARGISNULL(2))
+   minsize = VARLENA_MINSIZE;
+   else
+   minsize = PG_GETARG_INT32(2);

VARLENA_MINSIZE is 32.

Cheers

On Fri, Jan 1, 2021 at 3:29 AM Pavel Stehule 
wrote:

> Hi
>
> rebase
>
> Regards
>
> Pavel
>
>

Re: Spurious "apparent wraparound" via SimpleLruTruncate() rounding

[Andrey Borodin]

Hi Noah!

I've found this thread in CF looking for something to review.

> 9 нояб. 2020 г., в 09:53, Noah Misch  написал(а):
> 
> Rebased both patches, necessitated by commit c732c3f (a repair of commit
> dee663f).  As I mentioned on another branch of the thread, I'd be content if
> someone reviews the slru-truncate-modulo patch and disclaims knowledge of the
> slru-truncate-insurance patch; I would then abandon the latter patch.
> 

Commit c732c3f adds some SYNC_FORGET_REQUESTs.
slru-truncate-modulo-v5.patch fixes off-by-one error in functions like 
*PagePrecedes(int page1, int page2).
slru-truncate-t-insurance-v4.patch ensures that off-by-one errors do not 
inflict data loss.

While I agree that fixing error is better than hiding it, I could not figure 
out how c732c3f is connected to these patches.
Can you please give me few pointers how to understand this connection?

Best regards, Andrey Borodin.

Re: Proposed patch for key management

[Alastair Turner]

Hi Stephen, Bruce, Fabien

On Thu, 31 Dec 2020 at 17:05, Stephen Frost  wrote:
>
> Greetings,
>
> * Bruce Momjian (br...@momjian.us) wrote:
> > On Thu, Dec 31, 2020 at 11:11:11AM +0100, Fabien COELHO wrote:
> > > > I am not sure what else I can add to this discussion.  Having something
> > > > that is completely external might be a nice option, but I don't think it
> > > > is the common use-case, and will make the most common use cases more
> > > > complex.
> > >
> > > I'm unsure about what is the "common use case". ISTM that having the
> > > postgres process holding the master key looks like a "no" for me in any 
> > > use
> > > case with actual security concern: as the database must be remotely
> > > accessible, a reasonable security assumption is that a pg account could be
> > > compromised, and the "master key" (if any, that is just one particular
> > > cryptographic design) should not be accessible in that case. The first
> > > barrier would be pg admin account.
> >
> > Let's unpack this logic, since I know others, like Alastair Turner (CC
> > added), had similar concerns.  Frankly, I feel this feature has limited
> > security usefulness, so if we don't feel it has sufficient value, let's
> > mark it as not wanted and move on.
>
> Considering the amount of requests for this, I don't feel it's at all
> reasonable to suggest that it's 'not wanted'.
>

FWIW, I think that cluster file encryption has great value, even if we
only consider its incremental value on top of disk or volume
encryption. Particularly in big IT estates where monitoring tools,
backup managers, etc have read access to a lot of mounted volumes.
Limiting the impact of these systems (or their credentials) being
compromised is definitely useful.

>
> > I think there are two basic key configurations:
> >
> > 1.  pass data encryption keys in from an external source
> > 2.  store the data encryption keys wrapped by a key encryption key (KEK)
> > passed in from an external source
>
> I agree those are two basic configurations (though they aren't the only
> possible ones).
>

If we rephrase these two in terms of what the server process does to
get a key, we could say

 1. Get the keys from another process at startup (and maybe reload) time
 2. Get the wrapped keys from a local file...
 2.a. directly and unwrap them using the server's own logic and user prompt
 2.b. via a library which manages wrapping and user prompting

With the general feeling in the mailing list discussions favouring an
approach in the #2 family, I have been looking at options around 2b. I
was hoping to provide some flexibility for users without adding
complexity to the pg code base and use some long standing,
standardised, features rather than reinventing or recoding the wheel.
It has not been a complete success, or failure.

The nearest thing to a standard for wrapped secret store files is
PKCS#12. I searched and cannot find it discussed on-list in this
context before. It is the format used for Java local keystores in
recent versions. The format is supported, up to a point, by OpenSSL,
nss and gnuTLS. OpenSSL also has command line support for many
operations on the files, including updating the wrapping password.

Content in the PKCS12 files is stored in typed "bags". The challenge
is that the OpenSSL command line tools currently support only the
content types related to TLS operations - certificates, public or
private keys, CRLs, ... and not the untyped secret bag which is
appropriate for storing AES keys. The c APIs will work with this
content but there are not as many helper functions as for other
content types.

For future information - the nss command line tools understand the
secret bags, but don't offer nearly so many options for configuration.

For building something now - the OpenSSL pkcs12 functions provide
features like tags and friendly names for keys, and configurable key
derivation from passwords, but the code to interact with the
secretBags is accumulating quickly in my prototype.

After the long intro, my question - If using a standard format,
managed by a library, for the internal keystore does not result in a
smaller or simpler patch, is there enough other value for this to be
worth considering? For security features, standards and building on
well exercised code bases do really have value, but is it enough...

Regards
Alastair

Re: faster ETL / bulk data load for heap tables

[Zhihong Yu]

Hi, Luc:
Happy New Year.

Looking at BufferAllocExtend()
in v1-0002-WIP-buffer-alloc-specialized-for-relation-extensi.patch. it
seems there is duplicate code with the existing BufferAlloc().

It would be good if some refactoring is done by extracting common code into
a helper function.

Thanks

On Fri, Jan 1, 2021 at 6:07 AM Luc Vlaming  wrote:

> Hi,
>
> In an effort to speed up bulk data loading/transforming I noticed that
> considerable time is spent in the relation extension lock. I know there
> are already many other efforts to increase the chances of using bulk
> loading [1], [2], [3], [4], efforts to make loading more parallel [5],
> and removing some syscalls [6], as well as completely new I/O systems
> [7] and some more extreme measures like disabling wal logging [8].
>
> Whilst they will all help, they will ultimately be stopped by the
> relation extension lock. Also it seems from the tests I've done so far
> that at least for bulk loading using pwrite() actually can carry us
> rather far as long as we are not doing it under a global lock. Moreover,
> the solution provided here might be an alternative to [7] because the
> results are quite promising, even with WAL enabled.
>
> Attached two WIP patches in the hopes of getting feedback.
> The first changes the way we do bulk loading: each backend now gets a
> standalone set of blocks allocated that are local to that backend. This
> helps both with reducing contention but also with some OS writeback
> mechanisms.
> The second patch reduces the time spent on locking the partition buffers
> by shifting around the logic to make each set of 128 blocks use the same
> buffer partition, and then adding a custom function to get buffer blocks
> specifically for extension, whilst keeping a previous partition lock,
> thereby reducing the amount of time we spent on futexes.
>
> The design:
> - add a set of buffers into the BulkInsertState that can be used by the
> backend without any locks.
> - add a ReadBufferExtendBulk function which extends the relation with
> BULK_INSERT_BATCH_SIZE blocks at once.
> - rework FileWrite to have a parameter to speed up relation extension by
> passing in if we are using filewrite just to extend the file. if
> supported uses ftruncate as this is much faster (also than
> posix_fallocate on my system) and according to the manpages
> (https://linux.die.net/man/2/ftruncate) should read as zeroed space. to
> be cleaned-up later possibly into a special function FileExtend().
> - rework mdextend to get a page count.
> - make a specialized version of BufferAlloc called BufferAllocExtend
> which keeps around the lock on the last buffer partition and tries to
> reuse this so that there are a lot less futex calls.
>
> Many things that are still to be done; some are:
> - reintroduce FSM again, and possibly optimize the lock usage there. in
> other words: this patch currently can only start the server and run COPY
> FROM and read queries.
> - look into the wal logging. whilst it seems to be fairly optimal
> already wrt the locking and such i noticed there seems to be an
> alternating pattern between the bgwriter and the workers. whilst setting
> some parameters bigger helped a lot (wal_writer_flush_after,
> wal_writer_delay, wal_buffers)
> - work nicely with the code from [6] so that the register_dirty_segment
> is indeed not needed anymore; or alternatively optimize that code so
> that less locks are needed.
> - make use of [9] in the fallback case in FileWrite() when
> ftruncate/fallocate is not available so that the buffer size can be
> reduced.
>
> First results are below; all tests were loading 32 times the same 1G
> lineitem csv into the same table. tests were done both on a nvme and the
> more parallel ones also with a tmpfs disk to see potential disk
> bottlenecks and e.g. potential wrt using NVDIMM.
> =
> using an unlogged table:
> NVME, UNLOGGED table, 4 parallel streams:   HEAD 171s, patched 113s
> NVME, UNLOGGED table, 8 parallel streams:   HEAD 113s, patched 67s
> NVME, UNLOGGED table, 16 parallel streams:  HEAD 112s, patched 42s
> NVME, UNLOGGED table, 32 parallel streams:  HEAD 121s, patched 36s
> tmpfs, UNLOGGED table, 16 parallel streams: HEAD 96s, patched 38s
> tmpfs, UNLOGGED table, 32 parallel streams: HEAD 104s, patched 25s
> =
> using a normal table, wal-level=minimal, 16mb wal segments:
> NVME, 4 parallel streams:   HEAD 237s, patched 157s
> NVME, 8 parallel streams:   HEAD 200s, patched 142s
> NVME, 16 parallel streams:  HEAD 171s, patched 145s
> NVME, 32 parallel streams:  HEAD 199s, patched 146s
> tmpfs, 16 parallel streams: HEAD 131s, patched 89s
> tmpfs, 32 parallel streams: HEAD 148s, patched 98s
> =
> using a normal table, wal-level=minimal, 256mb wal segments,
> wal_init_zero = off, wal_buffers = 262143, wal_writer_delay = 1ms,
> wal_writer_flush_after = 512MB
>
> NVME, 4 parallel streams:   HEAD 201s, patched 15

Move --data-checksums to common options in initdb --help

[Michael Banck]

Hi,

I noticed -k/--data-checksums is currently in the less commonly used
options part of the initdb --help output:

|Less commonly used options:
|  -d, --debug   generate lots of debugging output
|  -k, --data-checksums  use data page checksums

I think enough people use data checksums these days that it warrants to
be moved into the "normal part", like in the attached.


Michael

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

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

Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index 0865f73ee0..71ded8af39 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -2275,6 +2275,7 @@ usage(const char *progname)
 	printf(_(" [-D, --pgdata=]DATADIR location for this database cluster\n"));
 	printf(_("  -E, --encoding=ENCODING   set default encoding for new databases\n"));
 	printf(_("  -g, --allow-group-access  allow group read/execute on data directory\n"));
+	printf(_("  -k, --data-checksums  use data page checksums\n"));
 	printf(_("  --locale=LOCALE   set default locale for new databases\n"));
 	printf(_("  --lc-collate=, --lc-ctype=, --lc-messages=LOCALE\n"
 			 "  --lc-monetary=, --lc-numeric=, --lc-time=LOCALE\n"
@@ -2290,7 +2291,6 @@ usage(const char *progname)
 	printf(_("  --wal-segsize=SIZEsize of WAL segments, in megabytes\n"));
 	printf(_("\nLess commonly used options:\n"));
 	printf(_("  -d, --debug   generate lots of debugging output\n"));
-	printf(_("  -k, --data-checksums  use data page checksums\n"));
 	printf(_("  -L DIRECTORY  where to find the input files\n"));
 	printf(_("  -n, --no-cleando not clean up after errors\n"));
 	printf(_("  -N, --no-sync do not wait for changes to be written safely to disk\n"));

Re: Safety/validity of resetting permissions by updating system tables

[Isaac Morland]

On Fri, 1 Jan 2021 at 11:44, Tom Lane  wrote:

> Isaac Morland  writes:
> > Is it safe and valid to reset to default permissions by doing
> > UPDATE pg_namespace/pg_class/pg_type/pg_proc
> > SET nspacl/relacl/typacl/proacl = NULL WHERE ... to accomplish this?
>
> Not terribly; the main objection is you'd fail to update pg_shdepend.
>

Right, the object would still be recorded as depending on the role, even
though it really didn't any more. I should have considered that.

I think I can fix that by first looping through using aclexplode() and
issuing a REVOKE against every role mentioned, then do a table update to
replace the empty array acl with a NULL. Of course I could also update
pg_shdepend myself but the goal is to minimize direct system table updates.

Thanks for the response.


> > And what do people think, conceptually, of the notion of adding a command
> > to do this without resorting to updating system tables directly?
>
> I'm a little skeptical as to the use-case, particularly once you take
> ALTER DEFAULT PRIVILEGES into account and try to figure out what that
> means.  If it means "apply the current default privileges", you could
> easily be "resetting" to a state that never actually prevailed in the
> past.


The use case is to ensure that after doing my GRANTs the permissions are in
a known state, no matter what they were before. Typically, one would follow
a reset command with some GRANTs. So maybe my permissions script contains:

GRANT UPDATE ON TABLE t1, t2 TO u1, u2;

Later, I revise this to:

GRANT UPDATE ON TABLE t1, t2 TO u1;

But the obsolete permissions will still be available to u2. I would like to
be able to put something like this at the top of the permissions script:

RESET PERMISSIONS ON ALL TABLES IN SCHEMA test;

Or in a different context:

RESET PERMISSIONS ON TABLE t1, t2;

Note: I'm not particularly fond of "RESET PERMISSIONS" as the syntax; I
just wrote that as an example of what it might look like.

If the tables are newly created this would have no effect; if they were
existing tables it would change the permissions to what newly created
tables would have.

In the absence of default privileges, I think it's clear that this means
setting the acl column (relacl, proacl, ...) to NULL; with default
privileges, I think it probably means resetting acl to NULL and then
applying the current default privileges as if the object had just been
created by its owner. As you point out, it's possible the object never had
this privilege set, which is an argument against using the word "reset" in
describing the feature. Maybe "GRANT DEFAULT"? But it's weird for GRANT to
actually revoke privileges, as it would for most object types.

Re: Moving other hex functions to /common

[Bruce Momjian]

On Thu, Dec 31, 2020 at 03:10:29PM +0900, Michael Paquier wrote:
> On Wed, Dec 30, 2020 at 08:22:07PM -0500, Bruce Momjian wrote:
> > So, I am learning this cfbot thing.  Seems I need -M100% to disable
> > rename detection for diffs to work with cfbot --- makes sense.
> 
> A good way to make sure that a patch format is correct for the CF bot
> would be to use "git format-patch -1" to generate a patch from a
> single commit.

Thanks.  I had to learn how to squash my commits into a new branch and
then generate a format-patch on that:

https://bugsdb.com/_en/debug/8b648ec395b86be32efa9629cb006d74

I wanted to see how the cfbot liked my original patch with the renames,
and it didn't, so now I know I have to use this method for the
commitfest.  Patch attached.

> > New patch attached.
> 
> I think that this patch would have more value if we remove completely
> the hex routines from ECPG and have ECPG use what's moved in
> src/common/, meaning the following changes:
> - Remove the exit(), pg_log_fatal() and ereport() calls from
> src/common/hex.c, replace the error code paths with -1, and return a
> signed result.
> - The ECPG copies make no use of ecpg_raise(), so things map easily.
> - This means keeping small wrappers in encode.c able to generate those
> ereport(FATAL) in the backend, but that's just necessary for the
> decode routine that's the only thing using get_hex().
> - Let's prefix the routines in src/common/ with "pg_", to be
> consistent with base64.
> - It would be good to document the top each routine in hex.c (see
> base64.c for a similar reference).

Let me get my patch building on the cfbot and then I will address each
of these.  I am trying to do one stage at a time since I am still
learning the process.  Thanks.

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

  The usefulness of a cup is in its emptiness, Bruce Lee

>From 464746996a22149edc607241d27837bf876cceda Mon Sep 17 00:00:00 2001
From: Bruce Momjian 
Date: Fri, 1 Jan 2021 15:04:42 -0500
Subject: [PATCH] hex squash commit

---
 src/backend/replication/backup_manifest.c |  2 +-
 src/backend/utils/adt/encode.c| 34 +--
 src/backend/utils/adt/varlena.c   |  2 +-
 src/common/Makefile   |  2 +-
 src/common/{hex_decode.c => hex.c}| 40 ---
 src/include/common/hex.h (new)| 18 ++
 src/include/common/hex_decode.h (gone)| 16 -
 src/include/utils/builtins.h  |  3 --
 src/tools/msvc/Mkvcbuild.pm   |  2 +-
 9 files changed, 59 insertions(+), 60 deletions(-)

diff --git a/src/backend/replication/backup_manifest.c b/src/backend/replication/backup_manifest.c
index c3f339c556..716f114d78 100644
--- a/src/backend/replication/backup_manifest.c
+++ b/src/backend/replication/backup_manifest.c
@@ -17,7 +17,7 @@
 #include "libpq/pqformat.h"
 #include "mb/pg_wchar.h"
 #include "replication/backup_manifest.h"
-#include "utils/builtins.h"
+#include "common/hex.h"
 #include "utils/json.h"
 
 static void AppendStringToManifest(backup_manifest_info *manifest, char *s);
diff --git a/src/backend/utils/adt/encode.c b/src/backend/utils/adt/encode.c
index a6c65b1657..bca941a496 100644
--- a/src/backend/utils/adt/encode.c
+++ b/src/backend/utils/adt/encode.c
@@ -15,7 +15,7 @@
 
 #include 
 
-#include "common/hex_decode.h"
+#include "common/hex.h"
 #include "mb/pg_wchar.h"
 #include "utils/builtins.h"
 #include "utils/memutils.h"
@@ -141,38 +141,6 @@ binary_decode(PG_FUNCTION_ARGS)
 }
 
 
-/*
- * HEX
- */
-
-static const char hextbl[] = "0123456789abcdef";
-
-uint64
-hex_encode(const char *src, size_t len, char *dst)
-{
-	const char *end = src + len;
-
-	while (src < end)
-	{
-		*dst++ = hextbl[(*src >> 4) & 0xF];
-		*dst++ = hextbl[*src & 0xF];
-		src++;
-	}
-	return (uint64) len * 2;
-}
-
-static uint64
-hex_enc_len(const char *src, size_t srclen)
-{
-	return (uint64) srclen << 1;
-}
-
-static uint64
-hex_dec_len(const char *src, size_t srclen)
-{
-	return (uint64) srclen >> 1;
-}
-
 /*
  * BASE64
  */
diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c
index 9300d19e0c..79fcdcd178 100644
--- a/src/backend/utils/adt/varlena.c
+++ b/src/backend/utils/adt/varlena.c
@@ -22,7 +22,7 @@
 #include "catalog/pg_type.h"
 #include "common/hashfn.h"
 #include "common/int.h"
-#include "common/hex_decode.h"
+#include "common/hex.h"
 #include "common/unicode_norm.h"
 #include "lib/hyperloglog.h"
 #include "libpq/pqformat.h"
diff --git a/src/common/Makefile b/src/common/Makefile
index f624977939..93eb27a2aa 100644
--- a/src/common/Makefile
+++ b/src/common/Makefile
@@ -58,7 +58,7 @@ OBJS_COMMON = \
 	file_perm.o \
 	file_utils.o \
 	hashfn.o \
-	hex_decode.o \
+	hex.o \
 	ip.o \
 	jsonapi.o \
 	keywords.o \
diff --git a/src/common/hex_decode.c b/src/common/hex.c
similarity index 79%
rename from src/common/hex_decode.c
rename to src/common/hex.c
in

Re: pgbench: option delaying queries till connections establishment?

[Fabien COELHO]

It looks like macOS doesn't have pthread barriers (via cfbot 2021, now
with more operating systems):


Indeed:-(

I'll look into that.

--
Fabien.

Re: Spurious "apparent wraparound" via SimpleLruTruncate() rounding

[Noah Misch]

On Fri, Jan 01, 2021 at 11:05:29PM +0500, Andrey Borodin wrote:
> I've found this thread in CF looking for something to review.

Thanks for taking a look.

> > 9 нояб. 2020 г., в 09:53, Noah Misch  написал(а):
> > 
> > Rebased both patches, necessitated by commit c732c3f (a repair of commit
> > dee663f).  As I mentioned on another branch of the thread, I'd be content if
> > someone reviews the slru-truncate-modulo patch and disclaims knowledge of 
> > the
> > slru-truncate-insurance patch; I would then abandon the latter patch.
> > 
> 
> Commit c732c3f adds some SYNC_FORGET_REQUESTs.
> slru-truncate-modulo-v5.patch fixes off-by-one error in functions like 
> *PagePrecedes(int page1, int page2).
> slru-truncate-t-insurance-v4.patch ensures that off-by-one errors do not 
> inflict data loss.
> 
> While I agree that fixing error is better than hiding it, I could not figure 
> out how c732c3f is connected to these patches.
> Can you please give me few pointers how to understand this connection?

Commit c732c3f is the last commit that caused a merge conflict.  There's no
other connection to this thread, and one can review patches on this thread
without studying commit c732c3f.  Specifically, this thread's
slru-truncate-modulo patch and commit c732c3f modify adjacent lines in
SlruScanDirCbDeleteCutoff(); here's the diff after merge conflict resolution:

--- a/src/backend/access/transam/slru.c
+++ b/src/backend/access/transam/slru.c
@@@ -1525,4 -1406,4 +1517,4 @@@ SlruScanDirCbDeleteCutoff(SlruCtl ctl, 
  
 -  if (ctl->PagePrecedes(segpage, cutoffPage))
 +  if (SlruMayDeleteSegment(ctl, segpage, cutoffPage))
-   SlruInternalDeleteSegment(ctl, filename);
+   SlruInternalDeleteSegment(ctl, segpage / 
SLRU_PAGES_PER_SEGMENT);
  

Re: Implement for window functions

[Krasiyan Andreev]

Hi, it looks like cfbot.cputube.org didn't recognize and can't apply a
patch, so I resend it now with a different format, no other changes.

На ср, 30.12.2020 г. в 22:16 ч. Krasiyan Andreev 
написа:

> It works - now it compiles clean and all checks are passed, thank you. I
> will continue with more complex tests.
>
> На ср, 30.12.2020 г. в 21:50 ч. David Fetter  написа:
>
>> On Wed, Dec 30, 2020 at 09:32:26PM +0200, Krasiyan Andreev wrote:
>> > Hi, after latest committed patches about multirange datatypes, I get a
>> > compilation error,
>>
>> Oh, right. I'd been meaning to send a patch to fix that. Here it is.
>>
>> Best,
>> David.
>> --
>> David Fetter  http://fetter.org/
>> Phone: +1 415 235 3778
>>
>> Remember to vote!
>> Consider donating to Postgres: http://www.postgresql.org/about/donate
>>
>
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 5021ac1ca9..0e877c48df 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -20373,6 +20373,14 @@ SELECT count(*) FROM sometable;
about frame specifications.
   
 
+  
+   The functions lead, lag,
+   first_value, last_value, and
+   nth_value accept a null treatment option which is
+   RESPECT NULLS or IGNORE NULLS.
+   If this option is not specified, the default is RESPECT NULLS.
+  
+
   
When an aggregate function is used as a window function, it aggregates
over the rows within the current row's window frame.
@@ -20386,14 +20394,9 @@ SELECT count(*) FROM sometable;
 
   

-The SQL standard defines a RESPECT NULLS or
-IGNORE NULLS option for lead, lag,
-first_value, last_value, and
-nth_value.  This is not implemented in
-PostgreSQL: the behavior is always the
-same as the standard's default, namely RESPECT NULLS.
-Likewise, the standard's FROM FIRST or FROM LAST
-option for nth_value is not implemented: only the
+The SQL standard defines a FROM FIRST or FROM LAST
+option for nth_value.  This is not implemented in
+PostgreSQL: only the
 default FROM FIRST behavior is supported.  (You can achieve
 the result of FROM LAST by reversing the ORDER BY
 ordering.)
diff --git a/doc/src/sgml/ref/create_function.sgml b/doc/src/sgml/ref/create_function.sgml
index 3c1eaea651..31e08c26b4 100644
--- a/doc/src/sgml/ref/create_function.sgml
+++ b/doc/src/sgml/ref/create_function.sgml
@@ -28,6 +28,7 @@ CREATE [ OR REPLACE ] FUNCTION
   { LANGUAGE lang_name
 | TRANSFORM { FOR TYPE type_name } [, ... ]
 | WINDOW
+| TREAT NULLS
 | IMMUTABLE | STABLE | VOLATILE | [ NOT ] LEAKPROOF
 | CALLED ON NULL INPUT | RETURNS NULL ON NULL INPUT | STRICT
 | [ EXTERNAL ] SECURITY INVOKER | [ EXTERNAL ] SECURITY DEFINER
@@ -293,6 +294,17 @@ CREATE [ OR REPLACE ] FUNCTION
  
 
 
+
+ TREAT NULLS
+
+ 
+  TREAT NULLS indicates that the function is able
+  to handle the RESPECT NULLS and IGNORE
+  NULLS options.  Only window functions may specify this.
+  
+ 
+
+
 
  IMMUTABLE
  STABLE
diff --git a/doc/src/sgml/syntax.sgml b/doc/src/sgml/syntax.sgml
index d66560b587..103595d21b 100644
--- a/doc/src/sgml/syntax.sgml
+++ b/doc/src/sgml/syntax.sgml
@@ -1766,6 +1766,8 @@ FROM generate_series(1,10) AS s(i);
 The syntax of a window function call is one of the following:
 
 
+function_name (expression , expression ... ) [ RESPECT NULLS | IGNORE NULLS ] OVER window_name
+function_name (expression , expression ... ) [ RESPECT NULLS | IGNORE NULLS ] OVER ( window_definition )
 function_name (expression , expression ... ) [ FILTER ( WHERE filter_clause ) ] OVER window_name
 function_name (expression , expression ... ) [ FILTER ( WHERE filter_clause ) ] OVER ( window_definition )
 function_name ( * ) [ FILTER ( WHERE filter_clause ) ] OVER window_name
@@ -1779,6 +1781,18 @@ FROM generate_series(1,10) AS s(i);
 [ ORDER BY expression [ ASC | DESC | USING operator ] [ NULLS { FIRST | LAST } ] [, ...] ]
 [ frame_clause ]
 
+   
+
+   
+
+ The versions with RESPECT NULLS or IGNORE
+ NULLS only apply to true window functions, whereas the versions
+ with FILTER only apply to aggregate functions used as
+ window functions.
+
+   
+
+   
 The optional frame_clause
 can be one of
 
diff --git a/src/backend/catalog/pg_aggregate.c b/src/backend/catalog/pg_aggregate.c
index 7664bb6285..fea8778ec8 100644
--- a/src/backend/catalog/pg_aggregate.c
+++ b/src/backend/catalog/pg_aggregate.c
@@ -627,6 +627,7 @@ AggregateCreate(const char *aggName,
 	 * definable for agg) */
 			 false, /* isLeakProof */
 			 false, /* isStrict (not needed for agg) */
+			 false, /* null_treatment (not needed for agg) */
 			 PROVOLATILE_IMMUTABLE, /* volatility (not needed
 	 * for agg) */
 			 proparallel,
@@ -848,7 +849,7 @@ lookup_agg_function(List *fnName,
 			   nargs, input_types, false, false,
 			   &fnOid, rettype, &retset,
 			   &nvargs, &vatype,
-			   &true_oid_a

Re: Implement for window functions

[Zhihong Yu]

Krasiyan:
Happy New Year.

For WinGetFuncArgInPartition():

+   if (target > 0)
+   step = 1;
+   else if (target < 0)
+   step = -1;
+   else
+   step = 0;

When would the last else statement execute ? Since the above code is
for WINDOW_SEEK_CURRENT, I wonder why step should be 0.

Similar question for the last else statement below
in WinGetFuncArgInFrame():

+   else if (seektype == WINDOW_SEEK_TAIL)
+   step = -1;
+   else
+   step = 0;

Thanks

On Fri, Jan 1, 2021 at 12:59 PM Krasiyan Andreev  wrote:

> Hi, it looks like cfbot.cputube.org didn't recognize and can't apply a
> patch, so I resend it now with a different format, no other changes.
>
> На ср, 30.12.2020 г. в 22:16 ч. Krasiyan Andreev 
> написа:
>
>> It works - now it compiles clean and all checks are passed, thank you. I
>> will continue with more complex tests.
>>
>> На ср, 30.12.2020 г. в 21:50 ч. David Fetter  написа:
>>
>>> On Wed, Dec 30, 2020 at 09:32:26PM +0200, Krasiyan Andreev wrote:
>>> > Hi, after latest committed patches about multirange datatypes, I get a
>>> > compilation error,
>>>
>>> Oh, right. I'd been meaning to send a patch to fix that. Here it is.
>>>
>>> Best,
>>> David.
>>> --
>>> David Fetter  http://fetter.org/
>>> Phone: +1 415 235 3778
>>>
>>> Remember to vote!
>>> Consider donating to Postgres: http://www.postgresql.org/about/donate
>>>
>>

Re: adding wait_start column to pg_locks

[Justin Pryzby]

On Tue, Dec 15, 2020 at 11:47:23AM +0900, torikoshia wrote:
> So I'm now thinking about adding a new column in pg_locks which
> keeps the time at which locks started waiting.
> 
> Attached a patch.

This is failing make check-world, would you send an updated patch ?

I added you as an author so it shows up here.
http://cfbot.cputube.org/atsushi-torikoshi.html

-- 
Justin

Re: [PATCH] postgres_fdw connection caching - cause remote sessions linger till the local session exit

[Bharath Rupireddy]

Thanks for taking a look at the patches.

On Fri, Jan 1, 2021 at 9:35 PM Zhihong Yu  wrote:
> Happy new year.
>
> +   appendStringInfo(&buf, "(%s, %s)", server->servername,
> +entry->invalidated ? "false" : "true");
>
> Is it better to use 'invalidated' than 'false' in the string ?

This point was earlier discussed in [1] and [2], but the agreement was
on having true/false [2] because of a simple reason specified in [1],
that is when some users have foreign server names as invalid or valid,
then the output is difficult to interpret which one is what. With
having true/false, it's easier. IMO, let's keep the true/false as is,
since it's also suggested in [2].

[1] - 
https://www.postgresql.org/message-id/CALj2ACUv%3DArQXs0U9PM3YXKCeSzJ1KxRokDY0g_0aGy--kDScA%40mail.gmail.com
[2] - 
https://www.postgresql.org/message-id/6da38393-6ae5-4d87-2690-11c932123403%40oss.nttdata.com

> For the first if block of postgres_fdw_disconnect():
>
> +* Check if the connection associated with the given foreign server is
> +* in use i.e. entry->xact_depth > 0. Since we can not close it, so
> +* error out.
> +*/
> +   if (is_in_use)
> +   ereport(WARNING,
>
> since is_in_use is only set in the if (server) block, I think the above 
> warning can be moved into that block.

Modified that a bit. Since we error out when no server object is
found, then no need of keeping the code in else part. We could save on
some indentation

+if (!server)
+ereport(ERROR,
+(errcode(ERRCODE_CONNECTION_DOES_NOT_EXIST),
+ errmsg("foreign server \"%s\" does not exist",
servername)));
+
+hashvalue = GetSysCacheHashValue1(FOREIGNSERVEROID,
+  ObjectIdGetDatum(server->serverid));
+result = disconnect_cached_connections(hashvalue, false, &is_in_use);
+
+/*
+ * Check if the connection associated with the given foreign server is
+ * in use i.e. entry->xact_depth > 0. Since we can not close it, so
+ * error out.
+ */
+if (is_in_use)
+ereport(WARNING,
+(errmsg("cannot close the connection because it
is still in use")));

Attaching v6 patch set. Please have a look.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com


v6-0001-postgres_fdw-function-to-discard-cached-connectio.patch
Description: Binary data


v6-0002-postgres_fdw-add-keep_connections-GUC-to-not-cach.patch
Description: Binary data


v6-0003-postgres_fdw-server-level-option-keep_connection-.patch
Description: Binary data

Re: Moving other hex functions to /common

[Michael Paquier]

On Fri, Jan 01, 2021 at 03:06:13PM -0500, Bruce Momjian wrote:
> Thanks.  I had to learn how to squash my commits into a new branch and
> then generate a format-patch on that:
> 
>   https://bugsdb.com/_en/debug/8b648ec395b86be32efa9629cb006d74
> 
> I wanted to see how the cfbot liked my original patch with the renames,
> and it didn't, so now I know I have to use this method for the
> commitfest.  Patch attached.

There are many ways to do that, indeed.  On my end, I use a local
branch. and then apply a set of git reset --soft before recreating a
single commit.

> Let me get my patch building on the cfbot and then I will address each
> of these.  I am trying to do one stage at a time since I am still
> learning the process.  Thanks.

No problem.  On my end, this stuff has been itching me for a couple of
days and I could not recall why..  Until I remembered that the design
of the hex APIs in your patch is weak with overflow handling because
we don't pass down to the function the size of the destination buffer.
We have finished with a similar set of issues on the past with SCRAM
and base64, with has led to CVE-2019-10164 and the improvements done
in cfc40d3.  So I think that we need to improve things in a safer way.
Mapping with the design for base64, I have finished with the attached
patch, and the following set:
+extern int64 pg_hex_decode(const char *src, int64 len, char *dst, int64 
dstlen);
+extern int64 pg_hex_encode(const char *src, int64 len, char *dst, int64 
dstlen);
+extern int64 pg_hex_enc_len(int64 srclen);
+extern int64 pg_hex_dec_len(int64 srclen);

This ensures that the result never overflows, which explains the
introduction of an error code for the encoding part, and does not 
use elog()/pg_log() so as external libraries can use them.  ECPG uses
long variables in a couple of places, explaining why it feels safer to
use int64.  int should give enough room to any caller of those APIs,
but there is no drawback in using a 64-bit API either, and I don't
think it is worth the risk to break ECPG either for long handling,
even if I don't believe either that folks are going to work on strings
larger than 2GB.

Things get trickier for the bytea input/output because we want more
generic error messages depending for invalid values or an incorrect
number of digits, which is why I have left the copies in encode.c.
This design could be easily extended with more types of error codes,
though I am not sure if that's worth bothering.

Even with that, this leads to much more sanity for hex buffer
manipulation in backup manifests (I don't think that using  
PG_SHAXXX_DIGEST_STRING_LENGTH is a good idea either, I'd like to get
rid of it in the long-term) and ECPG, so that's clearly a gain.

I don't have a Windows host at hand, though I think that it should
work there correctly.  What do you think about the ideas in the
attached patch?
--
Michael
From 8952fb2d5e5a7a01799fe380408a17441a185436 Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Sat, 2 Jan 2021 14:00:22 +0900
Subject: [PATCH] Refactor the hex code

---
 src/include/common/hex.h |  23 +++
 src/include/common/hex_decode.h  |  16 --
 src/include/utils/builtins.h |   1 +
 src/backend/replication/backup_manifest.c|  39 +++--
 src/backend/utils/adt/encode.c   |  60 ++-
 src/backend/utils/adt/varlena.c  |  12 +-
 src/common/Makefile  |   2 +-
 src/common/hex.c | 166 +++
 src/common/hex_decode.c  | 106 
 src/interfaces/ecpg/ecpglib/data.c   |  92 +-
 src/interfaces/ecpg/ecpglib/ecpglib_extern.h |   3 -
 src/interfaces/ecpg/ecpglib/execute.c|  28 +++-
 src/interfaces/ecpg/include/ecpgerrno.h  |   1 +
 src/tools/msvc/Mkvcbuild.pm  |   2 +-
 14 files changed, 321 insertions(+), 230 deletions(-)
 create mode 100644 src/include/common/hex.h
 create mode 100644 src/common/hex.c

diff --git a/src/include/common/hex.h b/src/include/common/hex.h
new file mode 100644
index 00..cf36895ee4
--- /dev/null
+++ b/src/include/common/hex.h
@@ -0,0 +1,23 @@
+/*
+ *
+ *	hex.h
+ *	  Encoding and decoding routines for hex strings.
+ *
+ *	Portions Copyright (c) 1996-2020, PostgreSQL Global Development Group
+ *	Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * IDENTIFICATION
+ *		  src/include/common/hex.h
+ *
+ *
+ */
+
+#ifndef COMMON_HEX_H
+#define COMMON_HEX_H
+
+extern int64 pg_hex_decode(const char *src, int64 len, char *dst, int64 dstlen);
+extern int64 pg_hex_encode(const char *src, int64 len, char *dst, int64 dstlen);
+extern int64 pg_hex_enc_len(int64 srclen);
+extern int64 pg_hex_dec_len(int64 srclen);
+
+#endif			/* COMMON_HEX_H */
diff --git a/src/include/commo

Re: Move --data-checksums to common options in initdb --help

[Michael Paquier]

On Fri, Jan 01, 2021 at 08:34:34PM +0100, Michael Banck wrote:
> I think enough people use data checksums these days that it warrants to
> be moved into the "normal part", like in the attached.

+1.  Let's see first what others think about this change.
--
Michael


signature.asc
Description: PGP signature

Re: [PATCH] postgres_fdw connection caching - cause remote sessions linger till the local session exit

[Bharath Rupireddy]

On Sat, Jan 2, 2021 at 10:53 AM Bharath Rupireddy
 wrote:
> Thanks for taking a look at the patches.
>
> On Fri, Jan 1, 2021 at 9:35 PM Zhihong Yu  wrote:
> > Happy new year.
> >
> > +   appendStringInfo(&buf, "(%s, %s)", server->servername,
> > +entry->invalidated ? "false" : "true");
> >
> > Is it better to use 'invalidated' than 'false' in the string ?
>
> This point was earlier discussed in [1] and [2], but the agreement was
> on having true/false [2] because of a simple reason specified in [1],
> that is when some users have foreign server names as invalid or valid,
> then the output is difficult to interpret which one is what. With
> having true/false, it's easier. IMO, let's keep the true/false as is,
> since it's also suggested in [2].
>
> [1] - 
> https://www.postgresql.org/message-id/CALj2ACUv%3DArQXs0U9PM3YXKCeSzJ1KxRokDY0g_0aGy--kDScA%40mail.gmail.com
> [2] - 
> https://www.postgresql.org/message-id/6da38393-6ae5-4d87-2690-11c932123403%40oss.nttdata.com
>
> > For the first if block of postgres_fdw_disconnect():
> >
> > +* Check if the connection associated with the given foreign server 
> > is
> > +* in use i.e. entry->xact_depth > 0. Since we can not close it, so
> > +* error out.
> > +*/
> > +   if (is_in_use)
> > +   ereport(WARNING,
> >
> > since is_in_use is only set in the if (server) block, I think the above 
> > warning can be moved into that block.
>
> Modified that a bit. Since we error out when no server object is
> found, then no need of keeping the code in else part. We could save on
> some indentation
>
> +if (!server)
> +ereport(ERROR,
> +(errcode(ERRCODE_CONNECTION_DOES_NOT_EXIST),
> + errmsg("foreign server \"%s\" does not exist",
> servername)));
> +
> +hashvalue = GetSysCacheHashValue1(FOREIGNSERVEROID,
> +  
> ObjectIdGetDatum(server->serverid));
> +result = disconnect_cached_connections(hashvalue, false, &is_in_use);
> +
> +/*
> + * Check if the connection associated with the given foreign server 
> is
> + * in use i.e. entry->xact_depth > 0. Since we can not close it, so
> + * error out.
> + */
> +if (is_in_use)
> +ereport(WARNING,
> +(errmsg("cannot close the connection because it
> is still in use")));
>
> Attaching v6 patch set. Please have a look.

I'm sorry for the mess. I missed adding the new files into the v6-0001
patch. Please ignore the v6 patch set and consder the v7 patch set for
further review. Note that 0002 and 0003 patches have no difference
from v5 patch set.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com


v7-0001-postgres_fdw-function-to-discard-cached-connectio.patch
Description: Binary data


v7-0002-postgres_fdw-add-keep_connections-GUC-to-not-cach.patch
Description: Binary data


v7-0003-postgres_fdw-server-level-option-keep_connection-.patch
Description: Binary data

Re: WIP: BRIN multi-range indexes

[Thomas Munro]

On Sun, Dec 20, 2020 at 1:16 PM Tomas Vondra
 wrote:
> Attached is an updated version of the patch series, rebased on current
> master, and results for benchmark comparing the various bloom variants.

Perhaps src/include/utils/inet.h needs to include ,
because FreeBSD says:

brin_minmax_multi.c:1693:24: error: use of undeclared identifier 'AF_INET'
if (ip_family(ipa) == PGSQL_AF_INET)
^
../../../../src/include/utils/inet.h:39:24: note: expanded from macro
'PGSQL_AF_INET'
#define PGSQL_AF_INET (AF_INET + 0)
^

Re: psql \df choose functions by their arguments

[Thomas Munro]

On Thu, Dec 31, 2020 at 7:01 AM Greg Sabino Mullane  wrote:
> Attached is the latest patch against HEAD - basically fixes a few typos.

Hi Greg,

It looks like there is a collation dependency here that causes the
test to fail on some systems:

=== ./src/test/regress/regression.diffs ===
diff -U3 /tmp/cirrus-ci-build/src/test/regress/expected/psql.out
/tmp/cirrus-ci-build/src/test/regress/results/psql.out
--- /tmp/cirrus-ci-build/src/test/regress/expected/psql.out 2021-01-01
16:05:25.749692000 +
+++ /tmp/cirrus-ci-build/src/test/regress/results/psql.out 2021-01-01
16:11:28.525632000 +
@@ -5094,8 +5094,8 @@
public | mtest | integer | double precision, double precision, integer | func
public | mtest | integer | integer | func
public | mtest | integer | integer, text | func
- public | mtest | integer | timestamp without time zone, timestamp
with time zone | func
public | mtest | integer | time without time zone, time with time zone | func
+ public | mtest | integer | timestamp without time zone, timestamp
with time zone | func

Re: Spurious "apparent wraparound" via SimpleLruTruncate() rounding

[Andrey Borodin]

> 2 янв. 2021 г., в 01:35, Noah Misch  написал(а):
> There's no
> other connection to this thread, and one can review patches on this thread
> without studying commit c732c3f. 

OK, thanks!

Do I understand correctly that this is bugfix that needs to be back-patched?
Thus we should not refactor 4 identical *PagePrecedes(int page1, int page2) 
into 1 generic function?
Since functions are not symmetric anymore, maybe we should have better names 
for arguments than "page1" and "page2"? At least in dev branch.

Is it common practice to embed tests into assert checking like in 
SlruPagePrecedesUnitTests()?

SLRU seems no near simple, BTW. The only simple place is naive caching 
algorithm. I remember there was a thread to do relations from SLRUs.

Best regards, Andrey Borodin.

Re: faster ETL / bulk data load for heap tables

[Amit Kapila]

On Fri, Jan 1, 2021 at 7:37 PM Luc Vlaming  wrote:
>
> Hi,
>
> In an effort to speed up bulk data loading/transforming I noticed that
> considerable time is spent in the relation extension lock.
>

We already do extend the relation in bulk when there is a contention
on relation extension lock via RelationAddExtraBlocks. I wonder why is
that not able to address this kind of workload. On a quick look at
your patch, it seems you are always trying to extend the relation by
128 blocks for copy operation after acquiring the lock whereas the
current mechanism has some smarts where it decides based on the number
of waiters. Now, it is possible that we should extend by a larger
number of blocks as compared to what we are doing now but using some
ad-hoc number might lead to space wastage. Have you tried to fiddle
with the current scheme of bulk-extension to see if that addresses
some of the gains you are seeing? I see that you have made quite a few
other changes that might be helping here but still, it is better to
see how much bottleneck is for relation extension lock and if that can
be addressed with the current mechanism rather than changing the
things in a different way.

-- 
With Regards,
Amit Kapila.