Re: PoC Refactor AM analyse API

[Denis Smirnov]

> But why do you pass int natts and VacAttrStats **stats to 
> acquire_sample_rows()? Is it of any use? It seems to break abstraction too.

Yes, it is really a kluge that should be discussed. The main problem is that we 
don’t pass projection information to analyze scan (analyze begin scan relise 
only on relation information during initialization). And as a result we can’t 
benefit from column AMs during «analyze t(col)» and consume data only from 
target columns. This parameters were added to solve this problem.

Best regards,
Denis Smirnov | Developer
s...@arenadata.io 
Arenadata | Godovikova 9-17, Moscow 129085 Russia

Re: Parallel Inserts in CREATE TABLE AS

[Dilip Kumar]

On Wed, Dec 30, 2020 at 10:49 AM Dilip Kumar  wrote:
>
> On Wed, 30 Dec 2020 at 10:47 AM, Bharath Rupireddy 
>  wrote:
>>
>> On Wed, Dec 30, 2020 at 10:32 AM Dilip Kumar  wrote:
>> > I have completed reviewing 0001, I don't have more comments, just one
>> > question.  Soon I will review the remaining patches.
>>
>> Thanks.
>>
>> > +/* If parallel inserts are to be allowed, set a few extra 
>> > information. */
>> > +if (myState->is_parallel)
>> > +{
>> > +myState->object_id = intoRelationAddr.objectId;
>> > +
>> > +/*
>> > + * We don't need to skip contacting FSM while inserting tuples for
>> > + * parallel mode, while extending the relations, workers instead 
>> > of
>> > + * blocking on a page while another worker is inserting, can 
>> > check the
>> > + * FSM for another page that can accommodate the tuples. This 
>> > results
>> > + * in major benefit for parallel inserts.
>> > + */
>> > +myState->ti_options = 0;
>> >
>> > Is there any performance data for this or just theoretical analysis?
>>
>> I have seen that we don't get much performance with the skip fsm
>> option, though I don't have the data to back it up. I'm planning to
>> run performance tests after the patches 0001, 0002 and 0003 get
>> reviewed. I will capture the data at that time. Hope that's fine.
>
>
> Yeah that’s fine
>

Some comments in 0002

1.
+/*
+ * Information sent to the planner from CTAS to account for the cost
+ * calculations in cost_gather. We need to do this because, no tuples will be
+ * received by the Gather node if the workers insert the tuples in parallel.
+ */
+typedef enum CTASParallelInsertOpt
+{
+ CTAS_PARALLEL_INS_UNDEF = 0, /* undefined */
+ CTAS_PARALLEL_INS_SELECT = 1 << 0, /* turn on this before planning */
+ /*
+ * Turn on this while planning for upper Gather path to ignore parallel
+ * tuple cost in cost_gather.
+ */
+ CTAS_PARALLEL_INS_TUP_COST_CAN_IGN = 1 << 1,
+ /* Turn on this after the cost is ignored. */
+ CTAS_PARALLEL_INS_TUP_COST_IGNORED = 1 << 2
+} CTASParallelInsertOpt;


I don't like the naming of these flags.  Especially no need to define
CTAS_PARALLEL_INS_UNDEF, we can directl use 0
for that purpose instead of giving some weird name.  So I suggest
first, just get rid of CTAS_PARALLEL_INS_UNDEF.

2.
+ /*
+ * Turn on a flag to ignore parallel tuple cost by the Gather path in
+ * cost_gather if the SELECT is for CTAS and we are generating an upper
+ * level Gather path.
+ */
+ bool ignore = ignore_parallel_tuple_cost(root);
+
  generate_useful_gather_paths(root, rel, false);

+ /*
+ * Reset the ignore flag, in case we turned it on but
+ * generate_useful_gather_paths returned without reaching cost_gather.
+ * If we reached cost_gather, we would have been reset it there.
+ */
+ if (ignore && (root->parse->CTASParallelInsInfo &
+ CTAS_PARALLEL_INS_TUP_COST_CAN_IGN))
+ {
+ root->parse->CTASParallelInsInfo &=
+ ~CTAS_PARALLEL_INS_TUP_COST_CAN_IGN;
+ }

I think th way we are using these cost ignoring flag, doesn't look clean.

I mean first, CTAS_PARALLEL_INS_SELECT is set if it is coming from
CTAS and then ignore_parallel_tuple_cost will
set the CTAS_PARALLEL_INS_TUP_COST_CAN_IGN if it satisfies certain
condition which is fine.  Now, internally cost
gather will add CTAS_PARALLEL_INS_TUP_COST_IGNORED and remove
CTAS_PARALLEL_INS_TUP_COST_CAN_IGN and if
CTAS_PARALLEL_INS_TUP_COST_CAN_IGN is not removed then we will remove
it outside.  Why do we need to remove
CTAS_PARALLEL_INS_TUP_COST_CAN_IGN flag at all?

3.
+ if (tuple_cost_flags && gstate->ps.ps_ProjInfo)
+ Assert(!(*tuple_cost_flags & CTAS_PARALLEL_INS_TUP_COST_IGNORED));

Instead of adding Assert inside an IF statement, you can convert whole
statement as an assert.  Lets not add unnecessary
if in the release mode.

4.
+ if ((root->parse->CTASParallelInsInfo & CTAS_PARALLEL_INS_SELECT) &&
+ (root->parse->CTASParallelInsInfo &
+ CTAS_PARALLEL_INS_TUP_COST_CAN_IGN))
+ {
+ ignore_tuple_cost = true;
+ root->parse->CTASParallelInsInfo &=
+ ~CTAS_PARALLEL_INS_TUP_COST_CAN_IGN;
+ root->parse->CTASParallelInsInfo |= CTAS_PARALLEL_INS_TUP_COST_IGNORED;
+ }
+
+ if (!ignore_tuple_cost)
+ run_cost += parallel_tuple_cost * path->path.rows;

Changes this to (if, else) as shown below, because if it goes to the
IF part then ignore_tuple_cost will always be true
so no need to have an extra if check.

if ((root->parse->CTASParallelInsInfo & CTAS_PARALLEL_INS_SELECT) &&
(root->parse->CTASParallelInsInfo &
CTAS_PARALLEL_INS_TUP_COST_CAN_IGN))
{
ignore_tuple_cost = true;
root->parse->CTASParallelInsInfo &=
~CTAS_PARALLEL_INS_TUP_COST_CAN_IGN;
root->parse->CTASParallelInsInfo |= CTAS_PARALLEL_INS_TUP_COST_IGNORED;
}
else
run_cost += parallel_tuple_cost * path->path.rows;

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

popcount

[David Fetter]

Hi,

Per request, I'd like to see about surfacing $Subject to SQL.

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
>From 28c9b7acad605197a8a242ff929bcce6f3100c91 Mon Sep 17 00:00:00 2001
From: David Fetter 
Date: Wed, 30 Dec 2020 02:51:46 -0800
Subject: [PATCH v1] popcount
To: hackers
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="2.29.2"

This is a multi-part message in MIME format.
--2.29.2
Content-Type: text/plain; charset=UTF-8; format=fixed
Content-Transfer-Encoding: 8bit


Now it's accessible to SQL for the BIT VARYING and BYTEA types.

diff --git doc/src/sgml/func.sgml doc/src/sgml/func.sgml
index 5021ac1ca9..439a3a4a06 100644
--- doc/src/sgml/func.sgml
+++ doc/src/sgml/func.sgml
@@ -4069,6 +4069,23 @@ SELECT format('Testing %3$s, %2$s, %s', 'one', 'two', 'three');

   
 
+  
+   
+
+ popcount
+
+popcount ( bytes bytea )
+integer
+   
+   
+Counts the number of bits set in a binary string.
+   
+   
+popcount('\xdeadbeef'::bytea)
+24
+   
+  
+
   

 
@@ -4830,6 +4847,24 @@ SELECT format('Testing %3$s, %2$s, %s', 'one', 'two', 'three');
 101010001010101010

   
+
+  
+   
+
+ popcount
+
+popcount ( bits bit )
+int4
+   
+   
+Counts the bits set in a bit string.
+   
+   
+popcount(B'101010101010101010')
+9
+   
+  
+
  
 

diff --git src/include/catalog/pg_proc.dat src/include/catalog/pg_proc.dat
index 139f4a08bd..0e6d0636fa 100644
--- src/include/catalog/pg_proc.dat
+++ src/include/catalog/pg_proc.dat
@@ -1446,6 +1446,9 @@
 { oid => '752', descr => 'substitute portion of string',
   proname => 'overlay', prorettype => 'bytea',
   proargtypes => 'bytea bytea int4', prosrc => 'byteaoverlay_no_len' },
+{ oid => '8436', descr => 'count set bits',
+  proname => 'popcount', prorettype => 'int4', proargtypes => 'bytea',
+  prosrc => 'byteapopcount'},
 
 { oid => '725',
   proname => 'dist_pl', prorettype => 'float8', proargtypes => 'point line',
@@ -3865,6 +3868,9 @@
 { oid => '3033', descr => 'set bit',
   proname => 'set_bit', prorettype => 'bit', proargtypes => 'bit int4 int4',
   prosrc => 'bitsetbit' },
+{ oid => '8435', descr => 'count set bits',
+  proname => 'popcount', prorettype => 'int4', proargtypes => 'bit',
+  prosrc => 'bitpopcount'},
 
 # for macaddr type support
 { oid => '436', descr => 'I/O',
diff --git src/backend/utils/adt/varbit.c src/backend/utils/adt/varbit.c
index 3c03459f51..e92d1a7f8f 100644
--- src/backend/utils/adt/varbit.c
+++ src/backend/utils/adt/varbit.c
@@ -29,6 +29,8 @@
  *-
  */
 
+#include 
+
 #include "postgres.h"
 
 #include "access/htup_details.h"
@@ -36,6 +38,7 @@
 #include "libpq/pqformat.h"
 #include "nodes/nodeFuncs.h"
 #include "nodes/supportnodes.h"
+#include "port/pg_bitutils.h"
 #include "utils/array.h"
 #include "utils/builtins.h"
 #include "utils/varbit.h"
@@ -1872,3 +1875,24 @@ bitgetbit(PG_FUNCTION_ARGS)
 	else
 		PG_RETURN_INT32(0);
 }
+
+/*
+ * bitpopcount
+ *
+ * Returns the number of bits set in a bit array.
+ *
+ */
+Datum
+bitpopcount(PG_FUNCTION_ARGS)
+{
+	VarBit		*arg1 = PG_GETARG_VARBIT_P(0);
+	bits8		*p;
+	int			len, popcount;
+
+	len = ceil(VARBITLEN(arg1) / (float4)BITS_PER_BYTE);
+	p = VARBITS(arg1);
+
+	popcount = (int)pg_popcount((const char *)p, len);
+
+	PG_RETURN_INT32(popcount);
+}
diff --git src/backend/utils/adt/varlena.c src/backend/utils/adt/varlena.c
index 9300d19e0c..9e788babd3 100644
--- src/backend/utils/adt/varlena.c
+++ src/backend/utils/adt/varlena.c
@@ -3415,6 +3415,21 @@ bytea_overlay(bytea *t1, bytea *t2, int sp, int sl)
 	return result;
 }
 
+/*
+ * popcount
+ */
+Datum
+byteapopcount(PG_FUNCTION_ARGS)
+{
+	bytea	*t1 = PG_GETARG_BYTEA_PP(0);
+	int		len, result;
+
+	len = VARSIZE_ANY_EXHDR(t1);
+	result = pg_popcount(VARDATA_ANY(t1), len);
+
+	PG_RETURN_INT32(result);
+}
+
 /*
  * byteapos -
  *	  Return the position of the specified substring.
diff --git src/test/regress/expected/bit.out src/test/regress/expected/bit.out
index a1fab7ebcb..c91ad3d0d5 100644
--- src/test/regress/expected/bit.out
+++ src/test/regress/expected/bit.out
@@ -681,6 +681,19 @@ SELECT overlay(B'0101011100' placing '001' from 20);
  010101111
 (1 row)
 
+-- Popcount
+SELECT popcount(B'0101011100'::bit(10));
+ popcount 
+--
+5
+(1 row)
+
+SELECT popcount(B'11'::bit(10));
+ popcount 
+--
+   10
+(1 row)
+
 -- This table is intentionally left around to exercise pg_dump/pg_upgrade
 CREATE TABLE bit_defaults(
   b1 bit(4) DEFAULT '1001',
diff --git src/test/regress/expected/strings.out src/test/regress/expected/strings.out
index 298b6c48c2..97

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

[Alexey Kondratov]

On 2020-12-30 09:10, Bharath Rupireddy wrote:

Hi,

I'm posting a v4-0001 patch for the new functions
postgres_fdw_get_connections() and postgres_fdw_disconnect().  In this
patch, I tried to address the review comments provided upthread.

Thoughts?



I still have some doubts that it is worth of allowing to call 
postgres_fdw_disconnect() in the explicit transaction block, since it 
adds a lot of things to care and check for. But otherwise current logic 
looks solid.


+ errdetail("Such connections get closed either in the next use or 
at the end of the current transaction.")
+ : errdetail("Such connection gets closed either in the next use or 
at the end of the current transaction.")));


Does it really have a chance to get closed on the next use? If foreign 
server is dropped then user mapping should be dropped as well (either 
with CASCADE or manually), but we do need user mapping for a local cache 
lookup. That way, if I understand all the discussion up-thread 
correctly, we can only close such connections at the end of xact, do we?


+ * This function returns false if the cache doesn't exist.
+ * When the cache exists:

I think that this will be corrected later by pg_indent, but still. In 
this comment section following points 1) and 2) have a different 
combination of tabs/spaces.



Regards
--
Alexey Kondratov

Postgres Professional https://www.postgrespro.com
Russian Postgres CompanyFrom 82b82b901e8f0ca84e1b21454a3b68f4fd8f0016 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy 
Date: Wed, 30 Dec 2020 11:07:35 +0530
Subject: [PATCH v4] 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/connection.c | 319 +-
 .../postgres_fdw/expected/postgres_fdw.out| 415 +-
 contrib/postgres_fdw/postgres_fdw--1.0.sql|  15 +
 contrib/postgres_fdw/sql/postgres_fdw.sql | 166 +++
 doc/src/sgml/postgres-fdw.sgml|  57 ++-
 5 files changed, 953 insertions(+), 19 deletions(-)

diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c
index d841cec39b..496ef2bae2 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 *entry, UserMapping *user)
 	entry->have_error = false;
 	entry->changing_xact_state = false;
 	entry->invalidated = false;
+	entry->serverid = server->serverid;
 	entry->server_hashvalue =
 		GetSysCacheHashValue1(FOREIGNSERVEROID,
 			  ObjectIdGetDatum(server->serverid));
@@ -789,6 +801,13 @@ pgfdw_xact_callback(XactEvent event, void *arg)
 	if (!xact_got_connection)
 		return;
 
+	/*
+	 * Quick exit if the cache has been destroyed in
+	 * disconnect_cached_connections.
+	 */
+	if (!ConnectionHash)
+		return;
+
 	/*
 	 * Scan all connection cache entries to find open remote transactions, and
 	 * close them.
@@ -985,6 +1004,13 @@ pgfdw_subxact_callback(SubXactEvent event, SubTransactionId mySubid,
 	if (!xact_got_connection)
 		retu

Re: Parallel Inserts in CREATE TABLE AS

[vignesh C]

On Wed, Dec 30, 2020 at 10:47 AM Bharath Rupireddy
 wrote:
>
> On Wed, Dec 30, 2020 at 10:32 AM Dilip Kumar  wrote:
> > I have completed reviewing 0001, I don't have more comments, just one
> > question.  Soon I will review the remaining patches.
>
> Thanks.
>
> > +/* If parallel inserts are to be allowed, set a few extra information. 
> > */
> > +if (myState->is_parallel)
> > +{
> > +myState->object_id = intoRelationAddr.objectId;
> > +
> > +/*
> > + * We don't need to skip contacting FSM while inserting tuples for
> > + * parallel mode, while extending the relations, workers instead of
> > + * blocking on a page while another worker is inserting, can check 
> > the
> > + * FSM for another page that can accommodate the tuples. This 
> > results
> > + * in major benefit for parallel inserts.
> > + */
> > +myState->ti_options = 0;
> >
> > Is there any performance data for this or just theoretical analysis?
>
> I have seen that we don't get much performance with the skip fsm
> option, though I don't have the data to back it up. I'm planning to
> run performance tests after the patches 0001, 0002 and 0003 get
> reviewed. I will capture the data at that time. Hope that's fine.
>

When you run the performance tests, you can try to capture and publish
relation size & the number of pages that are getting created for base
table and the CTAS table, you can use something like SELECT relpages
FROM pg_class WHERE relname = 'tablename &  SELECT
pg_total_relation_size('tablename'). Just to make sure that there is
no significant difference between the base table and CTAS table.

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com

Re: [Patch] Optimize dropping of relation buffers using dlist

[Amit Kapila]

On Wed, Dec 30, 2020 at 11:28 AM Tang, Haiying
 wrote:
>
> Hi Amit,
>
> In last 
> mail(https://www.postgresql.org/message-id/66851e198f6b41eda59e6257182564b6%40G08CNEXMBPEKD05.g08.fujitsu.local),
> I've sent you the performance test results(run only 1 time) on single table. 
> Here is my the retested results(average by 15 times) which I think is more 
> accurate.
>
> In terms of 20G and 100G, the optimization on 100G is linear, but 20G is 
> nonlinear(also include test results on shared buffers of 50G/60G), so it's a 
> little difficult to decide the threshold from the two for me.
> If just consider 100G, I think NBuffers/32 is the optimized max relation 
> size. But I don't know how to judge for 20G. If you have any suggestion, 
> kindly let me know.
>

Considering these results NBuffers/64 seems a good threshold as beyond
that there is no big advantage. BTW, it is not clear why the advantage
for single table is not as big as multiple tables with the Truncate
command. Can you share your exact test steps for any one of the tests?
Also, did you change autovacumm = off for these tests, if not then the
results might not be reliable because before you run the test via
Vacuum command autovacuum would have done that work?

-- 
With Regards,
Amit Kapila.

Re: Parallel Inserts in CREATE TABLE AS

[vignesh C]

On Wed, Dec 30, 2020 at 9:25 AM Bharath Rupireddy
 wrote:
>
> On Wed, Dec 30, 2020 at 5:22 AM Zhihong Yu  wrote:
> > w.r.t. v17-0004-Enable-CTAS-Parallel-Inserts-For-Append.patch
> >
> > + * Push the dest receiver to Gather node when it is either at the top of 
> > the
> > + * plan or under top Append node unless it does not have any projections 
> > to do.
> >
> > I think the 'unless' should be 'if'. As can be seen from the body of the 
> > method:
> >
> > +   if (!ps->ps_ProjInfo)
> > +   {
> > +   GatherState *gstate = (GatherState *) ps;
> > +
> > +   parallel = true;
>
> Thanks. Modified it in the 0004 patch. Attaching v18 patch set. Note
> that no change in 0001 to 0003 patches from v17.
>
> Please consider v18 patch set for further review.
>

Few comments:
-   /*
-* To allow parallel inserts, we need to ensure that they are safe to be
-* performed in workers. We have the infrastructure to allow parallel
-* inserts in general except for the cases where inserts generate a new
-* CommandId (eg. inserts into a table having a foreign key column).
-*/
-   if (IsParallelWorker())
-   ereport(ERROR,
-   (errcode(ERRCODE_INVALID_TRANSACTION_STATE),
-errmsg("cannot insert tuples in a
parallel worker")));

Is it possible to add a check if it is a CTAS insert here as we do not
support insert in parallel workers from others as of now.

+   Oid objectid;   /* workers to
open relation/table.  */
+   /* Number of tuples inserted by all the workers. */
+   pg_atomic_uint64processed;

We can just mention relation instead of relation/table.

+select explain_pictas(
+'create table parallel_write as select length(stringu1) from tenk1;');
+  explain_pictas
+--
+ Gather (actual rows=N loops=N)
+   Workers Planned: 4
+   Workers Launched: N
+ ->  Create parallel_write
+   ->  Parallel Seq Scan on tenk1 (actual rows=N loops=N)
+(5 rows)
+
+select count(*) from parallel_write;

Can we include selection of cmin, xmin for one of the test to verify
that it uses the same transaction id  in the parallel workers
something like:
select distinct(cmin,xmin) from parallel_write;

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com

Re: Dump public schema ownership & seclabels

[Noah Misch]

On Tue, Dec 29, 2020 at 05:49:24AM -0800, Noah Misch wrote:
> https://postgr.es/m/20201031163518.gb4039...@rfd.leadboat.com gave $SUBJECT as
> one of the constituent projects for changing the public schema default ACL.
> This ended up being simple.  Attached.

This is defective; it fails to reproduce nspacl after "ALTER SCHEMA public
OWNER TO pg_write_server_files; REVOKE ALL ON SCHEMA public FROM
pg_write_server_files;".  I will try again later.

Re: Commitfest 2021-01

[Masahiko Sawada]

Hi all,

On Fri, Dec 25, 2020 at 4:32 PM Masahiko Sawada  wrote:
>
> Hello, hackers!
>
> 2021-01 commitfest will start from the new year, in a week. I'm happy
> to volunteer to be the CFM for this one.
>
> It's time to register your patch in the commitfest, if not yet.
>
> If you already have a patch in the commitfest, update its status and
> make sure it still applies and that the tests pass. Check the state at
> http://cfbot.cputube.org/
>
> If there is a long-running stale discussion, please send a short
> summary update about its current state, open questions, and TODOs. I
> hope it will encourage reviewers to pay more attention to the thread.
>

2021-01 commitfest will start just in a few days.

If you submit the patch to pgsql-hackers but don't register it to
commitfest app[1], please register it before 2021-01 AoE[2].

If you already have a patch in the commitfest, please update its
status and make sure it still applies to the HEAD and test pass. As of
now, there are about 40 patches that are Needs Review but don't pass
any cfbot tests[3].

Regards,

[1] https://commitfest.postgresql.org/31/
[2] https://en.wikipedia.org/wiki/Anywhere_on_Earth
[3] http://commitfest.cputube.org/

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

Re: pglz compression performance, take two

[Andrey Borodin]

Thanks for looking into this, Justin!

> 30 дек. 2020 г., в 09:39, Justin Pryzby  написал(а):
> 
> There's some typos in the current patch;
> 
> farer (further: but it's not your typo)
> positiion
> reduce a => reduce the
> monotonicity what => monotonicity, which
> lesser good => less good
> allign: align

Fixed.
> 
> This comment I couldn't understand:
> +* As initial compare for short matches compares 4 bytes then for the 
> end
> +* of stream length of match should be cut

I've reworded comments.

Best regards, Andrey Borodin.



v3-0001-Reorganize-pglz-compression-code.patch
Description: Binary data

Re: create table like: ACCESS METHOD

[Simon Riggs]

On Tue, 29 Dec 2020 at 23:08, Justin Pryzby  wrote:
>
> On Fri, Dec 25, 2020 at 03:41:46PM +0900, Michael Paquier wrote:
> > On Wed, Dec 09, 2020 at 02:13:29PM -0600, Justin Pryzby wrote:
> > > I thought this was a good idea, but didn't hear back when I raised it 
> > > before.
> > >
> > > Failing to preserve access method is arguably a bug, reminiscent of CREATE
> > > STATISTICS and 5564c1181.  But maybe it's not important to backpatch a 
> > > fix in
> > > this case, since access methods are still evolving.
> >
> > Interesting.  Access methods for tables are released for more than one
> > year now, so my take about a backpatch is that this boat has already
> > sailed.  This may give a reason to actually not introduce this
> > feature.
>
> Are you saying that since v12/13 didn't preserve the access method, it might 
> be
> preferred to never do it ?  I think it's reasonable to not change v12/13 but
> the behavior seems like an omission going forward.  It's not so important 
> right
> now, since AMs aren't widely used.

Omitting copying the AM seems like a bug during
  CREATE TABLE likeamlike(LIKE likeam INCLUDING ALL);
But this does allow you to specify the TableAM by using
default_table_access_method, and to use
  CREATE TABLE likeamlike(LIKE likeam INCLUDING ALL) USING (heapdup);
if you wish to set the AM explicitly, so I don't see this as needing backpatch.

Which means that the emphasis for the earlier functionality was
towards one "preferred AM" rather than using multiple AMs at same
time. Allowing this change in later releases makes sense.

Please make sure this is marked as an incompatibility in the release notes.

> > This patch should have more tests.  Something could be added in
> > create_am.sql where there is a fake heap2 as table AM.
>
> Yes, I had already done that locally.

There are no tests for the new functionality, please could you add some?

-- 
Simon Riggshttp://www.EnterpriseDB.com/

Re: Allow CURRENT_ROLE in GRANTED BY

[Simon Riggs]

On Thu, 10 Dec 2020 at 18:40, Peter Eisentraut
 wrote:
>
> On 2020-06-24 20:21, Peter Eisentraut wrote:
> > On 2020-06-24 10:12, Vik Fearing wrote:
> >> On 6/24/20 8:35 AM, Peter Eisentraut wrote:
> >>> I was checking some loose ends in SQL conformance, when I noticed: We
> >>> support GRANT role ... GRANTED BY CURRENT_USER, but we don't support
> >>> CURRENT_ROLE in that place, even though in PostgreSQL they are
> >>> equivalent.  Here is a trivial patch to add that.
> >>
> >>
> >> The only thing that isn't dead-obvious about this patch is the commit
> >> message says "[PATCH 1/2]".  What is in the other part?
> >
> > Hehe.  The second patch is some in-progress work to add the GRANTED BY
> > clause to the regular GRANT command.  More on that perhaps at a later date.
>
> Here is the highly anticipated and quite underwhelming second part of
> this patch set.

Looks great, but no test to confirm it works. I would suggest adding a
test and committing directly since I don't see any cause for further
discussion.

-- 
Simon Riggshttp://www.EnterpriseDB.com/

Re: Fail Fast In CTAS/CMV If Relation Already Exists To Avoid Unnecessary Rewrite, Planning Costs

[Michael Paquier]

On Thu, Dec 24, 2020 at 01:23:40PM +0900, Michael Paquier wrote:
> Please note that I have added an entry in the CF app for the moment so
> as we don't lose track of it:
> https://commitfest.postgresql.org/31/2892/

I have been able to look at that again today, and applied it.  I have
tweaked a bit the comments, and added an elog(ERROR) as a safety net
for explain.c if the IFNE code path is taken for an object type that
is not expected with CreateTableAsStmt.
--
Michael


signature.asc
Description: PGP signature

Re: Disable WAL logging to speed up data loading

[Michael Paquier]

On Mon, Dec 28, 2020 at 11:06:30AM +, Simon Riggs wrote:
> Agreed, it is a footgun. -1 to commit the patch as-is.
> 
> The patch to avoid WAL is simple but it is dangerous for both the user
> and the PostgreSQL project.

Something that has not been mentioned on this thread is that if you
could also put pg_wal/ on a RAM disk.  That's similarly unsafe, of
course, but it does not require any extra upstream patching, and that
should be really fast for the case of this thread.  If you care about
consistency, there are solutions like PMEM that we could look more 
into.

> In my experience, people will use this option and when it crashes and
> they lose their data, they will claim PostgreSQL broke and that they
> were not stupid enough to use this option. Data loss has always been
> the most serious error for PostgreSQL and our reputation for
> protecting data has been hard won; it can easily be lost in a moment
> of madness. Please consider how the headlines will read, bearing in
> mind past incidents and negative press.

Yeah..

> Yes, we did think of this feature already and rejected it.

I don't recall seeing such a discussion.  Do you have some references?
Just a matter of curiosity :)
--
Michael


signature.asc
Description: PGP signature

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

[Amit Kapila]

On Tue, Dec 29, 2020 at 3:15 PM Ajin Cherian  wrote:
>
> Hi Sawada-san,
>
> I think Amit has a plan to commit this patch-set in phases.
>

I have pushed the first patch and I would like to make a few changes
in the second patch after which I will post the new version. I'll try
to do that tomorrow if possible and register the patch.

> I will
> leave it to him to decide because I think he has a plan.
> I took time to refactor the test_decoding isolation test for
> consistent snapshot so that it uses just 3 sessions rather than 4.
> Posting an updated patch-0009
>

Thanks, I will look into this.

-- 
With Regards,
Amit Kapila.

Re: Lazy JIT IR code generation to increase JIT speed with partitions

[Luc Vlaming]

On 30-12-2020 02:57, Andres Freund wrote:

Hi,

Great to see work in this area!

On 2020-12-28 09:44:26 +0100, Luc Vlaming wrote:

I would like to propose a small patch to the JIT machinery which makes the
IR code generation lazy. The reason for postponing the generation of the IR
code is that with partitions we get an explosion in the number of JIT
functions generated as many child tables are involved, each with their own
JITted functions, especially when e.g. partition-aware joins/aggregates are
enabled. However, only a fraction of those functions is actually executed
because the Parallel Append node distributes the workers among the nodes.
With the attached patch we get a lazy generation which makes that this is no
longer a problem.


I unfortunately don't think this is quite good enough, because it'll
lead to emitting all functions separately, which can also lead to very
substantial increases of the required time (as emitting code is an
expensive step). Obviously that is only relevant in the cases where the
generated functions actually end up being used - which isn't the case in
your example.

If you e.g. look at a query like
   SELECT blub, count(*),sum(zap) FROM foo WHERE blarg = 3 GROUP BY blub;
on a table without indexes, you would end up with functions for

- WHERE clause (including deforming)
- projection (including deforming)
- grouping key
- aggregate transition
- aggregate result projection

with your patch each of these would be emitted separately, instead of
one go. Which IIRC increases the required time by a significant amount,
especially if inlining is done (where each separate code generation ends
up with copies of the inlined code).


As far as I can see you've basically falsified the second part of this
comment (which you moved):


+
+   /*
+* Don't immediately emit nor actually generate the function.
+* instead do so the first time the expression is actually evaluated.
+* That allows to emit a lot of functions together, avoiding a lot of
+* repeated llvm and memory remapping overhead. It also helps with not
+* compiling functions that will never be evaluated, as can be the case
+* if e.g. a parallel append node is distributing workers between its
+* child nodes.
+*/



-   /*
-* Don't immediately emit function, instead do so the first time the
-* expression is actually evaluated. That allows to emit a lot of
-* functions together, avoiding a lot of repeated llvm and memory
-* remapping overhead.
-*/


Greetings,

Andres Freund



Hi,

Happy to help out, and thanks for the info and suggestions.
Also, I should have first searched psql-hackers and the like, as I just 
found out there is already discussions about this in [1] and [2].
However I think the approach I took can be taken independently and then 
other solutions could be added on top.


Assuming I understood all suggestions correctly, the ideas so far are:
1. add a LLVMAddMergeFunctionsPass so that duplicate code is removed and 
not optimized several times (see [1]). Requires all code to be emitted 
in the same module.

2. JIT only parts of the plan, based on cost (see [2]).
3. Cache compilation results to avoid recompilation. this would either 
need a shm capable optimized IR cache or would not work with parallel 
workers.

4. Lazily jitting (this patch)

An idea that might not have been presented in the mailing list yet(?):
5. Only JIT in nodes that process a certain amount of rows. Assuming 
there is a constant overhead for JITting and the goal is to gain runtime.


Going forward I would first try to see if my current approach can work 
out. The only idea that would be counterproductive to my solution would 
be solution 1. Afterwards I'd like to continue with either solution 2, 
5, or 3 in the hopes that we can reduce JIT overhead to a minimum and 
can therefore apply it more broadly.


To test out why and where the JIT performance decreased with my solution 
I improved the test script and added various queries to model some of 
the cases I think we should care about. I have not (yet) done big scale 
benchmarks as these queries seemed to already show enough problems for 
now. Now there are 4 queries which test JITting with/without partitions, 
and with varying amounts of workers and rowcounts. I hope these are 
indeed a somewhat representative set of queries.


As pointed out the current patch does create a degradation in 
performance wrt queries that are not partitioned (basically q3 and q4). 
After looking into those queries I noticed two things:
- q3 is very noisy wrt JIT timings. This seems to be the result of 
something wrt parallel workers starting up the JITting and creating very 
high amounts of noise (e.g. inlining timings varying between 3.8s and 6.2s)

- q4 seems very stable with JIT timings (after the first run).
I'm wondering if this could mean that with parallel workers quite a lot 
of time is spent on 

Re: Added missing copy related data structures to typedefs.list

[Amit Kapila]

On Sat, Dec 26, 2020 at 9:16 PM vignesh C  wrote:
>
> On Thu, Dec 17, 2020 at 4:28 AM Bruce Momjian  wrote:
> >
> > On Mon, Dec  7, 2020 at 01:56:50PM +0530, vignesh C wrote:
> > > Hi,
> > >
> > > Added missing copy related data structures to typedefs.list, these
> > > data structures were added while copy files were split during the
> > > recent commit. I found this while running pgindent for parallel copy
> > > patches.
> > > The Attached patch has the changes for the same.
> > > Thoughts?
> >
> > Uh, we usually only update the typedefs file before we run pgindent on
> > the master branch.
> >
>
> Ok, Thanks for the clarification. I was not sure as in few of the
> enhancements it was included as part of the patches.
>

Yeah, I do that while committing patches that require changes in
typedefs. It is not a norm and I am not sure how much value it adds to
do it separately for the missing ones unless you are making changes in
the same file they are used and you are facing unrelated diffs due to
those missing ones.

-- 
With Regards,
Amit Kapila.

Re: [HACKERS] [PATCH] Generic type subscripting

[Dmitry Dolgov]

> On Sat, Dec 26, 2020 at 01:24:04PM -0500, Tom Lane wrote:
>
> In a case like jsonpath['...'], the initially UNKNOWN-type literal could
> in theory be coerced to any of these types, so you'd have to resolve that
> case manually.  The overloaded-function code has an internal preference
> that makes it choose TEXT if it has a choice of TEXT or some other target
> type for an UNKNOWN input (cf parse_func.c starting about line 1150), but
> if you ask can_coerce_type() it's going to say TRUE for all three cases.
>
> Roughly speaking, then, I think what you want to do is
>
> 1. If input type is UNKNOWNOID, choose result type TEXT.
>
> 2. Otherwise, apply can_coerce_type() to see if the input type can be
> coerced to int4, text, or jsonpath.  If it succeeds for none or more
> than one of these, throw error.  Otherwise choose the single successful
> type.
>
> 3. Apply coerce_type() to coerce to the chosen result type.
>
> 4. At runtime, examine exprType() of the input to figure out what to do.

Thanks, that was super useful. Following this suggestion I've made
necessary adjustments for the patch. There is no jsonpath support, but
this could be easily added on top.

Re: [HACKERS] [PATCH] Generic type subscripting

[Dmitry Dolgov]

> On Wed, Dec 30, 2020 at 02:45:12PM +0100, Dmitry Dolgov wrote:
> > On Sat, Dec 26, 2020 at 01:24:04PM -0500, Tom Lane wrote:
> >
> > In a case like jsonpath['...'], the initially UNKNOWN-type literal could
> > in theory be coerced to any of these types, so you'd have to resolve that
> > case manually.  The overloaded-function code has an internal preference
> > that makes it choose TEXT if it has a choice of TEXT or some other target
> > type for an UNKNOWN input (cf parse_func.c starting about line 1150), but
> > if you ask can_coerce_type() it's going to say TRUE for all three cases.
> >
> > Roughly speaking, then, I think what you want to do is
> >
> > 1. If input type is UNKNOWNOID, choose result type TEXT.
> >
> > 2. Otherwise, apply can_coerce_type() to see if the input type can be
> > coerced to int4, text, or jsonpath.  If it succeeds for none or more
> > than one of these, throw error.  Otherwise choose the single successful
> > type.
> >
> > 3. Apply coerce_type() to coerce to the chosen result type.
> >
> > 4. At runtime, examine exprType() of the input to figure out what to do.
>
> Thanks, that was super useful. Following this suggestion I've made
> necessary adjustments for the patch. There is no jsonpath support, but
> this could be easily added on top.

And the forgotten patch itself.
>From 7e2fa3dc4a8b1f907a385451c6782f2dfdc743ab Mon Sep 17 00:00:00 2001
From: Dmitrii Dolgov <9erthali...@gmail.com>
Date: Fri, 18 Dec 2020 17:19:51 +0100
Subject: [PATCH v39] Subscripting for jsonb

Subscripting implementation for jsonb. It does not support slices, does
not have a limit for number of subscripts and for assignment expects a
replace value to be of jsonb type. There is also one functional
difference in assignment via subscripting from jsonb_set, when an
original jsonb container is NULL, subscripting replaces it with an empty
jsonb and proceed with assignment.

For the sake of code reuse, some parts of jsonb functionality were
rearranged to allow use the same functions for jsonb_set and assign
subscripting operation.

The original idea belongs to Oleg Bartunov.

Reviewed-by: Tom Lane, Arthur Zakirov, Pavel Stehule
---
 doc/src/sgml/json.sgml  |  48 
 src/backend/utils/adt/Makefile  |   1 +
 src/backend/utils/adt/jsonb_util.c  |  76 -
 src/backend/utils/adt/jsonbsubs.c   | 412 
 src/backend/utils/adt/jsonfuncs.c   | 180 ++--
 src/include/catalog/pg_proc.dat |   4 +
 src/include/catalog/pg_type.dat |   3 +-
 src/include/utils/jsonb.h   |   6 +-
 src/test/regress/expected/jsonb.out | 272 +-
 src/test/regress/sql/jsonb.sql  |  84 +-
 10 files changed, 981 insertions(+), 105 deletions(-)
 create mode 100644 src/backend/utils/adt/jsonbsubs.c

diff --git a/doc/src/sgml/json.sgml b/doc/src/sgml/json.sgml
index 5b9a5557a4..100d1a60f4 100644
--- a/doc/src/sgml/json.sgml
+++ b/doc/src/sgml/json.sgml
@@ -602,6 +602,54 @@ SELECT jdoc->'guid', jdoc->'name' FROM api WHERE jdoc @> '{"tags": ["qu
   
  
 
+ 
+  jsonb Subscripting
+  
+   jsonb data type supports array-style subscripting expressions
+   to extract or update particular elements. It's possible to use multiple
+   subscripting expressions to extract nested values. In this case, a chain of
+   subscripting expressions follows the same rules as the
+   path argument in jsonb_set function,
+   e.g. in case of arrays it is a 0-based operation or that negative integers
+   that appear in path count from the end of JSON arrays.
+   The result of subscripting expressions is always jsonb data type. An
+   example of subscripting syntax:
+
+-- Extract value by key
+SELECT ('{"a": 1}'::jsonb)['a'];
+
+-- Extract nested value by key path
+SELECT ('{"a": {"b": {"c": 1}}}'::jsonb)['a']['b']['c'];
+
+-- Extract element by index
+SELECT ('[1, "2", null]'::jsonb)[1];
+
+-- Update value by key, note the single quotes - the assigned value
+-- needs to be of jsonb type as well
+UPDATE table_name SET jsonb_field['key'] = '1';
+
+-- Select records using where clause with subscripting. Since the result of
+-- subscripting is jsonb and we basically want to compare two jsonb objects, we
+-- need to put the value in double quotes to be able to convert it to jsonb.
+SELECT * FROM table_name WHERE jsonb_field['key'] = '"value"';
+
+
+  Subscripting for jsonb does not support slice expressions,
+  even if it contains an array.
+
+  In case if source jsonb is NULL, assignment
+  via subscripting will proceed as if it was an empty JSON object:
+
+-- If jsonb_field here is NULL, the result is {"a": 1}
+UPDATE table_name SET jsonb_field['a'] = '1';
+
+-- If jsonb_field here is NULL, the result is [1]
+UPDATE table_name SET jsonb_field[0] = '1';
+
+
+  
+ 
+
  
   Transforms
 
diff --git a/src/backend/utils/adt/Makefile b/src/backend/utils/adt/Makefile
index ce09ad7375..0ef0f36e36 100644
--- a/src/backend/utils/adt/Makefile
+++ b/src/backend/utils/adt/Makefile
@@ -50,6 +50,7 @@ OBJS 

Re: Disable WAL logging to speed up data loading

[Simon Riggs]

On Wed, 30 Dec 2020 at 13:04, Michael Paquier  wrote:

> > Yes, we did think of this feature already and rejected it.
>
> I don't recall seeing such a discussion.  Do you have some references?
> Just a matter of curiosity :)

Off-list discussions.

-- 
Simon Riggshttp://www.EnterpriseDB.com/

Re: allow partial union-all and improve parallel subquery costing

[Luc Vlaming]

On 23-10-2020 07:51, Luc Vlaming wrote:

On 14.10.20 09:38, Luc Vlaming wrote:

Hi,

It seems I ran the wrong make checks to verify everything is correct 
(make check instead
of make installcheck-world) and this uncovered another regress test 
change. I also noticed
the statistics are sometimes giving different row count results so I 
increased the row
statistics target to make sure the regress output is stable. Updated 
patch attached which

now successfully runs installcheck-world for v13 and master.

Kind regards,
Luc


From: Luc Vlaming 
Sent: Tuesday, October 13, 2020 10:57 AM
To: pgsql-hackers
Subject: allow partial union-all and improve parallel subquery costing

Hi,

While developing some improvements for TPC-DS queries I found out that 
with
UNION ALL partial paths are not emitted. Whilst fixing that I also 
came across
the subquery costing which does not seem to consider parallelism when 
doing

the costing.

I added a simplified testcase in pg-regress to show this goes wrong, and
attached also a before and after explain output of tpc-ds SF100 query 5
based on version 12.4.

I hope I followed all etiquette and these kind of improvements are 
welcome.


Kind regards,
Luc
Swarm64



Hi,

Created a commitfest entry assuming this is the right thing to do so 
that someone can potentially pick it up during the commitfest.


Kind regards,
Luc
Swarm64


Hi,

Providing an updated patch based on latest master.

Cheers,
Luc
>From 032c48b51ee0d436c91d9db81ce800d91168bd01 Mon Sep 17 00:00:00 2001
From: Luc Vlaming 
Date: Wed, 30 Dec 2020 14:49:48 +0100
Subject: [PATCH v3] Allow partial UNION ALL; improve parallel subquery costing

---
 src/backend/optimizer/path/costsize.c | 11 
 src/backend/optimizer/prep/prepunion.c|  4 ++
 .../regress/expected/incremental_sort.out | 10 ++--
 src/test/regress/expected/union.out   | 52 +++
 src/test/regress/sql/union.sql| 37 +
 5 files changed, 108 insertions(+), 6 deletions(-)

diff --git a/src/backend/optimizer/path/costsize.c b/src/backend/optimizer/path/costsize.c
index 22d6935824..1c3b04c4d7 100644
--- a/src/backend/optimizer/path/costsize.c
+++ b/src/backend/optimizer/path/costsize.c
@@ -1328,6 +1328,17 @@ cost_subqueryscan(SubqueryScanPath *path, PlannerInfo *root,
 	startup_cost += path->path.pathtarget->cost.startup;
 	run_cost += path->path.pathtarget->cost.per_tuple * path->path.rows;
 
+	/* Adjust costing for parallelism, if used. */
+	if (path->path.parallel_workers > 0)
+	{
+		double  parallel_divisor = get_parallel_divisor(&path->path);
+
+		path->path.rows = clamp_row_est(path->path.rows / parallel_divisor);
+
+		/* The CPU cost is divided among all the workers. */
+		run_cost /= parallel_divisor;
+	}
+
 	path->path.startup_cost += startup_cost;
 	path->path.total_cost += startup_cost + run_cost;
 }
diff --git a/src/backend/optimizer/prep/prepunion.c b/src/backend/optimizer/prep/prepunion.c
index 745f443e5c..4001eb87f3 100644
--- a/src/backend/optimizer/prep/prepunion.c
+++ b/src/backend/optimizer/prep/prepunion.c
@@ -678,6 +678,10 @@ generate_union_paths(SetOperationStmt *op, PlannerInfo *root,
 			   NIL, NULL,
 			   parallel_workers, enable_parallel_append,
 			   NIL, -1);
+
+		if (op->all && enable_parallel_append)
+			add_partial_path(result_rel, ppath);
+
 		ppath = (Path *)
 			create_gather_path(root, result_rel, ppath,
 			   result_rel->reltarget, NULL, NULL);
diff --git a/src/test/regress/expected/incremental_sort.out b/src/test/regress/expected/incremental_sort.out
index a8cbfd9f5f..40265674c4 100644
--- a/src/test/regress/expected/incremental_sort.out
+++ b/src/test/regress/expected/incremental_sort.out
@@ -1459,14 +1459,12 @@ explain (costs off) select * from t union select * from t order by 1,3;
->  Unique
  ->  Sort
Sort Key: t.a, t.b, t.c
-   ->  Append
- ->  Gather
-   Workers Planned: 2
+   ->  Gather
+ Workers Planned: 2
+ ->  Parallel Append
->  Parallel Seq Scan on t
- ->  Gather
-   Workers Planned: 2
->  Parallel Seq Scan on t t_1
-(13 rows)
+(11 rows)
 
 -- Full sort, not just incremental sort can be pushed below a gather merge path
 -- by generate_useful_gather_paths.
diff --git a/src/test/regress/expected/union.out b/src/test/regress/expected/union.out
index 75f78db8f5..cf7660f524 100644
--- a/src/test/regress/expected/union.out
+++ b/src/test/regress/expected/union.out
@@ -1420,3 +1420,55 @@ where (x = 0) or (q1 >= q2 and q1 <= q2);
  4567890123456789 |  4567890123456789 | 1
 (6 rows)
 
+-- Test handling of appendrel with different types which disables the path flattening and
+-- forces a subquery node. for the subquery node ensure the rowcounts are correc

Re: Parallel Inserts in CREATE TABLE AS

[Bharath Rupireddy]

Thanks for the comments.

How about naming like below more generically and placing them in
parallel.h so that it will also be used for refresh materialized view?

+typedef enum ParallelInsertTupleCostOpt
+{
+ PINS_SELECT_QUERY = 1 << 0, /* turn on this before planning */
+ /*
+ * Turn on this while planning for upper Gather path to ignore parallel
+ * tuple cost in cost_gather.
+ */
+ PINS_CAN_IGN_TUP_COST = 1 << 1,
+ /* Turn on this after the cost is ignored. */
+ PINS_TUP_COST_IGNORED = 1 << 2

My plan was to get the main design idea of pushing the dest receiver
to gather reviewed and once agreed, then I thought of making few
functions common and place them in parallel.h and parallel.c so that
they can be used for Parallel Inserts in REFRESH MATERIALIZED VIEW
because the same design idea can be applied there as well.

For instance my thoughts are: add the below structures, functions and
other macros to parallel.h and parallel.c:
typedef enum ParallelInsertKind
{
PINS_UNDEF = 0,
PINS_CREATE_TABLE_AS,
PINS_REFRESH_MAT_VIEW
} ParallelInsertKind;

typedef struct ParallelInsertCTASInfo
{
IntoClause *intoclause;
Oid objectid;
} ParallelInsertCTASInfo;

typedef struct ParallelInsertRMVInfo
{
Oid objectid;
} ParallelInsertRMVInfo;

ExecInitParallelPlan(PlanState *planstate, EState *estate,
  Bitmapset *sendParams, int nworkers,
- int64 tuples_needed)
+ int64 tuples_needed, ParallelInsertKind pinskind,
+ void *pinsinfo)

Change ExecParallelInsertInCTAS to

+static void
+ExecParallelInsert(GatherState *node)
+{

Change SetCTASParallelInsertState to
+void
+SetParallelInsertState(QueryDesc *queryDesc)

Change IsParallelInsertionAllowedInCTAS to

+bool
+IsParallelInsertionAllowed(ParallelInsertKind pinskind, IntoClause *into)
+{

Thoughts?

If okay, I can work on these points and add a new patch into the patch
set that will have changes for parallel inserts in REFRESH
MATERIALIZED VIEW.

On Wed, Dec 30, 2020 at 3:04 PM Dilip Kumar  wrote:
> Some comments in 0002
>
> 1.
> +/*
> + * Information sent to the planner from CTAS to account for the cost
> + * calculations in cost_gather. We need to do this because, no tuples will be
> + * received by the Gather node if the workers insert the tuples in parallel.
> + */
> +typedef enum CTASParallelInsertOpt
> +{
> + CTAS_PARALLEL_INS_UNDEF = 0, /* undefined */
> + CTAS_PARALLEL_INS_SELECT = 1 << 0, /* turn on this before planning */
> + /*
> + * Turn on this while planning for upper Gather path to ignore parallel
> + * tuple cost in cost_gather.
> + */
> + CTAS_PARALLEL_INS_TUP_COST_CAN_IGN = 1 << 1,
> + /* Turn on this after the cost is ignored. */
> + CTAS_PARALLEL_INS_TUP_COST_IGNORED = 1 << 2
> +} CTASParallelInsertOpt;
>
>
> I don't like the naming of these flags.  Especially no need to define
> CTAS_PARALLEL_INS_UNDEF, we can directl use 0
> for that purpose instead of giving some weird name.  So I suggest
> first, just get rid of CTAS_PARALLEL_INS_UNDEF.

+1. I will change it in the next version of the patch.

> 2.
> + /*
> + * Turn on a flag to ignore parallel tuple cost by the Gather path in
> + * cost_gather if the SELECT is for CTAS and we are generating an upper
> + * level Gather path.
> + */
> + bool ignore = ignore_parallel_tuple_cost(root);
> +
>   generate_useful_gather_paths(root, rel, false);
>
> + /*
> + * Reset the ignore flag, in case we turned it on but
> + * generate_useful_gather_paths returned without reaching cost_gather.
> + * If we reached cost_gather, we would have been reset it there.
> + */
> + if (ignore && (root->parse->CTASParallelInsInfo &
> + CTAS_PARALLEL_INS_TUP_COST_CAN_IGN))
> + {
> + root->parse->CTASParallelInsInfo &=
> + ~CTAS_PARALLEL_INS_TUP_COST_CAN_IGN;
> + }
>
> I think th way we are using these cost ignoring flag, doesn't look clean.
>
> I mean first, CTAS_PARALLEL_INS_SELECT is set if it is coming from
> CTAS and then ignore_parallel_tuple_cost will
> set the CTAS_PARALLEL_INS_TUP_COST_CAN_IGN if it satisfies certain
> condition which is fine.  Now, internally cost
> gather will add CTAS_PARALLEL_INS_TUP_COST_IGNORED and remove
> CTAS_PARALLEL_INS_TUP_COST_CAN_IGN and if
> CTAS_PARALLEL_INS_TUP_COST_CAN_IGN is not removed then we will remove
> it outside.  Why do we need to remove
> CTAS_PARALLEL_INS_TUP_COST_CAN_IGN flag at all?

Yes we don't need to remove the CTAS_PARALLEL_INS_TUP_COST_CAN_IGN
flag. I will change it in the next version.

> 3.
> + if (tuple_cost_flags && gstate->ps.ps_ProjInfo)
> + Assert(!(*tuple_cost_flags & CTAS_PARALLEL_INS_TUP_COST_IGNORED));
>
> Instead of adding Assert inside an IF statement, you can convert whole
> statement as an assert.  Lets not add unnecessary
> if in the release mode.

+1. I will change it in the version.

> 4.
> + if ((root->parse->CTASParallelInsInfo & CTAS_PARALLEL_INS_SELECT) &&
> + (root->parse->CTASParallelInsInfo &
> + CTAS_PARALLEL_INS_TUP_COST_CAN_IGN))
> + {
> + igno

Re: adding partitioned tables to publications

[????]

The first file of Amit's patch can not only re-range the code, but also fix a 
hidden bug.
To make it easy to see, I attach another patch.
"RelationIdGetRelation" will increase ref on owner->relrefarr, without 
"RelationClose", the owner->relrefarr will enlarge and re-hash.
When the capacity of owner->relrefarr is over than 10 million, enlarge and 
re-hash takes serial hours. And what's worse, increase ref will also take 
minutes, as the hash collision resolution is based on looking up an array in 
order.
When we want to publish 10 billion data under one partition table, it takes 
serial days up to increase ref, enlarge and re-hash, and CPU is always 99%. 
After applying my patch, 10 billion will be published in 10 minutes.





-- Original --
From:  "Amit Langote";https://www.postgresql.org/message-id/CA%2BHiwqFyydvQ5g%3Dqa54UM%2BXjm77BdhX-nM4dXQkNOgH%3DzvDjoA%40mail.gmail.com

To summarize:
1. Missing coverage for a couple of related blocks in
apply_handle_tuple_routing()
2. Missing coverage report for the code in pgoutput.c added by 83fd4532

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

0001-RelationClose-after-RelationIdGetRelation.patch
Description: Binary data

Re: dynamic result sets support in extended query protocol

[Peter Eisentraut]

On 2020-10-09 20:46, Andres Freund wrote:

Is there really a good reason for forcing the client to issue
NextResult, Describe, Execute for each of the dynamic result sets? It's
not like there's really a case for allowing the clients to skip them,
right?  Why aren't we sending something more like

S: CommandPartiallyComplete
S: RowDescription
S: DataRow...
S: CommandPartiallyComplete
S: RowDescription
S: DataRow...
...
S: CommandComplete
C: Sync


I want to post my current patch, to keep this discussion moving.  There 
are still a number of pieces to pull together, but what I have is a 
self-contained functioning prototype.


The interesting thing about the above message sequence is that the 
"CommandPartiallyComplete" isn't actually necessary.  Since an Execute 
message normally does not issue a RowDescription response, the 
appearance of one is already enough to mark the beginning of a new 
result set.  Moreover, libpq already handles this correctly, so we 
wouldn't need to change it at all.


We might still want to add a new protocol message, for clarity perhaps, 
and that would probably only be a few lines of code on either side, but 
that would only serve for additional error checking and wouldn't 
actually be needed to identify what's going on.


What else we need:

- Think about what should happen if the Execute message specifies a row 
count, and what should happen during subsequent Execute messages on the 
same portal.  I suspect that there isn't a particularly elegant answer, 
but we need to pick some behavior.


- Some way for psql to display multiple result sets. Proposals have been 
made in [0] and [1].  (You need either patch or one like it for the 
regression tests in this patch to pass.)


- Session-level default result formats setting, proposed in [2].  Not 
strictly necessary, but would be most sensible to coordinate these two.


- We don't have a way to test the extended query protocol.  I have 
attached my test program, but we might want to think about something 
more permanent.  Proposals for this have already been made in [3].


- Right now, this only supports returning dynamic result sets from a 
top-level CALL.  Specifications for passing dynamic result sets from one 
procedure to a calling procedure exist in the SQL standard and could be 
added later.


(All the SQL additions in this patch are per SQL standard.  DB2 appears 
to be the closest existing implementation.)



[0]:
https://www.postgresql.org/message-id/flat/4580ff7b-d610-eaeb-e06f-4d686896b93b%402ndquadrant.com
[1]: https://commitfest.postgresql.org/29/2096/
[2]: https://commitfest.postgresql.org/31/2812/
[3]: 
https://www.postgresql.org/message-id/4f733cca-5e07-e167-8b38-05b5c9066d04%402ndQuadrant.com


--
Peter Eisentraut
2ndQuadrant, an EDB company
https://www.2ndquadrant.com/
From 03e7b009ca54f686f0798c764ffc802e70f64076 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Wed, 30 Dec 2020 14:30:33 +0100
Subject: [PATCH v1] Dynamic result sets from procedures

Declaring a cursor WITH RETURN in a procedure makes the cursor's data be
returned as a result of the CALL invocation.  The procedure needs to
be declared with the DYNAMIC RESULT SETS attribute.
---
 doc/src/sgml/catalogs.sgml| 10 +++
 doc/src/sgml/information_schema.sgml  |  3 +-
 doc/src/sgml/protocol.sgml| 19 +
 doc/src/sgml/ref/alter_procedure.sgml | 12 +++
 doc/src/sgml/ref/create_procedure.sgml| 14 
 doc/src/sgml/ref/declare.sgml | 34 -
 src/backend/catalog/information_schema.sql|  2 +-
 src/backend/catalog/pg_aggregate.c|  3 +-
 src/backend/catalog/pg_proc.c |  4 +-
 src/backend/catalog/sql_features.txt  |  2 +-
 src/backend/commands/functioncmds.c   | 75 +--
 src/backend/commands/portalcmds.c | 23 ++
 src/backend/commands/typecmds.c   | 12 ++-
 src/backend/parser/gram.y | 20 -
 src/backend/tcop/postgres.c   | 62 ++-
 src/backend/tcop/pquery.c |  6 ++
 src/backend/utils/errcodes.txt|  1 +
 src/backend/utils/mmgr/portalmem.c| 48 
 src/bin/pg_dump/pg_dump.c | 16 +++-
 src/include/catalog/pg_proc.h |  6 +-
 src/include/commands/defrem.h |  2 +
 src/include/nodes/parsenodes.h|  9 ++-
 src/include/parser/kwlist.h   |  3 +
 src/include/utils/portal.h| 14 
 src/interfaces/libpq/fe-protocol3.c   |  6 +-
 .../regress/expected/create_procedure.out | 41 +-
 src/test/regress/sql/create_procedure.sql | 30 +++-
 27 files changed, 443 insertions(+), 34 deletions(-)

diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 3a2266526c..cde88b54e2 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -58

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

[Bharath Rupireddy]

On Wed, Dec 30, 2020 at 5:20 PM Alexey Kondratov
 wrote:
>
> On 2020-12-30 09:10, Bharath Rupireddy wrote:
> > Hi,
> >
> > I'm posting a v4-0001 patch for the new functions
> > postgres_fdw_get_connections() and postgres_fdw_disconnect().  In this
> > patch, I tried to address the review comments provided upthread.
> >
> > Thoughts?
> >
>
> I still have some doubts that it is worth of allowing to call
> postgres_fdw_disconnect() in the explicit transaction block, since it
> adds a lot of things to care and check for. But otherwise current logic
> looks solid.
>
> +errdetail("Such connections get closed 
> either in the next use or
> at the end of the current transaction.")
> +: errdetail("Such connection gets closed 
> either in the next use or
> at the end of the current transaction.")));
>
> Does it really have a chance to get closed on the next use? If foreign
> server is dropped then user mapping should be dropped as well (either
> with CASCADE or manually), but we do need user mapping for a local cache
> lookup. That way, if I understand all the discussion up-thread
> correctly, we can only close such connections at the end of xact, do we?

The next use of such a connection in the following query whose foreign
server would have been dropped fails because of the cascading that can
happen to drop the user mapping and the foreign table as well. During
the start of the next query such connection will be marked as
invalidated because xact_depth of that connection is > 1 and when the
fail happens, txn gets aborted due to which pgfdw_xact_callback gets
called and in that the connection gets closed. To make it more clear,
please have a look at the scenarios [1].

I still feel the detailed message "Such connections get closed either
in the next use or at the end of the current transaction" is
appropriate. Please have a closer look at the possible use cases [1].

And IMO, anyone dropping a foreign server inside an explicit txn block
in which the foreign server was used is extremely rare, so still
showing this message and allowing postgres_fdw_disconnect() in
explicit txn block is useful. For all other cases the
postgres_fdw_disconnect behaves as expected.

Thoughts?

[1]
case 1:
BEGIN;
SELECT 1 FROM f1 LIMIT 1;  --> xact_depth becomes 1
DROP SERVER loopback1 CASCADE; --> drop cascades to the user mapping
and the foreign table and the connection gets invalidated in
pgfdw_inval_callback because xact_depth is 1
SELECT 1 FROM f1 LIMIT 1; --> since the failure occurs for this query
and txn is aborted, the connection gets closed in pgfdw_xact_callback.
SELECT * FROM postgres_fdw_get_connections(); --> txn was aborted
SELECT * FROM postgres_fdw_disconnect(); --> txn was aborted
COMMIT;

case 2:
BEGIN;
SELECT 1 FROM f1 LIMIT 1;  --> xact_depth becomes 1
DROP SERVER loopback1 CASCADE; --> drop cascades to the user mapping
and the foreign table and the connection gets invalidated in
pgfdw_inval_callback because xact_depth is 1
SELECT * FROM postgres_fdw_get_connections(); --> shows the above
warning because foreign server name can not be fetched
SELECT * FROM postgres_fdw_disconnect(); --> the connection can not be
closed here as well because xact_depth is 1, then it issues a warning
"cannot close any connection because they are still in use"
COMMIT; --> finally the connection gets closed here in pgfdw_xact_callback.

case 3:
SELECT 1 FROM f1 LIMIT 1;
BEGIN;
DROP SERVER loopback1 CASCADE; --> drop cascades to the user mapping
and the foreign table and the connection gets closed in
pgfdw_inval_callback because xact_depth is 0
SELECT 1 FROM f1 LIMIT 1; --> since the failure occurs for this query
and the connection was closed previously then the txn gets aborted
SELECT * FROM postgres_fdw_get_connections(); --> txn was aborted
SELECT * FROM postgres_fdw_disconnect(); --> txn was aborted
COMMIT;

> + * This function returns false if the cache doesn't exist.
> + * When the cache exists:
>
> I think that this will be corrected later by pg_indent, but still. In
> this comment section following points 1) and 2) have a different
> combination of tabs/spaces.

I can change that in the next version.

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

Re: Multi Inserts in CREATE TABLE AS - revived patch

[Simon Riggs]

On Mon, 16 Nov 2020 at 15:32, Bharath Rupireddy
 wrote:
>
> On Mon, Nov 16, 2020 at 8:02 PM Paul Guo  wrote:
> >
> > > On Nov 13, 2020, at 7:21 PM, Bharath Rupireddy 
> > >  wrote:
> > >
> > > On Tue, Nov 10, 2020 at 3:47 PM Paul Guo  wrote:
> > >>
> > >> Thanks for doing this. There might be another solution - use raw insert 
> > >> interfaces (i.e. raw_heap_insert()).
> > >> Attached is the test (not formal) patch that verifies this idea. 
> > >> raw_heap_insert() writes the page into the
> > >> table files directly and also write the FPI xlog when the tuples filled 
> > >> up the whole page. This seems be
> > >> more efficient.
> > >>
> > >
> > > Thanks. Will the new raw_heap_insert() APIs scale well (i.e. extend
> > > the table parallelly) with parallelism? The existing
> > > table_multi_insert() API scales well, see, for instance, the benefit
> > > with parallel copy[1] and parallel multi inserts in CTAS[2].
> >
> > Yes definitely some work needs to be done to make raw heap insert 
> > interfaces fit the parallel work, but
> > it seems that there is no hard blocking issues for this?
> >
>
> I may be wrong here. If we were to allow raw heap insert APIs to
> handle parallelism, shouldn't we need some sort of shared memory to
> allow coordination among workers? If we do so, at the end, aren't
> these raw insert APIs equivalent to current table_multi_insert() API
> which uses a separate shared ring buffer(bulk insert state) for
> insertions?
>
> And can we think of these raw insert APIs similar to the behaviour of
> table_multi_insert() API for unlogged tables?

I found the additional performance of Paul Guo's work to be compelling
and the idea workable for very large loads.

Surely LockRelationForExtension() is all the inter-process
coordination we need to make this work for parallel loads?

-- 
Simon Riggshttp://www.EnterpriseDB.com/

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

[Stephen Frost]

Greetings,

* Paul Martinez (paul...@google.com) wrote:
> This small patch simply modifies the code to replace the if/else-if chain with
> separate if statements, and always checks whether the user has the CREATEROLE
> privilege. (The have_createrole_privilege function includes another superuser
> check.) Given the current checks in each branch, this code is equivalent, but
> easier to modify in the future.

While I do appreciate that it'd be nice if we made all of the code in
the backend easier for folks to maintain their own patch sets against
core, it'd also mean that we're now being asked to provide some level of
commitment that we aren't going to change these things later because
then we'd break $whomever's patches.  That's certainly not something
that is really reasonable for us to agree to.

I'd strongly suggest that, instead, you consider proposing changes which
would address the actual use cases you have and work with the community
to have those included in core, which would further have the added
property that everyone would then benefit from those improvements.

Thanks,

Stephen


signature.asc
Description: PGP signature

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

[Stephen Frost]

Greetings,

* Michael Paquier (mich...@paquier.xyz) wrote:
> On Tue, Dec 29, 2020 at 02:26:19PM -0600, Paul Martinez wrote:
> > The checks for whether the current user can create a user with the 
> > SUPERUSER,
> > REPLICATION, or BYPASSRLS attributes are chained together using if/else-if,
> > before finally checking whether the user has CREATEROLE privileges in a
> > final else. This construction is error-prone, since once one branch is 
> > visited,
> > later ones are skipped, and it implicitly assumes that the permissions 
> > needed
> > for each subsequent action are subsets of the permissions needed for the
> > previous action. Since each branch requires SUPERUSER this is fine, but
> > changing one of the checks could inadvertently allow users without the
> > CREATEROLE permission to create users.
> 
> Hmm.  I agree with your position here.  A careless change could easily
> miss that all those if branches have to use a superuser check.

That really strikes me as pretty darn unlikely to happen, it's not like
this code is really spread out across very many lines or that it's hard
to see the pretty clear pattern.

In thinking about these role attributes and the restrictions we have
about what attributes don't require superuser to set and what do, I do
feel like we're probably missing a bet regarding replication and
bypassrls..  In other words, I suspect people would be happier if we
provided a way for non-superusers a way to create replication roles and
bypassrls roles.  Exactly how we do that is a bit tricky to figure out
but that's certainly a much more productive direction to be going in.

Thanks,

Stephen


signature.asc
Description: PGP signature

Re: popcount

[Daniel Verite]

David Fetter wrote:

+Datum
+byteapopcount(PG_FUNCTION_ARGS)
+{
+   bytea   *t1 = PG_GETARG_BYTEA_PP(0);
+   int len, result;
+
+   len = VARSIZE_ANY_EXHDR(t1);
+   result = pg_popcount(VARDATA_ANY(t1), len);
+
+   PG_RETURN_INT32(result);
+}

The input may have more than 2 billion bits set to 1. The biggest possible
result should be 8 billion for bytea (1 GB with all bits set to 1).
So shouldn't this function return an int8?

There is a pre-existing function in core: bit_length(bytea) returning int4,
but it errors out for large values (in fact it's a SQL function that returns
octet_length($1)*8).

For the varbit case, it doesn't seem like it can hold so many bits, although
I don't see the limit documented in
https://www.postgresql.org/docs/current/datatype-bit.html


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: https://www.manitou-mail.org
Twitter: @DanielVerite

Re: popcount

[David Fetter]

On Wed, Dec 30, 2020 at 05:27:04PM +0100, Daniel Verite wrote:
>   David Fetter wrote:
> 
> +Datum
> +byteapopcount(PG_FUNCTION_ARGS)
> +{
> + bytea   *t1 = PG_GETARG_BYTEA_PP(0);
> + int len, result;
> +
> + len = VARSIZE_ANY_EXHDR(t1);
> + result = pg_popcount(VARDATA_ANY(t1), len);
> +
> + PG_RETURN_INT32(result);
> +}
> 
> The input may have more than 2 billion bits set to 1. The biggest possible
> result should be 8 billion for bytea (1 GB with all bits set to 1).
> So shouldn't this function return an int8?

It does now, and thanks for looking at this.

> There is a pre-existing function in core: bit_length(bytea) returning int4,
> but it errors out for large values (in fact it's a SQL function that returns
> octet_length($1)*8).
> 
> For the varbit case, it doesn't seem like it can hold so many bits, although
> I don't see the limit documented in
> https://www.postgresql.org/docs/current/datatype-bit.html

Out of an abundance of caution, I changed that one, too.

I'd noticed earlier that ceil() doesn't need to be quite as wasteful
as the way I had it earlier, and fixed that with help from Andrew
Gierth.

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
>From 6d35100b3bc16c1c7d3bf08c62589f3e039cee76 Mon Sep 17 00:00:00 2001
From: David Fetter 
Date: Wed, 30 Dec 2020 02:51:46 -0800
Subject: [PATCH v2] popcount
To: hackers
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="2.29.2"

This is a multi-part message in MIME format.
--2.29.2
Content-Type: text/plain; charset=UTF-8; format=fixed
Content-Transfer-Encoding: 8bit


Now it's accessible to SQL for the BIT VARYING and BYTEA types.

diff --git doc/src/sgml/func.sgml doc/src/sgml/func.sgml
index 5021ac1ca9..439a3a4a06 100644
--- doc/src/sgml/func.sgml
+++ doc/src/sgml/func.sgml
@@ -4069,6 +4069,23 @@ SELECT format('Testing %3$s, %2$s, %s', 'one', 'two', 'three');

   
 
+  
+   
+
+ popcount
+
+popcount ( bytes bytea )
+integer
+   
+   
+Counts the number of bits set in a binary string.
+   
+   
+popcount('\xdeadbeef'::bytea)
+24
+   
+  
+
   

 
@@ -4830,6 +4847,24 @@ SELECT format('Testing %3$s, %2$s, %s', 'one', 'two', 'three');
 101010001010101010

   
+
+  
+   
+
+ popcount
+
+popcount ( bits bit )
+int4
+   
+   
+Counts the bits set in a bit string.
+   
+   
+popcount(B'101010101010101010')
+9
+   
+  
+
  
 

diff --git src/include/catalog/pg_proc.dat src/include/catalog/pg_proc.dat
index 139f4a08bd..82e3d50114 100644
--- src/include/catalog/pg_proc.dat
+++ src/include/catalog/pg_proc.dat
@@ -1446,6 +1446,9 @@
 { oid => '752', descr => 'substitute portion of string',
   proname => 'overlay', prorettype => 'bytea',
   proargtypes => 'bytea bytea int4', prosrc => 'byteaoverlay_no_len' },
+{ oid => '8436', descr => 'count set bits',
+  proname => 'popcount', prorettype => 'int8', proargtypes => 'bytea',
+  prosrc => 'byteapopcount'},
 
 { oid => '725',
   proname => 'dist_pl', prorettype => 'float8', proargtypes => 'point line',
@@ -3865,6 +3868,9 @@
 { oid => '3033', descr => 'set bit',
   proname => 'set_bit', prorettype => 'bit', proargtypes => 'bit int4 int4',
   prosrc => 'bitsetbit' },
+{ oid => '8435', descr => 'count set bits',
+  proname => 'popcount', prorettype => 'int8', proargtypes => 'bit',
+  prosrc => 'bitpopcount'},
 
 # for macaddr type support
 { oid => '436', descr => 'I/O',
diff --git src/backend/utils/adt/varbit.c src/backend/utils/adt/varbit.c
index 3c03459f51..a71c2b6bf4 100644
--- src/backend/utils/adt/varbit.c
+++ src/backend/utils/adt/varbit.c
@@ -36,6 +36,7 @@
 #include "libpq/pqformat.h"
 #include "nodes/nodeFuncs.h"
 #include "nodes/supportnodes.h"
+#include "port/pg_bitutils.h"
 #include "utils/array.h"
 #include "utils/builtins.h"
 #include "utils/varbit.h"
@@ -1872,3 +1873,29 @@ bitgetbit(PG_FUNCTION_ARGS)
 	else
 		PG_RETURN_INT32(0);
 }
+
+/*
+ * bitpopcount
+ *
+ * Returns the number of bits set in a bit array.
+ *
+ */
+Datum
+bitpopcount(PG_FUNCTION_ARGS)
+{
+	VarBit		*arg1 = PG_GETARG_VARBIT_P(0);
+	bits8		*p;
+	int			len;
+	int8		popcount;
+
+	/*
+	 * ceil(VARBITLEN(ARG1)/BITS_PER_BYTE)
+	 * done to minimize branches and instructions.
+	 */
+	len = (VARBITLEN(arg1) + BITS_PER_BYTE + 1) / BITS_PER_BYTE;
+	p = VARBITS(arg1);
+
+	popcount = pg_popcount((const char *)p, len);
+
+	PG_RETURN_INT64(popcount);
+}
diff --git src/backend/utils/adt/varlena.c src/backend/utils/adt/varlena.c
index 9300d19e0c..9c5e5b5592 100644
--- src/backend/utils/adt/varlena.c
+++ src/backend/utils/adt/varlena.c
@@ -3415,6 +3415,22 @@ bytea_overlay(bytea *t1, bytea *t2, int sp, int sl)
 	return result

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

[Alexey Kondratov]

On 2020-12-30 17:59, Bharath Rupireddy wrote:

On Wed, Dec 30, 2020 at 5:20 PM Alexey Kondratov
 wrote:


On 2020-12-30 09:10, Bharath Rupireddy wrote:
I still have some doubts that it is worth of allowing to call
postgres_fdw_disconnect() in the explicit transaction block, since it
adds a lot of things to care and check for. But otherwise current 
logic

looks solid.

+errdetail("Such connections get 
closed either in the next use or

at the end of the current transaction.")
+: errdetail("Such connection gets 
closed either in the next use or

at the end of the current transaction.")));

Does it really have a chance to get closed on the next use? If foreign
server is dropped then user mapping should be dropped as well (either
with CASCADE or manually), but we do need user mapping for a local 
cache

lookup. That way, if I understand all the discussion up-thread
correctly, we can only close such connections at the end of xact, do 
we?


The next use of such a connection in the following query whose foreign
server would have been dropped fails because of the cascading that can
happen to drop the user mapping and the foreign table as well. During
the start of the next query such connection will be marked as
invalidated because xact_depth of that connection is > 1 and when the
fail happens, txn gets aborted due to which pgfdw_xact_callback gets
called and in that the connection gets closed. To make it more clear,
please have a look at the scenarios [1].



In my understanding 'connection gets closed either in the next use' 
means that connection will be closed next time someone will try to use 
it, i.e. GetConnection() will be called and it closes this connection 
because of a bad state. However, if foreign server is dropped 
GetConnection() cannot lookup the connection because it needs a user 
mapping oid as a key.


I had a look on your scenarios. IIUC, under **next use** you mean a 
select attempt from a table belonging to the same foreign server, which 
leads to a transaction abort and connection gets closed in the xact 
callback. Sorry, maybe I am missing something, but this just confirms 
that such connections only get closed in the xact callback (taking into 
account your recently committed patch [1]), so 'next use' looks 
misleading.


[1] 
https://www.postgresql.org/message-id/8b2aa1aa-c638-12a8-cb56-ea0f0a5019cf%40oss.nttdata.com



Regards
--
Alexey Kondratov

Postgres Professional https://www.postgrespro.com
Russian Postgres Company

Re: psql \df choose functions by their arguments

[Greg Sabino Mullane]

Attached is the latest patch against HEAD - basically fixes a few typos.

Cheers,
Greg


v3-psql-df-pick-function-by-type.patch
Description: Binary data

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

[Andrey Borodin]

> 30 дек. 2020 г., в 20:26, Stephen Frost  написал(а):
> 
> I'd strongly suggest that, instead, you consider proposing changes which
> would address the actual use cases you have and work with the community
> to have those included in core, which would further have the added
> property that everyone would then benefit from those improvements.
+1. Last time we asked to change something in privileges[0], we got a feedback 
pointing to possible vulnerability.
We fixed it in our services and reported to, AFAIR, RDS and Aiven (with PoC 
exploits).

I think that sharing "various small changes to permission checks" is a really 
good idea.

> 30 дек. 2020 г., в 20:39, Stephen Frost  написал(а):
> In other words, I suspect people would be happier if we
> provided a way for non-superusers a way to create replication roles and
> bypassrls roles.
+1 again. I hope we will return to the topic soon.

Best regards, Andrey Borodin.

[0] 
https://www.postgresql.org/message-id/flat/1269681541151271%40myt5-68ad52a76c91.qloud-c.yandex.net

Re: [HACKERS] [PATCH] Generic type subscripting

[Pavel Stehule]

st 30. 12. 2020 v 14:46 odesílatel Dmitry Dolgov <9erthali...@gmail.com>
napsal:

> > On Wed, Dec 30, 2020 at 02:45:12PM +0100, Dmitry Dolgov wrote:
> > > On Sat, Dec 26, 2020 at 01:24:04PM -0500, Tom Lane wrote:
> > >
> > > In a case like jsonpath['...'], the initially UNKNOWN-type literal
> could
> > > in theory be coerced to any of these types, so you'd have to resolve
> that
> > > case manually.  The overloaded-function code has an internal preference
> > > that makes it choose TEXT if it has a choice of TEXT or some other
> target
> > > type for an UNKNOWN input (cf parse_func.c starting about line 1150),
> but
> > > if you ask can_coerce_type() it's going to say TRUE for all three
> cases.
> > >
> > > Roughly speaking, then, I think what you want to do is
> > >
> > > 1. If input type is UNKNOWNOID, choose result type TEXT.
> > >
> > > 2. Otherwise, apply can_coerce_type() to see if the input type can be
> > > coerced to int4, text, or jsonpath.  If it succeeds for none or more
> > > than one of these, throw error.  Otherwise choose the single successful
> > > type.
> > >
> > > 3. Apply coerce_type() to coerce to the chosen result type.
> > >
> > > 4. At runtime, examine exprType() of the input to figure out what to
> do.
> >
> > Thanks, that was super useful. Following this suggestion I've made
> > necessary adjustments for the patch. There is no jsonpath support, but
> > this could be easily added on top.
>
> And the forgotten patch itself.
>

make check fails

But I dislike two issues

1. quietly ignored update

postgres=# update foo set a['a'][10] = '20';
UPDATE 1
postgres=# select * from foo;
┌┐
│ a  │
╞╡
│ {} │
└┘
(1 row)

The value should be modified or there should be an error (but I prefer
implicit creating nested empty objects when it is necessary).

update foo set a['a'] = '[]';

2. The index position was ignored.

postgres=# update foo set a['a'][10] = '20';
UPDATE 1
postgres=# select * from foo;
┌─┐
│  a  │
╞═╡
│ {"a": [20]} │
└─┘
(1 row)

Notes:

1. It is very nice so casts are supported. I wrote int2jsonb cast and it
was working. Maybe we can create buildin casts for int, bigint, numeric,
boolean, date, timestamp to jsonb.

Regards

Pavel

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

[David Fetter]

On Mon, Sep 07, 2020 at 11:57:58AM +0900, Michael Paquier wrote:
> On Thu, Jul 02, 2020 at 04:02:21PM +0200, Daniel Gustafsson wrote:
> > The CF Patch Tester consider this patch to be malformed and is unable to 
> > apply
> > and test it.  Can you please submit a rebased version?
> 
> 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..
> 
> The patch has stalled for two months now without a rebase provided, so
> I am marking it as returned with feedback.

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.

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: Let people set host(no)ssl settings from initdb

[David Fetter]

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:
> > On Thu, Jul 02, 2020 at 04:02:21PM +0200, Daniel Gustafsson wrote:
> > > The CF Patch Tester consider this patch to be malformed and is unable to 
> > > apply
> > > and test it.  Can you please submit a rebased version?
> > 
> > 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..
> > 
> > The patch has stalled for two months now without a rebase provided, so
> > I am marking it as returned with feedback.
> 
> 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.

*sigh*

This time with patch actually attached.

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
>From 537e58136890d17d33699e9965d0518d4c8a1822 Mon Sep 17 00:00:00 2001
From: David Fetter 
Date: Wed, 11 Dec 2019 18:55:03 -0800
Subject: [PATCH v5] Enable setting pg_hba.conf permissions from initdb
To: hackers
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="2.29.2"

This is a multi-part message in MIME format.
--2.29.2
Content-Type: text/plain; charset=UTF-8; format=fixed
Content-Transfer-Encoding: 8bit


The new optional arguments are --auth-hostssl, --auth-hostnossl,
--auth-hostgssenc, and --auth-hostnogssenc.

diff --git doc/src/sgml/ref/initdb.sgml doc/src/sgml/ref/initdb.sgml
index 385ac25150..c63e2c316e 100644
--- doc/src/sgml/ref/initdb.sgml
+++ doc/src/sgml/ref/initdb.sgml
@@ -152,6 +152,50 @@ PostgreSQL documentation
   
  
 
+ 
+  --auth-hostssl=authmethod
+  
+   
+This option specifies the authentication method for users via
+TLS remote connections used in pg_hba.conf
+(hostssl lines).
+   
+  
+ 
+
+ 
+  --auth-hostnossl=authmethod
+  
+   
+This option specifies the authentication method for users via
+non-TLS remote connections used in pg_hba.conf
+(hostnossl lines).
+   
+  
+ 
+
+ 
+  --auth-hostgssenc=authmethod
+  
+   
+This option specifies the authentication method for users via
+encrypted GSSAPI remote connections used in pg_hba.conf
+(hostgssenc lines).
+   
+  
+ 
+
+ 
+  --auth-hostnogssenc=authmethod
+  
+   
+This option specifies the authentication method for users via
+remote connections not using encrypted GSSAPI in pg_hba.conf
+(hostnogssenc lines).
+   
+  
+ 
+
  
   --auth-local=authmethod
   
diff --git src/backend/libpq/pg_hba.conf.sample src/backend/libpq/pg_hba.conf.sample
index b6de12b298..e132cf4db3 100644
--- src/backend/libpq/pg_hba.conf.sample
+++ src/backend/libpq/pg_hba.conf.sample
@@ -91,3 +91,15 @@ hostall all ::1/128 @authmethodhost@
 @remove-line-for-nolocal@local   replication all @authmethodlocal@
 hostreplication all 127.0.0.1/32@authmethodhost@
 hostreplication all ::1/128 @authmethodhost@
+
+# hostnossl all   all 0.0.0.0/0   @authmethodhostnossl@
+# hostnossl all   all ::/0@authmethodhostnossl@
+
+# hostssl all all 0.0.0.0/0   @authmethodhostssl@
+# hostssl all all ::/0@authmethodhostssl@
+
+# hostnogssenc all   all 0.0.0.0/0   @authmethodhostnogssenc@
+# hostnogssenc all   all ::/0@authmethodhostnogssenc@
+
+# hostgssenc all all 0.0.0.0/0   @authmethodhostgssenc@
+# hostgssenc all all ::/0@authmethodhostgssenc@
diff --git src/bin/initdb/initdb.c src/bin/initdb/initdb.c
index 0865f73ee0..ddb5cc5c9c 100644
--- src/bin/initdb/initdb.c
+++ src/bin/initdb/initdb.c
@@ -137,6 +137,10 @@ stati

Re: Implement for window functions

[Krasiyan Andreev]

Hi, after latest committed patches about multirange datatypes, I get a
compilation error,
when I try to apply a patch about respect/ignore null for window functions.
Without it applied, it complies clean and all checks are passed.

[krasiyan@localhost build]$ /home/krasiyan/pgsql/postgresql/configure
--with-openssl --with-libxml --with-libxslt --with-systemd --with-selinux
--with-perl --with-python --enable-cassert --prefix=/var/lib/pgsql/14
...
gcc -Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Werror=vla -Wendif-labels
-Wmissing-format-attribute -Wimplicit-fallthrough=3 -Wcast-function-type
-Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard
-Wno-format-truncation -Wno-stringop-truncation -O2 -I../../../src/include
-I/home/krasiyan/pgsql/postgresql/src/include  -D_GNU_SOURCE
-I/usr/include/libxml2   -c -o typecmds.o
/home/krasiyan/pgsql/postgresql/src/backend/commands/typecmds.c
/home/krasiyan/pgsql/postgresql/src/backend/commands/typecmds.c: In
function ‘makeMultirangeConstructors’:
/home/krasiyan/pgsql/postgresql/src/backend/commands/typecmds.c:1849:9:
warning: passing argument 17 of ‘ProcedureCreate’ makes integer from
pointer without a cast [-Wint-conversion]
 1849 | argtypes, /* parameterTypes */
  | ^~~~
  | |
  | oidvector *
In file included from
/home/krasiyan/pgsql/postgresql/src/backend/commands/typecmds.c:52:
/home/krasiyan/pgsql/postgresql/src/include/catalog/pg_proc.h:206:16: note:
expected ‘char’ but argument is of type ‘oidvector *’
  206 |   char parallel,
  |   ~^~~~
In file included from
/home/krasiyan/pgsql/postgresql/src/backend/commands/typecmds.c:32:
/home/krasiyan/pgsql/postgresql/src/include/postgres.h:556:29: warning:
passing argument 18 of ‘ProcedureCreate’ makes pointer from integer without
a cast [-Wint-conversion]
  556 | #define PointerGetDatum(X) ((Datum) (X))
  |~^~~~
  | |
  | long unsigned int
/home/krasiyan/pgsql/postgresql/src/backend/commands/typecmds.c:1850:9:
note: in expansion of macro ‘PointerGetDatum’
 1850 | PointerGetDatum(NULL), /* allParameterTypes */
  | ^~~
In file included from
/home/krasiyan/pgsql/postgresql/src/backend/commands/typecmds.c:52:
/home/krasiyan/pgsql/postgresql/src/include/catalog/pg_proc.h:207:22: note:
expected ‘oidvector *’ but argument is of type ‘long unsigned int’
  207 |   oidvector *parameterTypes,
  |   ~~~^~
In file included from
/home/krasiyan/pgsql/postgresql/src/include/access/tupdesc.h:19,
 from
/home/krasiyan/pgsql/postgresql/src/include/utils/relcache.h:18,
 from
/home/krasiyan/pgsql/postgresql/src/include/access/genam.h:21,
 from
/home/krasiyan/pgsql/postgresql/src/backend/commands/typecmds.c:34:
/home/krasiyan/pgsql/postgresql/src/include/nodes/pg_list.h:65:19: warning:
passing argument 21 of ‘ProcedureCreate’ makes integer from pointer without
a cast [-Wint-conversion]
   65 | #define NIL  ((List *) NULL)
  |  ~^~
  |   |
  |   List *
/home/krasiyan/pgsql/postgresql/src/backend/commands/typecmds.c:1853:9:
note: in expansion of macro ‘NIL’
 1853 | NIL, /* parameterDefaults */
  | ^~~
In file included from
/home/krasiyan/pgsql/postgresql/src/backend/commands/typecmds.c:52:
/home/krasiyan/pgsql/postgresql/src/include/catalog/pg_proc.h:210:17: note:
expected ‘Datum’ {aka ‘long unsigned int’} but argument is of type ‘List *’
  210 |   Datum parameterNames,
  |   ~~^~
In file included from
/home/krasiyan/pgsql/postgresql/src/backend/commands/typecmds.c:32:
/home/krasiyan/pgsql/postgresql/src/include/postgres.h:556:29: warning:
passing argument 22 of ‘ProcedureCreate’ makes pointer from integer without
a cast [-Wint-conversion]
  556 | #define PointerGetDatum(X) ((Datum) (X))
  |~^~~~
  | |
  | long unsigned int
/home/krasiyan/pgsql/postgresql/src/backend/commands/typecmds.c:1854:9:
note: in expansion of macro ‘PointerGetDatum’
 1854 | PointerGetDatum(NULL), /* trftypes */
  | ^~~
In file included from
/home/krasiyan/pgsql/postgresql/src/backend/commands/typecmds.c:52:
/home/krasiyan/pgsql/postgresql/src/include/catalog/pg_proc.h:211:17: note:
expected ‘List *’ but argument is of type ‘long unsigned int’
  211 |   List *parameterDefaults,
  |   ~~^
/home/krasiyan/pgsql/postgresql/src/backend/commands/typecmds.c:1833:11:
error: too few arguments to function ‘ProcedureCreate’
 1833 |  myself = ProcedureCreate(name, /* name: same as multirange type */
  |   ^~~
In

Re: Implement for window functions

[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
>From 7c8ae2cac7b2157f48e9e15c863eccde29a539b4 Mon Sep 17 00:00:00 2001
From: David Fetter 
Date: Tue, 22 Dec 2020 19:47:35 -0800
Subject: [PATCH v2] Vik's  patch
To: hackers
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="2.29.2"

This is a multi-part message in MIME format.
--2.29.2
Content-Type: text/plain; charset=UTF-8; format=fixed
Content-Transfer-Encoding: 8bit


diff --git doc/src/sgml/func.sgml doc/src/sgml/func.sgml
index 5021ac1ca9..0e877c48df 100644
--- doc/src/sgml/func.sgml
+++ 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 doc/src/sgml/ref/create_function.sgml doc/src/sgml/ref/create_function.sgml
index 3c1eaea651..31e08c26b4 100644
--- doc/src/sgml/ref/create_function.sgml
+++ 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 doc/src/sgml/syntax.sgml doc/src/sgml/syntax.sgml
index d66560b587..103595d21b 100644
--- doc/src/sgml/syntax.sgml
+++ 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 src/include/catalog/pg_proc.dat src/include/catalog/pg_proc.dat
index 139f4a08bd..50c7b47be2 100644
--- src/include/catalog/pg_proc.dat
+++ src/include/catalog/pg_proc.dat
@@ -9770,33 +9770,42 @@
   proargtypes => 'int4', prosrc => 'window_ntile' },
 { oid => '3106', descr => 'fetch the preceding row value',
   proname => 'lag', prokind => 'w', prorettype => 'anyelement',
-  proargtypes => 'anyelement', prosrc => 'window_lag' },
+  proargtypes => 'anyelement', prosrc => 'window_lag',
+  pronulltreatment => 't' },
 { oid => '3107', descr => 'fetch the Nth preceding row value',
   proname => 'lag', prokind => 'w', prorettype => 'anyelement',
-  proargtypes => 'anyelement int4', prosrc => 'window_lag_with_offset' },
+  proargtypes => 'anyelement int4'

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

[Tom Lane]

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.

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
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.)

regards, tom lane

Re: [HACKERS] [PATCH] Generic type subscripting

[Dmitry Dolgov]

> On Wed, Dec 30, 2020 at 07:48:57PM +0100, Pavel Stehule wrote:
> st 30. 12. 2020 v 14:46 odesílatel Dmitry Dolgov <9erthali...@gmail.com>
> napsal:
>
> > > On Wed, Dec 30, 2020 at 02:45:12PM +0100, Dmitry Dolgov wrote:
> > > > On Sat, Dec 26, 2020 at 01:24:04PM -0500, Tom Lane wrote:
> > > >
> > > > In a case like jsonpath['...'], the initially UNKNOWN-type literal
> > could
> > > > in theory be coerced to any of these types, so you'd have to resolve
> > that
> > > > case manually.  The overloaded-function code has an internal preference
> > > > that makes it choose TEXT if it has a choice of TEXT or some other
> > target
> > > > type for an UNKNOWN input (cf parse_func.c starting about line 1150),
> > but
> > > > if you ask can_coerce_type() it's going to say TRUE for all three
> > cases.
> > > >
> > > > Roughly speaking, then, I think what you want to do is
> > > >
> > > > 1. If input type is UNKNOWNOID, choose result type TEXT.
> > > >
> > > > 2. Otherwise, apply can_coerce_type() to see if the input type can be
> > > > coerced to int4, text, or jsonpath.  If it succeeds for none or more
> > > > than one of these, throw error.  Otherwise choose the single successful
> > > > type.
> > > >
> > > > 3. Apply coerce_type() to coerce to the chosen result type.
> > > >
> > > > 4. At runtime, examine exprType() of the input to figure out what to
> > do.
> > >
> > > Thanks, that was super useful. Following this suggestion I've made
> > > necessary adjustments for the patch. There is no jsonpath support, but
> > > this could be easily added on top.
> >
> > And the forgotten patch itself.
> >
>
> make check fails

Yeah, apparently I forgot to enable asserts back after the last
benchmarking discussion, and missed some of those. Will fix.

> 2. The index position was ignored.
>
> postgres=# update foo set a['a'][10] = '20';
> UPDATE 1
> postgres=# select * from foo;
> ┌─┐
> │  a  │
> ╞═╡
> │ {"a": [20]} │
> └─┘
> (1 row)

I just realized I haven't included "filling the gaps" part, that's why
it works as before. Can add this too.

> 1. quietly ignored update
>
> postgres=# update foo set a['a'][10] = '20';
> UPDATE 1
> postgres=# select * from foo;
> ┌┐
> │ a  │
> ╞╡
> │ {} │
> └┘
> (1 row)

This belongs to the original jsonb_set implementation. Although if we
started to change it anyway with "filling the gaps", maybe it's fine to
add one more flag to tune its behaviour in this case as well. I can
check how complicated that could be.

Re: Implement for window functions

[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: Let people set host(no)ssl settings from initdb

[Isaac Morland]

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


> 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.


I can't speak to other distributions, but on Ubuntu pg_createcluster allows
a -- followed by initdb options. So at least on Ubuntu any additional
options will indeed be available to everybody. I would hope that other
distributions have the same capability.

I for one would like to be able to tell initdb (pg_createcluster) what to
put in the first column of pb_hba.conf in the same way I can already use
--auth{,-host,-local}= to set the auth-method column. Ideally, for simple
situations (think testing scripts and the like, rather than long-term
installations) the pg_hba.conf could be created by initdb and not changed
after that.

Buildfarm's cross-version-upgrade tests

[Tom Lane]

I broke $SUBJECT again today by removing regress_putenv from
regress.so.  I don't really want to put it back, but that means
we need a way to drop the corresponding SQL function from the
old version's regression database before attempting an upgrade.

Since I'd just seen Noah's commit 52202bb39 go by, I tried
modifying src/bin/pg_upgrade/test.sh to do the drop, but that
isn't helping.  I recall now from prior discussions that we
have to modify code that's embedded in the buildfarm client
script to make this go.  (I wonder what test scenario Noah had
in mind, exactly.)

Realizing that this has happened before and will happen again,
I'm now wondering if we can't devise a fixup method that is
controllable by committers and doesn't require a buildfarm
client rollout to update.  I'm thinking maybe we could extract
the fixup logic from test.sh into a standalone SQL script that's
part of the regular source tree so it can be updated by committers,
and then both test.sh and the buildfarm script could invoke it.
Maybe that won't work for some reason, but I'd sure like to find
some way of handling this without making you get involved every time.

regards, tom lane

crash recovery vs partially written WAL

[Andres Freund]

Hi,

A question from a colleague made me wonder if there are scenarios where
two subsequent crashes could lead to wrong WAL to be applied.

Imagine the following scenario
[ xlog page 1 ][ xlog page 2 ][ xlog page 3 ][ xlog page 4 ]
^flush^write ^insert

if the machine crashes in this moment, we could end up with a situation
where page 1, 3, 4 made it out out to disk, but page 2 wasn't.

That itself is not a problem, when we perform crash recovery, we'll
detect the end of WAL. We'll zero out the invalid parts of page 2, and
log a end-of-recovery checkpoint (which has to fit either onto page 2 or
3).

What I am concerned about is what happens if after crash recovery we
fill up page 3 with new valid records, ending exactly at the page
boundary (i.e. .

[ xlog page 1 ][ xlog page 2 ][ xlog page 3 ][ xlog page 4 ]
 ^(flush,write)
  ^insert

if we crash now, we'll peform recovery from the end-fo-recovery record
somewhere on page 2 or 3, and replay the rest of page 3.


That's where I see/wonder about a problem: What guarantees that we find
the contents of xlog page 4 to be invalid? The page header will have the
appropriate xl_pageaddr/tli/info. and because the last record on page 3
ended precisely at the page boundary, there'll not be a xlp_rem_len
allowing us to detect this either.

While we zero out WAL pages in-memory before using them, this won't help
in this instance because a) nothing was inserted into page 4 b) page 4
was never written out.


WAL segment recycling doesn't cause similar problems because xlp_pageaddr
protects us against related issues.


Replaying the old records from page 4 is obviously wrong, since they may
rely on modifications the "old" records on page 2/3 would have performed
(but which got lost).



I don't immediately see a good fix for this. The most obvious thing
would be to explicitly zero-out all WAL files beyond the end-of-recovery
point that have a "correct" xlp_pageaddr, but that may reading a lot of
WAL due to WAL file recycling.


I hope I am missing some crosscheck making this a non-issue?


Greetings,

Andres Freund

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

[Paul Martinez]

On Wed, Dec 30, 2020 at 12:28 PM Andrey Borodin  wrote:
> I think that sharing "various small changes to permission checks" is a really 
> good idea.
>
> > 30 дек. 2020 г., в 20:39, Stephen Frost  написал(а):
> > In other words, I suspect people would be happier if we
> > provided a way for non-superusers a way to create replication roles and
> > bypassrls roles.
> +1 again. I hope we will return to the topic soon.

On Wed, Dec 30, 2020 at 9:26 AM Stephen Frost  wrote:
> While I do appreciate that it'd be nice if we made all of the code in
> the backend easier for folks to maintain their own patch sets against
> core, it'd also mean that we're now being asked to provide some level of
> commitment that we aren't going to change these things later because
> then we'd break $whomever's patches.  That's certainly not something
> that is really reasonable for us to agree to.
>
> I'd strongly suggest that, instead, you consider proposing changes which
> would address the actual use cases you have and work with the community
> to have those included in core, which would further have the added
> property that everyone would then benefit from those improvements.

On Wed, Dec 30, 2020 at 9:39 AM Stephen Frost  wrote:
>
> That really strikes me as pretty darn unlikely to happen, it's not like
> this code is really spread out across very many lines or that it's hard
> to see the pretty clear pattern.
>
> In thinking about these role attributes and the restrictions we have
> about what attributes don't require superuser to set and what do, I do
> feel like we're probably missing a bet regarding replication and
> bypassrls..  In other words, I suspect people would be happier if we
> provided a way for non-superusers a way to create replication roles and
> bypassrls roles.  Exactly how we do that is a bit tricky to figure out
> but that's certainly a much more productive direction to be going in.

Thanks everyone for taking a look!

You've identified exactly the problem we're running into -- we want to
allow customers, who aren't superusers, to create replication roles.

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())".

That was actually what I initially tried to do when modifying this bit of
code:

  if (issuperuser)
if (!superuser()) ereport(...);
  elsif (isreplication)
-   if (!superuser()) ereport(...);
+   if (!superuser() && !cloudsqlsuperuser()) ereport(...);
  elsif (bypassrls)
if (!superuser()) ereport(...);
  else
if (!have_createrole_privilege()) ereport()

But this inadvertently allowed cloudsqlsuperuser to _also_ create users
with the BYPASSRLS attribute by creating a user with both REPLICATION
and BYPASSRLS, which I only realized when a couple of tests failed.
Properly modifying the required permissions required separating the
if/else branches, which led to this patch.

>From my understanding, AWS RDS Postgres takes a slightly different approach
and allows the REPLICATION attribute to be inherited through membership in
a special rds_replication role.



We haven't seriously considered what some sort of general functionality
would look like to support our managed Postgres use-cases, but here's a
rough sketch of what we're trying to accomplish with roles and permissions:

- We, ideally, want to give our customers access to as much of Postgres'
  functionality as possible, including SUPERUSER capabilities

- But we do not want customers to have access to the underlying VM or
  filesystem

- There are certain parts of the system (i.e., certain databases, tables,
  individual rows in catalog tables, etc.) that we want to manage and we
  can't allow customers to modify these at all. Examples include:
  - Our own SUPERUSER role that we use to connect to the cluster; customers
shouldn't be able to modify this role at all
  - Replication slots used for managed replication shouldn't be able to be
modified by customers (even if they have the REPLICATION attribute)

- We want to prevent customers from depending on any data we choose to store
  in other database, as that limits our flexibility to make future changes
  - Notably this means we only can support logical replication, and not
physical replication.

I suppose this could be roughly summarized as "90% of a superuser, but also
still obeys SQL privileges for objects created by someone else".

We would definitely be happy to explore what sort of functionality like this
would be useful for others!

Re: [PATCH] Allow queries in WHEN expression of FOR EACH STATEMENT triggers

[Joe Wildish]

Hi Surafel,

On 3 Sep 2020, at 19:22, Surafel Temesgen wrote:


This is my review of your patch


Thanks for the review.


subqueries in row trigger's is not supported in your patch so the the
documentation have to reflect it


It is still the case that the documentation says this. But, that may 
have been unclear as the documentation wouldn't compile (as you noted), 
so it wasn't possible to read it in the rendered form.




+ UPDATE triggers are able to refer to both
OLD

+ and NEW

Opening and ending tag mismatch on UPDATE and OLD literal so 
documentation

build fails and please update the documentation on server programming
section too


Fixed.

I've also amended the server programming section to accurately reflect 
how WHEN conditions can be used.


Instead of planning every time the trigger fire I suggest to store 
plan or

prepared statement node so planning time can be saved


Yes, that would make sense. I'll look in to what needs to be done.

Do you know if there are other areas of the code that cache plans that 
could act as a guide as to how best to achieve it?



There are server crash on the following sequence of command

...

INSERT INTO main_table (a, b) VALUES

(101, 498),

(102, 499);

server crashed


Thanks. It was an incorrect Assert about NULL returns. Fixed.

-Joe

From 56d010c925db41ffe689044ba215640600976748 Mon Sep 17 00:00:00 2001
From: Joe Wildish 
Date: Wed, 30 Dec 2020 19:20:10 +
Subject: [PATCH] Allow queries in WHEN expression of FOR EACH STATEMENT
 triggers

Adds support to the trigger system to allow queries in the WHEN condition
of FOR EACH STATEMENT triggers. The expression can contain references to
the transition tables NEW and OLD, as well as the table which the
trigger is attached to, but other table references are disallowed.
---
 doc/src/sgml/ref/create_trigger.sgml   |  45 +-
 doc/src/sgml/trigger.sgml  |   7 +-
 src/backend/commands/trigger.c | 597 -
 src/backend/parser/parse_expr.c|   4 +-
 src/backend/utils/adt/ruleutils.c  |  94 ++--
 src/include/nodes/execnodes.h  |   2 +-
 src/test/regress/expected/triggers.out |  73 ++-
 src/test/regress/sql/triggers.sql  |  63 +++
 8 files changed, 699 insertions(+), 186 deletions(-)

diff --git a/doc/src/sgml/ref/create_trigger.sgml 
b/doc/src/sgml/ref/create_trigger.sgml
index 561af989a4..47f9a65fe4 100644
--- a/doc/src/sgml/ref/create_trigger.sgml
+++ b/doc/src/sgml/ref/create_trigger.sgml
@@ -160,13 +160,14 @@ CREATE [ OR REPLACE ] [ CONSTRAINT ] TRIGGER name
   
 
   
-   Also, a trigger definition can specify a Boolean WHEN
-   condition, which will be tested to see whether the trigger should
+   A trigger definition can specify a Boolean WHEN
+   condition which will be tested to see whether the trigger should
be fired.  In row-level triggers the WHEN condition can
-   examine the old and/or new values of columns of the row.  Statement-level
-   triggers can also have WHEN conditions, although the 
feature
-   is not so useful for them since the condition cannot refer to any values
-   in the table.
+   examine the old and/or new values of the columns of each row  which the
+   statement affects.  Statement-level triggers can also have
+   WHEN conditions, and are able to examine old and/or new
+   transition relations, that comprise of all rows either deleted or inserted
+   respectively by the triggering statement.
   
 
   
@@ -375,23 +376,41 @@ UPDATE OF column_name1 [, 
column_name2WHEN is specified, the
   function will only be called if the condition returns 
true.
-  In FOR EACH ROW triggers, the WHEN
+ 
+
+ 
+  In FOR EACH ROW triggers the WHEN
   condition can refer to columns of the old and/or new row values
   by writing OLD.column_name or
   NEW.column_name respectively.
-  Of course, INSERT triggers cannot refer to 
OLD
-  and DELETE triggers cannot refer to 
NEW.
+  The WHEN expression of a FOR EACH 
ROW
+  trigger cannot contain a subquery.
  
 
- INSTEAD OF triggers do not support 
WHEN
-  conditions.
+ 
+  In FOR EACH STATEMENT triggers the
+  WHEN condition can refer to the transition relations
+  OLD and NEW, and the relation that 
the
+  trigger is for. No other relations can be referenced. As 
OLD
+  and NEW are relations rather than row values, a
+  condition will typically 
comprise of
+  subquery expressions defined over those relations. Refer to
+   for subquery expression examples.
  
 
  
-  Currently, WHEN expressions cannot contain
-  subqueries.
+   In both FOR EACH ROW and FOR EACH 
STATEMENT
+   triggers, INSERT triggers cannot refer to 
OLD
+   row values or transition tables, and DELETE triggers 
cannot refer
+   to NEW row values or transition tables. However,
+   UPDATE triggers are able to refer to both 
OLD
+   and NEW
+ 
+
+ INSTEAD OF triggers do not support 

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

[David Fetter]

On Wed, Dec 30, 2020 at 03:00:17PM -0500, 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.

To that last, I suspect that there are folks in regulated industries
who want to be able to show that they've deployed at some kind of
minimal level of protection.  If there's not a window during which a
non-conforming pg_hba.conf is in play, that's easier to do.

> 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
> 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.)

Am I understanding correctly that you're suggesting we write up a
formal specification of pg_hba.conf? That could be handy if we don't
choose to export the parser the backend uses for it, for example
because we want it to respond super quickly to HUPs, which might
conflict with making it usable by things that weren't the backend.

I agree that anything that does a write to pg_hba.conf needs to be
approached with a good deal of caution. Audit tools such as you
propose could spit out a suggestion that doesn't overwrite, although
it could get a little hairy if it's intended to patch something that
has an include directive in it.

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: Reloptions for table access methods

[Simon Riggs]

On Tue, 15 Dec 2020 at 23:59, Jeff Davis  wrote:
>
> On Tue, 2020-12-15 at 17:37 +, Simon Riggs wrote:
> >
> > I definitely don't agree with the premise that all existing heap
> > options should be common across all new or extension tableAMs. There
> > are dozens of such options and I don't believe that they would all
> > have meaning in all future storage plugins.
>
> I agree in principle, but looking at the options that are present
> today, a lot of them are integrated into general database features,
> like autovacuum, toast, logical replication, and parellel query
> planning.
>
> We need to have some answer about how these features should interact
> with a custom AM if we can't assume anything about the reloptions it
> understands.
>
> > I think options should just work exactly the same for Indexes and
> > Tables.
>
> How should that work with the existing code? Should we have separate AM
> hooks for each of these interaction points, and then have the AM figure
> out how to handle its options? What about the toast.* options?
>
> It feels like some common options would make sense to avoid too much
> code duplication.
>
> I am not trying to push this in a specific direction, but I don't have
> a lot of good answers, so I went for the simplest thing I could think
> of that would allow an extension to have its own options, even if it's
> a bit hacky. I'm open to alternatives.

Your argument seems to be that all new Table AMs should offer some
kind of support for these "standard" options, even if it is to accept
and then ignore them, which would presumably require them to document
that. If that is the case, then at very least offer a doc patch that
says that so people know what to do and doc comments describe how we
should treat new parameters in the future.

But you cannot seriously argue that a one line patch that changes user
visible behavior can be acceptable in Postgres core without tests or
docs or code comments. You know as a Committer that that position is
not sustainable.

Minor changes could be OK if they are complete. Please either push
forward to a usable commit or withdraw, so other options can be
considered.

-- 
Simon Riggshttp://www.EnterpriseDB.com/

Re: Buildfarm's cross-version-upgrade tests

[Andrew Dunstan]

On 12/30/20 3:42 PM, Tom Lane wrote:
> I broke $SUBJECT again today by removing regress_putenv from
> regress.so.  I don't really want to put it back, but that means
> we need a way to drop the corresponding SQL function from the
> old version's regression database before attempting an upgrade.
>
> Since I'd just seen Noah's commit 52202bb39 go by, I tried
> modifying src/bin/pg_upgrade/test.sh to do the drop, but that
> isn't helping.  I recall now from prior discussions that we
> have to modify code that's embedded in the buildfarm client
> script to make this go.  (I wonder what test scenario Noah had
> in mind, exactly.)
>
> Realizing that this has happened before and will happen again,
> I'm now wondering if we can't devise a fixup method that is
> controllable by committers and doesn't require a buildfarm
> client rollout to update.  I'm thinking maybe we could extract
> the fixup logic from test.sh into a standalone SQL script that's
> part of the regular source tree so it can be updated by committers,
> and then both test.sh and the buildfarm script could invoke it.
> Maybe that won't work for some reason, but I'd sure like to find
> some way of handling this without making you get involved every time.
>
>   



Well, it's not hugely onerous, although it does seem to have happened a
bit more lately than once it used to. Basically, there are two sets of
adjustments. First there are the things we do when we're saving a
cluster for later upgrade testing. These can be found at lines 238 - 300
of the module (see
).
Then, when it comes time to do an upgrade test, we make a copy of the
cluster and make further adjustments depending on the target version as
well as the source version. See lines 363 - 496. Some of this is a bit
more complicated because we test all the way back to 9.2, even though
it's past EOL.

It seems reasonable to want to institutionalize this knowledge. I'll see
if I can extract it into one or two perl scripts suitable for inclusion
in core code.

I'll try to fix the test for the latest breakage shortly.


cheers


andrew


-- 

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

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

[Stephen Frost]

Greetings,

* Paul Martinez (paul...@google.com) wrote:
> You've identified exactly the problem we're running into -- we want to
> allow customers, who aren't superusers, to create replication roles.

This is also where it's probably useful to think about what the impact
of that is- after all, since it seems you're considering this for the
purposes of logical replication, it'd make sense to draw your attention
to this:

https://www.postgresql.org/docs/10/logical-replication-security.html

which talks about the rather serious consequences, from a security
perspective, of allowing non-superusers access to logical replication
and modifying publication and subscription sets and tables.  Perhaps
these are things you've already addressed, but we'd need to address them
in core to have such a capability make sense.

> We haven't seriously considered what some sort of general functionality
> would look like to support our managed Postgres use-cases, but here's a
> rough sketch of what we're trying to accomplish with roles and permissions:
> 
> - We, ideally, want to give our customers access to as much of Postgres'
>   functionality as possible, including SUPERUSER capabilities
> 
> - But we do not want customers to have access to the underlying VM or
>   filesystem

Certainly, the above two things are in pretty direct conflict, as you've
discovered. :)

That said, we've started working in this direction with the initial (or
default) role system, as a way to express capabilities which can then be
inherited or GRANT'd by roles with appropriate rights to other roles.
In general, it seems like that's a better approach and that it'd make
sense to move away from the role attributes.

> - There are certain parts of the system (i.e., certain databases, tables,
>   individual rows in catalog tables, etc.) that we want to manage and we
>   can't allow customers to modify these at all. Examples include:
>   - Our own SUPERUSER role that we use to connect to the cluster; customers
> shouldn't be able to modify this role at all

Roles, in general, can't modify other roles (except with CREATEROLE,
which is more an argument to get rid of it and not use it than anything
else, imv).

>   - Replication slots used for managed replication shouldn't be able to be
> modified by customers (even if they have the REPLICATION attribute)

That's an interesting example and one which probably means we need to
have some kind of ownership/privilege model added to replication slots.

> - We want to prevent customers from depending on any data we choose to store
>   in other database, as that limits our flexibility to make future changes
>   - Notably this means we only can support logical replication, and not
> physical replication.
> 

Being able to give users the ability to use logical replication for a
subset of the system is certainly a capability that I think we'd like to
have.

> I suppose this could be roughly summarized as "90% of a superuser, but also
> still obeys SQL privileges for objects created by someone else".

I don't really think that it makes sense to think of it that way-
superuser is superuser, everything is "user with certain privileges
granted to it" and we should be thinking of it that way.

Thanks,

Stephen


signature.asc
Description: PGP signature

Re: Proposed patch for key management

[Bruce Momjian]

On Mon, Dec 28, 2020 at 06:15:44PM -0400, Fabien COELHO wrote:
> The API should NOT make assumptions about the cryptographic design, what
> depends about what, where things are stored… ISTM that Pg should only care
> about naming keys, holding them when created/retrieved (but not create
> them), basically interacting with the key manager, passing the stuff to
> functions for encryption/decryption seen as black boxes.
> 
> I may have suggested something along these lines at the beginning of the key
> management thread, probably. Not going this way implicitely implies making
> some assumptions which may or may not suit other use cases, so makes them
> specific not generic. I do not think pg should do that.

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.  Adding a pre-defined system will not prevent future work on a
completely external option.  I know archive_command is completely
external, but that is much simpler than what would need to be done for
key management, key rotation, and key verification.

I will say that if the community feels external-only should be the only
option, I will stop working on this feature because I feel the result
would be too fragile to be reliable, and I would not want to be
associated with it.

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

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

Re: Proposed patch for key managment

[Stephen Frost]

Greetings,

* Fabien COELHO (coe...@cri.ensmp.fr) wrote:
> I think that an API should be carefully thought about, without assumption
> about the underlying cryptography (algorithm, key lengths, modes, how keys
> are derived and stored, and so on), and its usefulness be demonstrated by
> actually being used for one implementation which would be what is currently
> being proposed in the patch, and possibly others thrown in for free.

Perhaps I'm misunderstanding, but this is basically what we have in the
currently proposed patch as 'cipher.h', with cipher.c as a stub that
fails if called directly, and cipher_openssl.c with the actual OpenSSL
based implementation.

> The implementations should not have to be in any particular language: Shell,
> Perl, Python, C should be possible.

I disagree that it makes any sense to pass actual encryption out to a
shell script.

> After giving it more thought during the day, I think that only one
> command and a basic protocol is needed. Maybe something as simple as
> 
>   /path/to/command --options arguments…

This is what's proposed- a command is run to acquire the KEK (as a hex
encoded set of bytes, making it possible to use a shell script, which is
what the patch does too).

> With a basic (text? binary?) protocol on stdin/stdout (?) for the different
> functions. What the command actually does (connect to a remote server, ask
> for a master password, open some other database, whatever) should be
> irrelevant to pg, which would just get and pass bunch of bytes to functions,
> which could use them for keys, secrets, whatever, and be easily replaceable.

Right- we just get bytes back from the command and then we use that as
the KEK.  How that scripts gets those bytes is entirely up to the script
and is not an issue for PG to worry about.

> The API should NOT make assumptions about the cryptographic design, what
> depends about what, where things are stored… ISTM that Pg should only care
> about naming keys, holding them when created/retrieved (but not create
> them), basically interacting with the key manager, passing the stuff to
> functions for encryption/decryption seen as black boxes.

The API to fetch the KEK doesn't care at all about where it's stored or
how it's derived or anything like that.  There's a relatively small
change which could be made to have PG request all of the keys that it'll
need on startup, if we want to go there as has been suggested elsewhere,
but even if we do that, PG needs to be able to do that itself too,
otherwise it's not a complete capability and there seems little point in
doing something that's just a pass-thru to something else and isn't able
to really be used.

Thanks,

Stephen


signature.asc
Description: PGP signature

Re: HOT chain bug in latestRemovedXid calculation

[Peter Geoghegan]

On Mon, Dec 28, 2020 at 9:49 PM Peter Geoghegan  wrote:
> I would like to commit this patch to v12, the first version that did
> this process during original execution rather than in REDO routines.
> It seems worth keeping the back branches in sync here. I suspect that
> the old approach used prior to Postgres 12 has subtle buglets caused
> by inconsistencies during Hot Standby (I have heard rumors). I'd
> rather just not go there given the lack of field reports about this
> problem.

Pushed that a moment ago.

-- 
Peter Geoghegan

Moving other hex functions to /common

[Bruce Momjian]

I now understand the wisdom of moving the remaining hex functions to
/common.  I know someone already suggested that, and the attached patch
does this.

I will add the attached patch to the commitfest so I can get cfbot
testing.

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

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

diff --git a/src/backend/replication/backup_manifest.c b/src/backend/replication/backup_manifest.c
new file mode 100644
index c3f339c..716f114
*** a/src/backend/replication/backup_manifest.c
--- b/src/backend/replication/backup_manifest.c
***
*** 17,23 
  #include "libpq/pqformat.h"
  #include "mb/pg_wchar.h"
  #include "replication/backup_manifest.h"
! #include "utils/builtins.h"
  #include "utils/json.h"
  
  static void AppendStringToManifest(backup_manifest_info *manifest, char *s);
--- 17,23 
  #include "libpq/pqformat.h"
  #include "mb/pg_wchar.h"
  #include "replication/backup_manifest.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
new file mode 100644
index a6c65b1..bca941a
*** a/src/backend/utils/adt/encode.c
--- b/src/backend/utils/adt/encode.c
***
*** 15,21 
  
  #include 
  
! #include "common/hex_decode.h"
  #include "mb/pg_wchar.h"
  #include "utils/builtins.h"
  #include "utils/memutils.h"
--- 15,21 
  
  #include 
  
! #include "common/hex.h"
  #include "mb/pg_wchar.h"
  #include "utils/builtins.h"
  #include "utils/memutils.h"
*** binary_decode(PG_FUNCTION_ARGS)
*** 142,179 
  
  
  /*
-  * 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
   */
  
--- 142,147 
diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c
new file mode 100644
index 9300d19..79fcdcd
*** a/src/backend/utils/adt/varlena.c
--- b/src/backend/utils/adt/varlena.c
***
*** 22,28 
  #include "catalog/pg_type.h"
  #include "common/hashfn.h"
  #include "common/int.h"
! #include "common/hex_decode.h"
  #include "common/unicode_norm.h"
  #include "lib/hyperloglog.h"
  #include "libpq/pqformat.h"
--- 22,28 
  #include "catalog/pg_type.h"
  #include "common/hashfn.h"
  #include "common/int.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
new file mode 100644
index f624977..93eb27a
*** a/src/common/Makefile
--- b/src/common/Makefile
*** OBJS_COMMON = \
*** 58,64 
  	file_perm.o \
  	file_utils.o \
  	hashfn.o \
! 	hex_decode.o \
  	ip.o \
  	jsonapi.o \
  	keywords.o \
--- 58,64 
  	file_perm.o \
  	file_utils.o \
  	hashfn.o \
! 	hex.o \
  	ip.o \
  	jsonapi.o \
  	keywords.o \
diff --git a/src/common/hex_decode.c b/src/common/hex_decode.c
new file mode 100644
index 3ecdc73..97f57bc
*** a/src/common/hex_decode.c
--- b/src/common/hex_decode.c
***
*** 1,7 
  /*-
   *
!  * hex_decode.c
!  *		hex decoding
   *
   *
   * Portions Copyright (c) 1996-2020, PostgreSQL Global Development Group
--- 1,7 
  /*-
   *
!  * hex.c
!  *		hex processing
   *
   *
   * Portions Copyright (c) 1996-2020, PostgreSQL Global Development Group
***
*** 9,15 
   *
   *
   * IDENTIFICATION
!  *	  src/common/hex_decode.c
   *
   *-
   */
--- 9,15 
   *
   *
   * IDENTIFICATION
!  *	  src/common/hex.c
   *
   *-
   */
***
*** 26,32 
  #else
  #include "mb/pg_wchar.h"
  #endif
! #include "common/hex_decode.h"
  
  
  static const int8 hexlookup[128] = {
--- 26,32 
  #else
  #include "mb/pg_wchar.h"
  #endif
! #include "common/hex.h"
  
  
  static const int8 hexlookup[128] = {
*** static const int8 hexlookup[128] = {
*** 40,45 
--- 40,65 
  	-1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
  };
  
+ /*
+  * HEX
+  */
+ 
+ static const char hextbl[] = "0123456789abcdef";
+ 
+ uint64
+ hex_encode(const char *src, size_t len, char *dst)
+ {
+ 	const char *end = src + len;
+ 
+ 	while (src <

Re: Moving other hex functions to /common

[Bruce Momjian]

On Wed, Dec 30, 2020 at 07:35:57PM -0500, Bruce Momjian wrote:
> I now understand the wisdom of moving the remaining hex functions to
> /common.  I know someone already suggested that, and the attached patch
> does this.
> 
> I will add the attached patch to the commitfest so I can get cfbot
> testing.

So, I am learning this cfbot thing.  Seems I need -M100% to disable
rename detection for diffs to work with cfbot --- makes sense.
New patch attached.

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

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



hex.diff.gz
Description: application/gzip

Re: Proposed patch for key managment

[Bruce Momjian]

On Wed, Dec 30, 2020 at 06:49:34PM -0500, Stephen Frost wrote:
> The API to fetch the KEK doesn't care at all about where it's stored or
> how it's derived or anything like that.  There's a relatively small
> change which could be made to have PG request all of the keys that it'll
> need on startup, if we want to go there as has been suggested elsewhere,
> but even if we do that, PG needs to be able to do that itself too,
> otherwise it's not a complete capability and there seems little point in
> doing something that's just a pass-thru to something else and isn't able
> to really be used.

Right now, the script returns a cluster key (KEK), and during initdb the
server generates data encryption keys and wraps them with the KEK. 
During server start, the server validates the KEK and decrypts the data
keys.  pg_alterckey allows changing the KEK.

I think Fabien is saying this all should _only_ be done using external
tools --- that's what I don't agree with.

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

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

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

[Bharath Rupireddy]

On Wed, Dec 30, 2020 at 11:11 PM Alexey Kondratov
 wrote:
> On 2020-12-30 17:59, Bharath Rupireddy wrote:
> > On Wed, Dec 30, 2020 at 5:20 PM Alexey Kondratov
> >  wrote:
> >>
> >> On 2020-12-30 09:10, Bharath Rupireddy wrote:
> >> I still have some doubts that it is worth of allowing to call
> >> postgres_fdw_disconnect() in the explicit transaction block, since it
> >> adds a lot of things to care and check for. But otherwise current
> >> logic
> >> looks solid.
> >>
> >> +errdetail("Such connections get
> >> closed either in the next use or
> >> at the end of the current transaction.")
> >> +: errdetail("Such connection gets
> >> closed either in the next use or
> >> at the end of the current transaction.")));
> >>
> >> Does it really have a chance to get closed on the next use? If foreign
> >> server is dropped then user mapping should be dropped as well (either
> >> with CASCADE or manually), but we do need user mapping for a local
> >> cache
> >> lookup. That way, if I understand all the discussion up-thread
> >> correctly, we can only close such connections at the end of xact, do
> >> we?
> >
> > The next use of such a connection in the following query whose foreign
> > server would have been dropped fails because of the cascading that can
> > happen to drop the user mapping and the foreign table as well. During
> > the start of the next query such connection will be marked as
> > invalidated because xact_depth of that connection is > 1 and when the
> > fail happens, txn gets aborted due to which pgfdw_xact_callback gets
> > called and in that the connection gets closed. To make it more clear,
> > please have a look at the scenarios [1].
> >
>
> In my understanding 'connection gets closed either in the next use'
> means that connection will be closed next time someone will try to use
> it, i.e. GetConnection() will be called and it closes this connection
> because of a bad state. However, if foreign server is dropped
> GetConnection() cannot lookup the connection because it needs a user
> mapping oid as a key.

Right. We don't reach GetConnection(). The look up in either
GetForeignTable() or GetUserMapping() or GetForeignServer() fails (and
so the query) depending one which one gets called first.

> I had a look on your scenarios. IIUC, under **next use** you mean a
> select attempt from a table belonging to the same foreign server, which
> leads to a transaction abort and connection gets closed in the xact
> callback. Sorry, maybe I am missing something, but this just confirms
> that such connections only get closed in the xact callback (taking into
> account your recently committed patch [1]), so 'next use' looks
> misleading.
>
> [1]
> https://www.postgresql.org/message-id/8b2aa1aa-c638-12a8-cb56-ea0f0a5019cf%40oss.nttdata.com

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"

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

WIP: document the hook system

[David Fetter]

Please find attached :)

This could probably use a lot of filling in, but having it in the
actual documentation beats needing to know folklore even to know
that the capability is there.

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
>From 5152b3f5255ec91b6ea34b76ea765a26d392d3ac Mon Sep 17 00:00:00 2001
From: David Fetter 
Date: Wed, 30 Dec 2020 19:13:57 -0800
Subject: [PATCH v1] WIP: Document the hooks system
To: hackers
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="2.29.2"

This is a multi-part message in MIME format.
--2.29.2
Content-Type: text/plain; charset=UTF-8; format=fixed
Content-Transfer-Encoding: 8bit


Outline of something that should probably have more detail, but it's
probably better than nothing at all.

 create mode 100644 doc/src/sgml/hooks.sgml


--2.29.2
Content-Type: text/x-patch; name="v1-0001-WIP-Document-the-hooks-system.patch"
Content-Transfer-Encoding: 8bit
Content-Disposition: attachment; filename="v1-0001-WIP-Document-the-hooks-system.patch"

diff --git doc/src/sgml/filelist.sgml doc/src/sgml/filelist.sgml
index 38e8aa0bbf..16f78b371a 100644
--- doc/src/sgml/filelist.sgml
+++ doc/src/sgml/filelist.sgml
@@ -58,6 +58,7 @@
 
 
 
+
 
 
 
diff --git doc/src/sgml/hooks.sgml doc/src/sgml/hooks.sgml
new file mode 100644
index 00..5891f74dc8
--- /dev/null
+++ doc/src/sgml/hooks.sgml
@@ -0,0 +1,321 @@
+
+
+
+ Hooks System
+
+ 
+  Hook
+ 
+
+ 
+  This chapter explains the PostgreSQL
+  hooks system, a way to inject functionality in many different parts
+  of the database via function pointers to shared libraries. They simply
+  need to honor the hooks API, which consists roughly of the following:
+
+  
+
+ 
+ Initialize a hook of the correct type for what you are doing, conventionally
+ using a function with signature:
+
+void _PG_init(void)
+
+ which initializes whatever the hook code needs initialized.  Here, you will cache
+ any previous hook(s) with code that looks like:
+
+prev_Foo = Foo_hook;
+prev_Bar = Bar_hook;
+
+ 
+
+
+
+ 
+ Let your imagination run wild, but try very hard not to crash while doing so.
+ 
+
+
+
+ 
+ Restore cached hooks in a function conventionally called
+
+void _PG_fini(void)
+
+ with code that looks like:
+
+Foo_hook = prev_Foo;
+Bar_hook = prev_Bar;
+
+ 
+
+
+  
+ 
+
+ 
+  Available Hooks
+
+  
+   The following backend hooks are available for your use:
+  
+
+  
+   Analyze hook
+
+   
+Take control at the end of parse analysis.
+   
+  
+
+  
+   Authentication hook
+
+   
+Take control in ClientAuthentication.
+   
+  
+
+  
+   Logging hook
+
+   
+Intercept messages before they are sent to the server log.
+   
+  
+
+  
+   Executor hooks
+
+   
+The following hooks control the executor:
+   
+
+   
+Executor Start Hook
+
+
+ Take control in ExecutorStart().
+
+   
+
+   
+Executor Run Hook
+
+
+ Take control in ExecutorRun().
+
+   
+
+   
+Executor Finish Hook
+
+
+ Take control in ExecutorFinish().
+
+   
+
+   
+Executor End Hook
+
+
+ Take control in ExecutorEnd().
+
+   
+
+   
+Executor Check Permissions Hook
+
+
+ Take control in ExecutorCheckRTPerms().
+
+   
+
+  
+
+  
+   Explain hook
+
+   
+Take control of EXPLAIN.
+   
+
+   
+Explain One Query
+
+
+ Take control in ExplainOneQuery().
+
+   
+
+   
+Explain Get Index Name
+
+
+ Take control in explain_get_index_name().
+
+   
+
+  
+
+  
+   Function Call Hooks
+
+   
+Needs Function Manager
+
+
+ Look around and decide whether the function manager hook is needed.
+
+   
+
+   
+Function Manager
+
+
+ Take control at start, end, or abort of a ROUTINE.
+
+   
+  
+
+  
+   Shared Memory Startup
+
+   
+Take control of chunks of shared memory at startup.
+   
+  
+
+  
+   OpenSSL TLS Init
+
+   
+Take control in initialization of OpenSSL.
+   
+  
+
+  
+   Get Attribute Average Width
+
+   
+Take control in get_attavgwidth().
+   
+  
+
+  
+   Object Access Hooks
+
+   
+Take control just before or just after CREATE,
+DROP, ALTER,
+namespace search, function execution, and TRUNCATE.
+   
+  
+
+  
+   Path (Optimizer) Hooks
+
+   
+Take control during query planning.
+   
+
+   
+Set Relation Pathlist
+
+
+ Take control in set_rel_pathlist().
+
+   
+
+   
+Set Join Pathlist
+
+
+ Take control in set_join_pathlist().
+
+   
+
+   
+Join Search
+
+
+ Take control in join_search().
+
+   
+  
+
+  
+   Get Relation Info Hook
+
+   
+Take control in get_relation_info().
+   
+  
+
+  
+   Planner Hooks
+
+   
+Take control in parts of the planner intended to be called by the optimizer.
+   
+
+   
+ 

Re: Parallel Inserts in CREATE TABLE AS

[Dilip Kumar]

On Wed, Dec 30, 2020 at 7:47 PM Bharath Rupireddy
 wrote:
>
> Thanks for the comments.
>
> How about naming like below more generically and placing them in
> parallel.h so that it will also be used for refresh materialized view?
>
> +typedef enum ParallelInsertTupleCostOpt
> +{
> + PINS_SELECT_QUERY = 1 << 0, /* turn on this before planning */
> + /*
> + * Turn on this while planning for upper Gather path to ignore parallel
> + * tuple cost in cost_gather.
> + */
> + PINS_CAN_IGN_TUP_COST = 1 << 1,
> + /* Turn on this after the cost is ignored. */
> + PINS_TUP_COST_IGNORED = 1 << 2
>
> My plan was to get the main design idea of pushing the dest receiver
> to gather reviewed and once agreed, then I thought of making few
> functions common and place them in parallel.h and parallel.c so that
> they can be used for Parallel Inserts in REFRESH MATERIALIZED VIEW
> because the same design idea can be applied there as well.


I think instead of PINS_* we can name PARALLEL_INSERT_* other than
that I am fine with the name.

> For instance my thoughts are: add the below structures, functions and
> other macros to parallel.h and parallel.c:
> typedef enum ParallelInsertKind
> {
> PINS_UNDEF = 0,
> PINS_CREATE_TABLE_AS,
> PINS_REFRESH_MAT_VIEW
> } ParallelInsertKind;
>
> typedef struct ParallelInsertCTASInfo
> {
> IntoClause *intoclause;
> Oid objectid;
> } ParallelInsertCTASInfo;
>
> typedef struct ParallelInsertRMVInfo
> {
> Oid objectid;
> } ParallelInsertRMVInfo;
>
> ExecInitParallelPlan(PlanState *planstate, EState *estate,
>   Bitmapset *sendParams, int nworkers,
> - int64 tuples_needed)
> + int64 tuples_needed, ParallelInsertKind pinskind,
> + void *pinsinfo)
>
> Change ExecParallelInsertInCTAS to
>
> +static void
> +ExecParallelInsert(GatherState *node)
> +{
>
> Change SetCTASParallelInsertState to
> +void
> +SetParallelInsertState(QueryDesc *queryDesc)
>
> Change IsParallelInsertionAllowedInCTAS to
>
> +bool
> +IsParallelInsertionAllowed(ParallelInsertKind pinskind, IntoClause *into)
> +{
>
> Thoughts?
>

I haven’t thought about these structures yet but yeah making them
generic will be good.

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

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

[Amit Kapila]

On Thu, Dec 31, 2020 at 10:48 AM Amit Kapila  wrote:
>
> On Wed, Dec 30, 2020 at 6:49 PM Amit Kapila  wrote:
> >
> > On Tue, Dec 29, 2020 at 3:15 PM Ajin Cherian  wrote:
> > >
> > > Hi Sawada-san,
> > >
> > > I think Amit has a plan to commit this patch-set in phases.
> > >
> >
> > I have pushed the first patch and I would like to make a few changes
> > in the second patch after which I will post the new version. I'll try
> > to do that tomorrow if possible and register the patch.
> >
>
> Please find attached a rebased version of this patch-set.
>

Registered in CF (https://commitfest.postgresql.org/31/2914/).

-- 
With Regards,
Amit Kapila.

Re: Moving other hex functions to /common

[Michael Paquier]

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.

> 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).
--
Michael


signature.asc
Description: PGP signature

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

[Ajin Cherian]

On Thu, Dec 31, 2020 at 4:16 PM Amit Kapila  wrote:

> 3. Merged the doc changes patch after some changes mostly cosmetic.
Some minor comments here:

v35-0001 - logicaldecoding.sgml

In the example section:
Change "The following example shows SQL interface can be used to
decode prepared  transactions."
to "The following example shows the SQL interface that can be used to
decode prepared transactions."

Then in "Two-phase commit support for Logical Decoding" page:
Change "To support streaming of two-phase commands, an output plugin
needs to provide the additional callbacks."
to "To support streaming of two-phase commands, an output plugin needs
to provide additional callbacks."

Other than that, I have no more comments.

regards,
Ajin Cherian
Fujitsu Australia

Re: Buildfarm's cross-version-upgrade tests

[Noah Misch]

On Wed, Dec 30, 2020 at 04:28:44PM -0500, Andrew Dunstan wrote:
> On 12/30/20 3:42 PM, Tom Lane wrote:
> > Since I'd just seen Noah's commit 52202bb39 go by, I tried
> > modifying src/bin/pg_upgrade/test.sh to do the drop, but that
> > isn't helping.  I recall now from prior discussions that we
> > have to modify code that's embedded in the buildfarm client
> > script to make this go.  (I wonder what test scenario Noah had
> > in mind, exactly.)

Commit 52202bb39 affected the cross-version test harness described in
src/bin/pg_upgrade/TESTING.  That harness is disused, having been broken at
head for at least 40 months.  (One can work around the remaining breakage by
locally back-patching e78900a into REL_13_STABLE.)

> > I'm now wondering if we can't devise a fixup method that is
> > controllable by committers and doesn't require a buildfarm
> > client rollout to update.

Ideally, the buildfarm code should curate per-version source and installation
trees, provide their locations to the test harness, and collect logs.  Other
test logic (i.e. the kinds of steps that appear in test.sh) should live in
postgresql.git code.  (That's the separation of concerns for all
run-by-default buildfarm tests.)