jsonb_array_elements_recursive()

[Joel Jacobson]

Hi,

A particular useful feature of jsonb arrays,
is the ability to represent multidimensional arrays without matching dimensions,
which is not possible with normal PostgreSQL arrays.

SELECT array[[5,2],1,[8,[3,2],6]];
ERROR:  multidimensional arrays must have array expressions with matching 
dimensions

SELECT '[[5,2],1,[8,[3,2],6]]'::jsonb;
[[5, 2], 1, [8, [3, 2], 6]]

When working with jsonb array structures,
there is already jsonb_array_elements() to expand the top-level.

Another case that I think is common is wanting to expand all levels, not just 
the top-level.

Maybe it's common enough to motivate a new param:

   jsonb_array_elements(from_json jsonb [, recursive boolean ])

Or as a separate function. Below is a PoC in PL/pgSQL:

CREATE OR REPLACE FUNCTION jsonb_array_elements_recursive(from_json jsonb, OUT 
value jsonb)
RETURNS SETOF jsonb
LANGUAGE plpgsql
AS $$
BEGIN
FOR value IN SELECT jsonb_array_elements(from_json) LOOP
  IF jsonb_typeof(value) <> 'array' THEN
RETURN NEXT;
  ELSE
RETURN QUERY
SELECT * FROM jsonb_array_elements_recursive(value);
  END IF;
END LOOP;
END
$$;

# SELECT * FROM jsonb_array_elements_recursive('[[5, 2], 1, [8, [3, 2], 
6]]'::jsonb);
value
---
5
2
1
8
3
2
6
(7 rows)

I tried but failed to implement a PoC in pure SQL,
not even using the new CTE SEARCH functionality,
but maybe it's possible somehow.

/Joel

Re: Prevent printing "next step instructions" in initdb and pg_upgrade

[Magnus Hagander]

On Wed, Feb 3, 2021 at 4:21 PM Peter Eisentraut
 wrote:
>
> On 2021-01-17 14:38, Magnus Hagander wrote:
> > On Thu, Jan 7, 2021 at 11:53 AM Peter Eisentraut
> >  wrote:
> >>
> >> After pondering this again, I think we can go with initdb
> >> --no-instructions, as in your patch.
> >>
> >> As a minor nitpick, I would leave out the
> >>
> >>   else
> >>   printf(_("\nSuccess.\n"));
> >>
> >> in the --no-instructions case.
> >
> > OK, thanks. I have applied it as such, with that message moved inside
> > the if statement.
>
> It appears that there is an extra blank line in the initdb output before
> "Success" now.

Oops, clearly it does.

That said, the full output is:

"""
Success. You can now start the database server using:

bin/pg_ctl -D /tmp/data -l logfile start


Success.
"""

Isn't the whole "Success." at the end redundant here, and we should
just end the message after the pg_ctl command? So not just the extra
newline, but the whole thing?

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

Re: Bug in query rewriter - hasModifyingCTE not getting set

[Greg Nancarrow]

On Sun, Feb 7, 2021 at 10:03 AM Tom Lane  wrote:
>
> Greg Nancarrow  writes:
> > I found a bug in the query rewriter. If a query that has a modifying
> > CTE is re-written, the hasModifyingCTE flag is not getting set in the
> > re-written query.
>
> Ugh.
>
> > I've attached the patch with the suggested fix (reviewed by Amit Langote).
>
> I think either the bit about rule_action is unnecessary, or most of
> the code immediately above this is wrong, because it's only updating
> flags in sub_action.  Why do you think it's necessary to change
> rule_action in addition to sub_action?
>

I believe that the bit about rule_action IS necessary, as it's needed
for the case of INSERT...SELECT, so that hasModifyingCTE is set on the
rewritten INSERT (see comment above the call to
getInsertSelectQuery(), and the "KLUDGE ALERT" comment within that
function).

In the current Postgres code, it doesn't let INSERT run in
parallel-mode (only SELECT), but in the debugger you can clearly see
that for an INSERT with a subquery that uses a modifying CTE, the
hasModifyingCTE flag is not getting set on the rewritten INSERT query
by the query rewriter. As I've been working on parallel INSERT, I
found the issue first for INSERT (one test failure in the "with" tests
when force_parallel_mode=regress).

Here's some silly SQL (very similar to existing test case in the
"with" tests) to reproduce the issue for INSERT (as I said, it won't
give an error like the SELECT case, as currently INSERT is not allowed
in parallel-mode anyway, but the issue can be seen in the debugger):

set force_parallel_mode=on;
CREATE TABLE bug6051 AS
  select i from generate_series(1,3) as i;
SELECT * FROM bug6051;
CREATE TABLE bug6051_2 (i int);
CREATE RULE bug6051_ins AS ON INSERT TO bug6051 DO INSTEAD
 INSERT INTO bug6051_2
 SELECT NEW.i;
WITH t1 AS ( DELETE FROM bug6051 RETURNING * )
INSERT INTO bug6051 SELECT * FROM t1;


Regards,
Greg Nancarrow
Fujitsu Australia

Re: TRUNCATE on foreign table

[Kazutaka Onishi]

Thank you for your comment! :D
I have fixed it and attached the revised patch.

regards,



2021年2月7日(日) 2:08 Zhihong Yu :

> Hi,
> +   if (strcmp(defel->defname, "truncatable") == 0)
> +   server_truncatable = defGetBoolean(defel);
>
> Looks like we can break out of the loop when the condition is met.
>
> +   /* ExecForeignTruncate() is invoked for each server */
>
> The method name in the comment is slightly different from the actual
> method name.
>
> +   if (strcmp(defel->defname, "truncatable") == 0)
> +   truncatable = defGetBoolean(defel);
>
> We can break out of the loop when the condition is met.
>
> Cheers
>
> On Sat, Feb 6, 2021 at 5:11 AM Kazutaka Onishi 
> wrote:
>
>> Hello,
>>
>> The attached patch is for supporting "TRUNCATE" on  foreign tables.
>>
>> This patch includes:
>> * Adding "ExecForeignTruncate" function into FdwRoutine.
>> * Enabling "postgres_fdw" to use TRUNCATE.
>>
>> This patch was proposed by Kaigai-san in March 2020,
>> but it was returned because it can't be applied to the latest source
>> codes.
>>
>> Please refer to the discussion.
>>
>> https://www.postgresql.org/message-id/flat/CAOP8fzb-t3WVNLjGMC%2B4sV4AZa9S%3DMAQ7Q6pQoADMCf_1jp4ew%40mail.gmail.com#3b6c6ff85ff5c722b36c7a09b2dd7165
>>
>> I have fixed the patch due to submit it to Commit Fest 2021-03.
>>
>> regards,
>>
>> --
>> --
>> Kazutaka Onishi
>> (oni...@heterodb.com)
>>
>

-- 
--
Kazutaka Onishi
(oni...@heterodb.com)


-- 
--
Kazutaka Onishi
(oni...@heterodb.com)
diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c
index 6faf499f9a..a9ce323a67 100644
--- a/contrib/postgres_fdw/deparse.c
+++ b/contrib/postgres_fdw/deparse.c
@@ -2171,6 +2171,44 @@ deparseAnalyzeSql(StringInfo buf, Relation rel, List **retrieved_attrs)
 	deparseRelation(buf, rel);
 }
 
+/*
+ * Construct a simple "TRUNCATE rel" statement
+ */
+void
+deparseTruncateSql(StringInfo buf,
+   List *frels_list,
+   List *frels_extra,
+   DropBehavior behavior,
+   bool restart_seqs)
+{
+	ListCell   *lc1, *lc2;
+
+	appendStringInfoString(buf, "TRUNCATE ");
+	forboth (lc1, frels_list,
+			 lc2, frels_extra)
+	{
+		Relation	frel = lfirst(lc1);
+		int			extra = lfirst_int(lc2);
+
+		if (lc1 != list_head(frels_list))
+			appendStringInfoString(buf, ", ");
+		if (extra != 0)
+			appendStringInfoString(buf, "ONLY ");
+		deparseRelation(buf, frel);
+	}
+	appendStringInfo(buf, " %s IDENTITY",
+	 restart_seqs ? "RESTART" : "CONTINUE");
+	switch (behavior)
+	{
+		case DROP_RESTRICT:
+			appendStringInfoString(buf, " RESTRICT");
+			break;
+		case DROP_CASCADE:
+			appendStringInfoString(buf, " CASCADE");
+			break;
+	}
+}
+
 /*
  * Construct name to use for given column, and emit it into buf.
  * If it has a column_name FDW option, use that instead of attribute name.
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 60c7e115d6..e50704d101 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -8247,6 +8247,239 @@ select * from rem3;
 
 drop foreign table rem3;
 drop table loc3;
+-- ===
+-- test for TRUNCATE
+-- ===
+CREATE TABLE tru_rtable0 (id int primary key, x text);
+CREATE TABLE tru_rtable1 (id int primary key, y text);
+CREATE FOREIGN TABLE tru_ftable (id int, x text)
+   SERVER loopback OPTIONS (table_name 'tru_rtable0');
+INSERT INTO tru_rtable0 (SELECT x,md5(x::text) FROM generate_series(1,10) x);
+CREATE TABLE tru_ptable (id int, y text) PARTITION BY HASH(id);
+CREATE TABLE tru_ptable__p0 PARTITION OF tru_ptable
+FOR VALUES WITH (MODULUS 2, REMAINDER 0);
+CREATE FOREIGN TABLE tru_ftable__p1 PARTITION OF tru_ptable
+FOR VALUES WITH (MODULUS 2, REMAINDER 1)
+   SERVER loopback OPTIONS (table_name 'tru_rtable1');
+INSERT INTO tru_ptable (SELECT x,md5(x::text) FROM generate_series(11,20) x);
+CREATE TABLE tru_pk_table(id int primary key, z text);
+CREATE TABLE tru_fk_table(fkey int references tru_pk_table(id));
+INSERT INTO tru_pk_table (SELECT x,md5((x+1)::text) FROM generate_series(1,10) x);
+INSERT INTO tru_fk_table (SELECT x % 10 + 1 FROM generate_series(5,25) x);
+CREATE FOREIGN TABLE tru_pk_ftable (id int, z text)
+   SERVER loopback OPTIONS (table_name 'tru_pk_table');
+CREATE TABLE tru_rtable_parent (id int, a text);
+CREATE TABLE tru_rtable_child (id int, a text);
+CREATE FOREIGN TABLE tru_ftable_parent (id int, a text)
+   SERVER loopback OPTIONS (table_name 'tru_rtable_parent');
+CREATE FOREIGN TABLE tru_ftable_child () INHERITS (tru_ftable_parent)
+   SERVER loopback OPTIONS (table_name 'tru_rtable_child');
+INSERT INTO tru_rtable_parent (SELECT x, md5(x::text) FROM generat

Re: Is Recovery actually paused?

[Bharath Rupireddy]

On Fri, Feb 5, 2021 at 10:14 AM Bharath Rupireddy
 wrote:
> > We can not do that, basically, under one lock we need to check the
> > state and set it to pause.  Because by the time you release the lock
> > someone might set it to RECOVERY_NOT_PAUSED then you don't want to set
> > it to RECOVERY_PAUSED.
>
> Got it. Thanks.

Hi Dilip, I have one more question:

+/* test for recovery pause, if user has requested the pause */
+if (((volatile XLogCtlData *) XLogCtl)->recoveryPauseState ==
+RECOVERY_PAUSE_REQUESTED)
+recoveryPausesHere(false);
+
+now = GetCurrentTimestamp();
+

Do we need  now = GetCurrentTimestamp(); here? Because, I see that
whenever the variable now is used within the for loop in
WaitForWALToBecomeAvailable, it's re-calculated anyways. It's being
used within case XLOG_FROM_STREAM:

Am I missing something?

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

Re: Is Recovery actually paused?

[Dilip Kumar]

On Sun, Feb 7, 2021 at 6:44 PM Bharath Rupireddy
 wrote:
>
> On Fri, Feb 5, 2021 at 10:14 AM Bharath Rupireddy
>  wrote:
> > > We can not do that, basically, under one lock we need to check the
> > > state and set it to pause.  Because by the time you release the lock
> > > someone might set it to RECOVERY_NOT_PAUSED then you don't want to set
> > > it to RECOVERY_PAUSED.
> >
> > Got it. Thanks.
>
> Hi Dilip, I have one more question:
>
> +/* test for recovery pause, if user has requested the pause */
> +if (((volatile XLogCtlData *) XLogCtl)->recoveryPauseState ==
> +RECOVERY_PAUSE_REQUESTED)
> +recoveryPausesHere(false);
> +
> +now = GetCurrentTimestamp();
> +
>
> Do we need  now = GetCurrentTimestamp(); here? Because, I see that
> whenever the variable now is used within the for loop in
> WaitForWALToBecomeAvailable, it's re-calculated anyways. It's being
> used within case XLOG_FROM_STREAM:
>
> Am I missing something?

Yeah, I don't see any reason for doing this, maybe it got copy pasted
by mistake.  Thanks for observing this.


-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
From b5655514681317d997d8245068237acf5c6ada9d Mon Sep 17 00:00:00 2001
From: Dilip Kumar 
Date: Wed, 27 Jan 2021 16:46:04 +0530
Subject: [PATCH v12] Provide a new interface to get the recovery pause status

Currently, pg_is_wal_replay_paused, just checks whether the recovery
pause is requested or not but it doesn't actually tell whether the
recovery is actually paused or not.  So basically there is no way for
the user to know the actual status of the pause request.  This patch
provides a new interface pg_get_wal_replay_pause_state that will
return the actual status of the recovery pause i.e.'not paused' if
pause is not requested, 'pause requested' if pause is requested but
recovery is not yet paused and 'paused' if recovery is actually paused.
---
 doc/src/sgml/func.sgml | 32 +++---
 src/backend/access/transam/xlog.c  | 62 --
 src/backend/access/transam/xlogfuncs.c | 50 ---
 src/include/access/xlog.h  | 12 +--
 src/include/catalog/pg_proc.dat|  4 +++
 5 files changed, 132 insertions(+), 28 deletions(-)

diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 1ab31a9..9b2c429 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -25285,7 +25285,24 @@ postgres=# SELECT * FROM pg_walfile_name_offset(pg_stop_backup());
 boolean


-Returns true if recovery is paused.
+Returns true if recovery pause is requested.
+   
+  
+
+  
+   
+
+ pg_get_wal_replay_pause_state
+
+pg_get_wal_replay_pause_state ()
+text
+   
+   
+Returns recovery pause state.  The return values are 
+not paused if pause is not requested, 
+pause requested if pause is requested but recovery is
+not yet paused and, paused if the recovery is
+actually paused.

   
 
@@ -25324,10 +25341,15 @@ postgres=# SELECT * FROM pg_walfile_name_offset(pg_stop_backup());
 void


-Pauses recovery.  While recovery is paused, no further database
-changes are applied.  If hot standby is active, all new queries will
-see the same consistent snapshot of the database, and no further query
-conflicts will be generated until recovery is resumed.
+Request to pause recovery.  A request doesn't mean that recovery stops
+right away.  If you want a guarantee that recovery is actually paused,
+you need to check for the recovery pause state returned by
+pg_get_wal_replay_pause_state().  Note that
+pg_is_wal_replay_paused() returns whether a request
+is made.  While recovery is paused, no further database changes are applied.
+If hot standby is active, all new queries will see the same consistent
+snapshot of the database, and no further query conflicts will be generated
+until recovery is resumed.


 This function is restricted to superusers by default, but other users
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 8e3b5df..aa89e52 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -721,8 +721,8 @@ typedef struct XLogCtlData
 	 * only relevant for replication or archive recovery
 	 */
 	TimestampTz currentChunkStartTime;
-	/* Are we requested to pause recovery? */
-	bool		recoveryPause;
+	/* Recovery pause state */
+	RecoveryPauseState	recoveryPauseState;
 
 	/*
 	 * lastFpwDisableRecPtr points to the start of the last replayed
@@ -6019,7 +6019,7 @@ recoveryStopsAfter(XLogReaderState *record)
 }
 
 /*
- * Wait until shared recoveryPause flag is cleared.
+ * Wait until shared recoveryPauseState is set to RECOVERY_NOT

Re: Prevent printing "next step instructions" in initdb and pg_upgrade

[Bruce Momjian]

On Sun, Feb  7, 2021 at 11:21:05AM +0100, Magnus Hagander wrote:
> > It appears that there is an extra blank line in the initdb output before
> > "Success" now.
> 
> Oops, clearly it does.
> 
> That said, the full output is:
> 
> """
> Success. You can now start the database server using:
> 
> bin/pg_ctl -D /tmp/data -l logfile start
> 
> 
> Success.
> """
> 
> Isn't the whole "Success." at the end redundant here, and we should
> just end the message after the pg_ctl command? So not just the extra
> newline, but the whole thing?

Agreed.

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

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

Re: jsonb_array_elements_recursive()

[Joel Jacobson]

Having thought about this some more,
the function name should of course be jsonb_unnest(),
similar to how unnest() works for normal arrays:

SELECT unnest(array[[3,2],[1,4]]);
unnest

  3
  2
  1
  4
(4 rows)

SELECT jsonb_unnest('[[3,2],[1,4]]'::jsonb);
jsonb_unnest

3
2
1
4
(4 rows)

Thoughts?

Re: jsonb_array_elements_recursive()

[Pavel Stehule]

Hi

ne 7. 2. 2021 v 16:59 odesílatel Joel Jacobson  napsal:

> Having thought about this some more,
> the function name should of course be jsonb_unnest(),
> similar to how unnest() works for normal arrays:
>
> SELECT unnest(array[[3,2],[1,4]]);
> unnest
> 
>   3
>   2
>   1
>   4
> (4 rows)
>
> SELECT jsonb_unnest('[[3,2],[1,4]]'::jsonb);
> jsonb_unnest
> 
> 3
> 2
> 1
> 4
> (4 rows)
>
> Thoughts?
>

It  has  sense. Maybe it should return two columns - first path to value,
and second with value. It can be used like some "reader"

Regards

Pavel

Re: Prevent printing "next step instructions" in initdb and pg_upgrade

[Tom Lane]

Bruce Momjian  writes:
> On Sun, Feb  7, 2021 at 11:21:05AM +0100, Magnus Hagander wrote:
>> Isn't the whole "Success." at the end redundant here, and we should
>> just end the message after the pg_ctl command? So not just the extra
>> newline, but the whole thing?

> Agreed.

+1

regards, tom lane

Re: REINDEX backend filtering

[Zhihong Yu]

Hi,
For index_has_deprecated_collation(),

+   object.objectSubId = 0;

The objectSubId field is not accessed
by do_check_index_has_deprecated_collation(). Does it need to be assigned ?

For RelationGetIndexListFiltered(), it seems when (options &
REINDEXOPT_COLL_NOT_CURRENT) == 0, the full_list would be returned.
This can be checked prior to entering the foreach loop.

Cheers

On Sat, Feb 6, 2021 at 11:20 PM Julien Rouhaud  wrote:

> On Thu, Jan 21, 2021 at 11:12:56AM +0800, Julien Rouhaud wrote:
> >
> > There was a conflict with a3dc926009be8 (Refactor option handling of
> > CLUSTER, REINDEX and VACUUM), so rebased version attached.  No other
> > changes included yet.
>
> New conflict, v3 attached.
>

Re: jsonb_array_elements_recursive()

[Tom Lane]

"Joel Jacobson"  writes:
> Having thought about this some more,
> the function name should of course be jsonb_unnest(),
> similar to how unnest() works for normal arrays:

Why not just unnest(), then?

regards, tom lane

Re: jsonb_array_elements_recursive()

[Joel Jacobson]

On Sun, Feb 7, 2021, at 17:08, Pavel Stehule wrote:
>>ne 7. 2. 2021 v 16:59 odesílatel Joel Jacobson  napsal:
>>
>>SELECT jsonb_unnest('[[3,2],[1,4]]'::jsonb);
>>jsonb_unnest
>>
>>3
>>2
>>1
>>4
>>(4 rows)
>
>It  has  sense. Maybe it should return two columns - first path to value, and 
>second with value. It can be used like some >"reader"

Thanks for thinking about this.

I would expect jsonb_unnest() to have the same semantics as unnest(), but 
returning SETOF jsonb.

jsonb_unnest() implemented in C would of course be much more performant than 
the PL/pgSQL PoC.
And I think performance could be important for such a function,
so I think we should be careful adding extra complexity to such a function,
unless it can be demonstrated it is needed for a majority of cases.

/Joel

Re: jsonb_array_elements_recursive()

[Joel Jacobson]

On Sun, Feb 7, 2021, at 17:27, Tom Lane wrote:
>"Joel Jacobson"  writes:
>> Having thought about this some more,
>> the function name should of course be jsonb_unnest(),
>> similar to how unnest() works for normal arrays:
>
>Why not just unnest(), then?
>
>regards, tom lane

Ahh, of course! I totally forgot about function overloading when thinking about 
this.

+1

/Joel

Re: jsonb_array_elements_recursive()

[Zhihong Yu]

Hi,
# SELECT '[[5,2],"a",[8,[3,2],6]]'::jsonb;
 jsonb
---
 [[5, 2], "a", [8, [3, 2], 6]]
(1 row)

unnest(array[[3,2],"a",[1,4]]) is not accepted currently.

Would the enhanced unnest accept the above array ?

Cheers

On Sun, Feb 7, 2021 at 8:31 AM Joel Jacobson  wrote:

> On Sun, Feb 7, 2021, at 17:27, Tom Lane wrote:
> >"Joel Jacobson"  writes:
> >> Having thought about this some more,
> >> the function name should of course be jsonb_unnest(),
> >> similar to how unnest() works for normal arrays:
> >
> >Why not just unnest(), then?
> >
> >regards, tom lane
>
> Ahh, of course! I totally forgot about function overloading when thinking
> about this.
>
> +1
>
> /Joel
>

Re: jsonb_array_elements_recursive()

[Pavel Stehule]

Hi

ne 7. 2. 2021 v 18:31 odesílatel Zhihong Yu  napsal:

> Hi,
> # SELECT '[[5,2],"a",[8,[3,2],6]]'::jsonb;
>  jsonb
> ---
>  [[5, 2], "a", [8, [3, 2], 6]]
> (1 row)
>
> unnest(array[[3,2],"a",[1,4]]) is not accepted currently.
>
> Would the enhanced unnest accept the above array ?
>

There should be a special overwrite for json type. Json can hold an array,
but from Postgres perspective, it is not an array.

But there is really one specific case. We can have an array of json(b), and
inside there should be other arrays. So nesting can be across values.

Regards

Pavel



>
> Cheers
>
> On Sun, Feb 7, 2021 at 8:31 AM Joel Jacobson  wrote:
>
>> On Sun, Feb 7, 2021, at 17:27, Tom Lane wrote:
>> >"Joel Jacobson"  writes:
>> >> Having thought about this some more,
>> >> the function name should of course be jsonb_unnest(),
>> >> similar to how unnest() works for normal arrays:
>> >
>> >Why not just unnest(), then?
>> >
>> >regards, tom lane
>>
>> Ahh, of course! I totally forgot about function overloading when thinking
>> about this.
>>
>> +1
>>
>> /Joel
>>
>

Re: jsonb_array_elements_recursive()

[David G. Johnston]

On Sunday, February 7, 2021, Zhihong Yu  wrote:

> Hi,
> # SELECT '[[5,2],"a",[8,[3,2],6]]'::jsonb;
>  jsonb
> ---
>  [[5, 2], "a", [8, [3, 2], 6]]
> (1 row)
>
> unnest(array[[3,2],"a",[1,4]]) is not accepted currently.
>
> Would the enhanced unnest accept the above array ?
>

Its not possible to even create that sql array so whether the unnest
function could do something useful with it is immaterial.

David J.

Re: jsonb_array_elements_recursive()

[Joel Jacobson]

On Sun, Feb 7, 2021, at 18:33, Zhihong Yu wrote:
>Hi,
># SELECT '[[5,2],"a",[8,[3,2],6]]'::jsonb;
> jsonb
>---
> [[5, 2], "a", [8, [3, 2], 6]]
>(1 row)
>
>unnest(array[[3,2],"a",[1,4]]) is not accepted currently.
>
>Would the enhanced unnest accept the above array ?
>
>Cheers

Yes, but only if the overloaded jsonb version of unnest() exists,
and only if it's a jsonb array, not a normal array, like Pavel explained.

Your example using a PoC PL/pgSQL:

CREATE FUNCTION unnest(jsonb)
RETURNS SETOF jsonb
LANGUAGE plpgsql
AS $$
DECLARE
value jsonb;
BEGIN
FOR value IN SELECT jsonb_array_elements($1) LOOP
  IF jsonb_typeof(value) <> 'array' THEN
RETURN NEXT value;
  ELSE
RETURN QUERY
SELECT pit.jsonb_array_elements_recursive(value);
  END IF;
END LOOP;
END
$$;

SELECT unnest('[[5,2],"a",[8,[3,2],6]]'::jsonb);
unnest

5
2
"a"
8
3
2
6
(7 rows)

Cheers,

/Joel

Re: jsonb_array_elements_recursive()

[Joel Jacobson]

On Sun, Feb 7, 2021, at 18:42, Joel Jacobson wrote:
>SELECT pit.jsonb_array_elements_recursive(value);

Sorry, that line should have been:

SELECT unnest(value);

Re: Bug in query rewriter - hasModifyingCTE not getting set

[Tom Lane]

Greg Nancarrow  writes:
> On Sun, Feb 7, 2021 at 10:03 AM Tom Lane  wrote:
>> I think either the bit about rule_action is unnecessary, or most of
>> the code immediately above this is wrong, because it's only updating
>> flags in sub_action.  Why do you think it's necessary to change
>> rule_action in addition to sub_action?

> I believe that the bit about rule_action IS necessary, as it's needed
> for the case of INSERT...SELECT, so that hasModifyingCTE is set on the
> rewritten INSERT (see comment above the call to
> getInsertSelectQuery(), and the "KLUDGE ALERT" comment within that
> function).

Hm.  So after looking at this more, the problem is that the rewrite
is producing something equivalent to

INSERT INTO bug6051_2
(WITH t1 AS (DELETE FROM bug6051 RETURNING *) SELECT * FROM t1);

If you try to do that directly, the parser will give you the raspberry:

ERROR:  WITH clause containing a data-modifying statement must be at the top 
level
LINE 2: (WITH t1 AS (DELETE FROM bug6051 RETURNING *) SELECT * FROM ...
  ^

The code throwing that error, in analyzeCTE(), explains

/*
 * We disallow data-modifying WITH except at the top level of a query,
 * because it's not clear when such a modification should be executed.
 */

That semantic issue doesn't get any less pressing just because the query
was generated by rewrite.  So I now think that what we have to do is
throw an error if we have a modifying CTE and sub_action is different
from rule_action.  Not quite sure how to phrase the error though.

In view of this, maybe the right thing is to disallow modifying CTEs
in rule actions in the first place.  I see we already do that for
views (i.e. ON SELECT rules), but they're not really any safer in
other types of rules.  Given that non-SELECT rules are an undertested
legacy thing, I'm not that excited about moving mountains to make
this case possible.

Anyway, I think I'm going to go revert the patch I crammed in last night.
There's more here than meets the eye, and right before a release is no
time to be fooling with an issue that's been there for years.

regards, tom lane

Re: jsonb_array_elements_recursive()

[Pavel Stehule]

ne 7. 2. 2021 v 18:43 odesílatel Joel Jacobson  napsal:

> On Sun, Feb 7, 2021, at 18:33, Zhihong Yu wrote:
> >Hi,
> ># SELECT '[[5,2],"a",[8,[3,2],6]]'::jsonb;
> > jsonb
> >---
> > [[5, 2], "a", [8, [3, 2], 6]]
> >(1 row)
> >
> >unnest(array[[3,2],"a",[1,4]]) is not accepted currently.
> >
> >Would the enhanced unnest accept the above array ?
> >
> >Cheers
>
> Yes, but only if the overloaded jsonb version of unnest() exists,
> and only if it's a jsonb array, not a normal array, like Pavel explained.
>
> Your example using a PoC PL/pgSQL:
>
> CREATE FUNCTION unnest(jsonb)
> RETURNS SETOF jsonb
> LANGUAGE plpgsql
> AS $$
> DECLARE
> value jsonb;
> BEGIN
> FOR value IN SELECT jsonb_array_elements($1) LOOP
>   IF jsonb_typeof(value) <> 'array' THEN
> RETURN NEXT value;
>   ELSE
> RETURN QUERY
> SELECT pit.jsonb_array_elements_recursive(value);
>   END IF;
> END LOOP;
> END
> $$;
>
> SELECT unnest('[[5,2],"a",[8,[3,2],6]]'::jsonb);
> unnest
> 
> 5
> 2
> "a"
> 8
> 3
> 2
> 6
> (7 rows)
>
> Cheers,
>

just note - isn't it possible to use "not committed yet" function
json_table instead?

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

I understand your request - but I am afraid so we are opening a Pandora box
a little bit. There is a possible collision between Postgres first class
arrays and non atomic types. I am not sure if a functional API is enough to
cover all valuable cases. The functional API is limited and if we cross
some borders, we can get more often errors of type FUNCLOOKUP_AMBIGUOUS. So
if proposed functionality can be implemented by ANSI/SQL dedicated
function, then it can be better. Second possibility is enhancing the
PLpgSQL FOREACH statement. There we have more possibilities to design
necessary syntax, and we don't need to solve possible problems with
handling ambiguous  overloaded functions. I don't afraid of semantics. The
problems can be in parser in function lookup.

Semantically - now the types can support a subscripting interface. There
can be some similarity for type's iterators over nested fields.

Regards

Pavel



> /Joel
>

Re: proposal: enhancing plpgsql debug API - returns text value of variable content

[Pavel Stehule]

Hi

fresh rebase

Regards

Pavel
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index b4c70aaa7f..1bafd202ff 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -4093,6 +4093,8 @@ plpgsql_estate_setup(PLpgSQL_execstate *estate,
 	{
 		(*plpgsql_plugin_ptr)->error_callback = plpgsql_exec_error_callback;
 		(*plpgsql_plugin_ptr)->assign_expr = exec_assign_expr;
+		(*plpgsql_plugin_ptr)->eval_datum = exec_eval_datum;
+		(*plpgsql_plugin_ptr)->cast_value = do_cast_value;
 
 		if ((*plpgsql_plugin_ptr)->func_setup)
 			((*plpgsql_plugin_ptr)->func_setup) (estate, func);
diff --git a/src/pl/plpgsql/src/pl_gram.y b/src/pl/plpgsql/src/pl_gram.y
index 34e0520719..d23b1f5752 100644
--- a/src/pl/plpgsql/src/pl_gram.y
+++ b/src/pl/plpgsql/src/pl_gram.y
@@ -417,6 +417,7 @@ pl_block		: decl_sect K_BEGIN proc_sect exception_sect K_END opt_label
 		new->cmd_type	= PLPGSQL_STMT_BLOCK;
 		new->lineno		= plpgsql_location_to_lineno(@2);
 		new->stmtid		= ++plpgsql_curr_compile->nstatements;
+		new->ns			= plpgsql_ns_top();
 		new->label		= $1.label;
 		new->n_initvars = $1.n_initvars;
 		new->initvarnos = $1.initvarnos;
@@ -911,7 +912,8 @@ stmt_perform	: K_PERFORM
 		new = palloc0(sizeof(PLpgSQL_stmt_perform));
 		new->cmd_type = PLPGSQL_STMT_PERFORM;
 		new->lineno   = plpgsql_location_to_lineno(@1);
-		new->stmtid = ++plpgsql_curr_compile->nstatements;
+		new->stmtid   = ++plpgsql_curr_compile->nstatements;
+		new->ns		  = plpgsql_ns_top();
 		plpgsql_push_back_token(K_PERFORM);
 
 		/*
@@ -947,6 +949,7 @@ stmt_call		: K_CALL
 		new->cmd_type = PLPGSQL_STMT_CALL;
 		new->lineno = plpgsql_location_to_lineno(@1);
 		new->stmtid = ++plpgsql_curr_compile->nstatements;
+		new->ns = plpgsql_ns_top();
 		plpgsql_push_back_token(K_CALL);
 		new->expr = read_sql_stmt();
 		new->is_call = true;
@@ -966,6 +969,7 @@ stmt_call		: K_CALL
 		new->cmd_type = PLPGSQL_STMT_CALL;
 		new->lineno = plpgsql_location_to_lineno(@1);
 		new->stmtid = ++plpgsql_curr_compile->nstatements;
+		new->ns = plpgsql_ns_top();
 		plpgsql_push_back_token(K_DO);
 		new->expr = read_sql_stmt();
 		new->is_call = false;
@@ -1004,7 +1008,8 @@ stmt_assign		: T_DATUM
 		new = palloc0(sizeof(PLpgSQL_stmt_assign));
 		new->cmd_type = PLPGSQL_STMT_ASSIGN;
 		new->lineno   = plpgsql_location_to_lineno(@1);
-		new->stmtid = ++plpgsql_curr_compile->nstatements;
+		new->stmtid   = ++plpgsql_curr_compile->nstatements;
+		new->ns		  = plpgsql_ns_top();
 		new->varno = $1.datum->dno;
 		/* Push back the head name to include it in the stmt */
 		plpgsql_push_back_token(T_DATUM);
@@ -1026,6 +1031,7 @@ stmt_getdiag	: K_GET getdiag_area_opt K_DIAGNOSTICS getdiag_list ';'
 		new->cmd_type = PLPGSQL_STMT_GETDIAG;
 		new->lineno   = plpgsql_location_to_lineno(@1);
 		new->stmtid	  = ++plpgsql_curr_compile->nstatements;
+		new->ns		  = plpgsql_ns_top();
 		new->is_stacked = $2;
 		new->diag_items = $4;
 
@@ -1198,6 +1204,7 @@ stmt_if			: K_IF expr_until_then proc_sect stmt_elsifs stmt_else K_END K_IF ';'
 		new->cmd_type	= PLPGSQL_STMT_IF;
 		new->lineno		= plpgsql_location_to_lineno(@1);
 		new->stmtid		= ++plpgsql_curr_compile->nstatements;
+		new->ns			= plpgsql_ns_top();
 		new->cond		= $2;
 		new->then_body	= $3;
 		new->elsif_list = $4;
@@ -1303,6 +1310,7 @@ stmt_loop		: opt_loop_label K_LOOP loop_body
 		new->cmd_type = PLPGSQL_STMT_LOOP;
 		new->lineno   = plpgsql_location_to_lineno(@2);
 		new->stmtid   = ++plpgsql_curr_compile->nstatements;
+		new->ns		  = plpgsql_ns_top();
 		new->label	  = $1;
 		new->body	  = $3.stmts;
 
@@ -1321,6 +1329,7 @@ stmt_while		: opt_loop_label K_WHILE expr_until_loop loop_body
 		new->cmd_type = PLPGSQL_STMT_WHILE;
 		new->lineno   = plpgsql_location_to_lineno(@2);
 		new->stmtid	  = ++plpgsql_curr_compile->nstatements;
+		new->ns		  = plpgsql_ns_top();
 		new->label	  = $1;
 		new->cond	  = $3;
 		new->body	  = $4.stmts;
@@ -1385,6 +1394,7 @@ for_control		: for_variable K_IN
 			new = palloc0(sizeof(PLpgSQL_stmt_dynfors));
 			new->cmd_type = PLPGSQL_STMT_DYNFORS;
 			new->stmtid	  = ++plpgsql_curr_compile->nstatements;
+			new->ns		  = plpgsql_ns_top();
 			if ($1.row)
 			{
 new->var = (PLpgSQL_variable *) $1.row;
@@ -1431,6 +1441,7 @@ for_control		: for_variable K_IN
 			new = (PLpgSQL_stmt_forc *) palloc0(sizeof(PLpgSQL_stmt_forc));
 			new->cmd_type = PLPGSQL_STMT_FORC;
 			new->stmtid = ++plpgsql_curr_compile->nstatements;
+			new->ns = plpgsql_ns_top();
 			new->curvar = cursor->dno;
 
 			/* Should have had a single variable name */
@@ -1551,6 +1562,7 @@ for_control		: for_variable K_IN
 new = palloc0(sizeof(PLpgSQL_stmt_fori));
 new->cmd_type = PLPGSQL_STMT_

Re: GlobalVisIsRemovableFullXid() vs GlobalVisCheckRemovableXid()

[Peter Geoghegan]

On Sat, Feb 6, 2021 at 7:41 PM Peter Geoghegan  wrote:
> Yes, please do. I could do it myself, but better that you do it
> yourself, just in case.

I went ahead and fixed it myself.

Thanks
-- 
Peter Geoghegan

Re: jsonb_array_elements_recursive()

[Zhihong Yu]

Hi,

bq. SELECT unnest('[[5,2],"a",[8,[3,2],6]]'::jsonb);

Since the array without cast is not normal array (and would be rejected), I
wonder if the cast is needed.
Because casting to jsonb is the only legitimate interpretation here.

Cheers

On Sun, Feb 7, 2021 at 9:42 AM Joel Jacobson  wrote:

> On Sun, Feb 7, 2021, at 18:33, Zhihong Yu wrote:
> >Hi,
> ># SELECT '[[5,2],"a",[8,[3,2],6]]'::jsonb;
> > jsonb
> >---
> > [[5, 2], "a", [8, [3, 2], 6]]
> >(1 row)
> >
> >unnest(array[[3,2],"a",[1,4]]) is not accepted currently.
> >
> >Would the enhanced unnest accept the above array ?
> >
> >Cheers
>
> Yes, but only if the overloaded jsonb version of unnest() exists,
> and only if it's a jsonb array, not a normal array, like Pavel explained.
>
> Your example using a PoC PL/pgSQL:
>
> CREATE FUNCTION unnest(jsonb)
> RETURNS SETOF jsonb
> LANGUAGE plpgsql
> AS $$
> DECLARE
> value jsonb;
> BEGIN
> FOR value IN SELECT jsonb_array_elements($1) LOOP
>   IF jsonb_typeof(value) <> 'array' THEN
> RETURN NEXT value;
>   ELSE
> RETURN QUERY
> SELECT pit.jsonb_array_elements_recursive(value);
>   END IF;
> END LOOP;
> END
> $$;
>
> SELECT unnest('[[5,2],"a",[8,[3,2],6]]'::jsonb);
> unnest
> 
> 5
> 2
> "a"
> 8
> 3
> 2
> 6
> (7 rows)
>
> Cheers,
>
> /Joel
>

Re: jsonb_array_elements_recursive()

[Pavel Stehule]

ne 7. 2. 2021 v 19:18 odesílatel Zhihong Yu  napsal:

> Hi,
>
> bq. SELECT unnest('[[5,2],"a",[8,[3,2],6]]'::jsonb);
>
> Since the array without cast is not normal array (and would be rejected),
> I wonder if the cast is needed.
> Because casting to jsonb is the only legitimate interpretation here.
>

only until somebody does support for hstore, xml, ... some future data type

Minimally now, we have json, jsonb types.

Regards

Pavel

>
> Cheers
>
> On Sun, Feb 7, 2021 at 9:42 AM Joel Jacobson  wrote:
>
>> On Sun, Feb 7, 2021, at 18:33, Zhihong Yu wrote:
>> >Hi,
>> ># SELECT '[[5,2],"a",[8,[3,2],6]]'::jsonb;
>> > jsonb
>> >---
>> > [[5, 2], "a", [8, [3, 2], 6]]
>> >(1 row)
>> >
>> >unnest(array[[3,2],"a",[1,4]]) is not accepted currently.
>> >
>> >Would the enhanced unnest accept the above array ?
>> >
>> >Cheers
>>
>> Yes, but only if the overloaded jsonb version of unnest() exists,
>> and only if it's a jsonb array, not a normal array, like Pavel explained.
>>
>> Your example using a PoC PL/pgSQL:
>>
>> CREATE FUNCTION unnest(jsonb)
>> RETURNS SETOF jsonb
>> LANGUAGE plpgsql
>> AS $$
>> DECLARE
>> value jsonb;
>> BEGIN
>> FOR value IN SELECT jsonb_array_elements($1) LOOP
>>   IF jsonb_typeof(value) <> 'array' THEN
>> RETURN NEXT value;
>>   ELSE
>> RETURN QUERY
>> SELECT pit.jsonb_array_elements_recursive(value);
>>   END IF;
>> END LOOP;
>> END
>> $$;
>>
>> SELECT unnest('[[5,2],"a",[8,[3,2],6]]'::jsonb);
>> unnest
>> 
>> 5
>> 2
>> "a"
>> 8
>> 3
>> 2
>> 6
>> (7 rows)
>>
>> Cheers,
>>
>> /Joel
>>
>

Re: Support tab completion for upper character inputs in psql

[Tom Lane]

"Tang, Haiying"  writes:
> When using psql I found there's no tab completion for upper character inputs. 
> It's really inconvenient sometimes so I try to fix this problem in the 
> attached patch.

This looks like you're trying to force case-insensitive behavior
whether that is appropriate or not.  Does not sound like a good
idea.

regards, tom lane

Re: Key management with tests

[Bruce Momjian]

On Fri, Feb  5, 2021 at 07:53:18PM -0500, Bruce Momjian wrote:
> On Fri, Feb  5, 2021 at 05:21:22PM -0500, Stephen Frost wrote:
> > > I disagree.  If we only warn about some parts, attackers will just
> > > attack other parts.  It will also give users a false sense of security. 
> > > If you can get the keys, it doesn't matter if there is one or ten ways
> > > of getting them, if they are all of equal difficulty.  Same with
> > > modifying the system files.
> > 
> > I agree that there's an additional concern around the keys and that we
> > would want to have a solid way to avoid having them be compromised.  We
> > might not be able to guarantee that attackers who can write to PGDATA
> > can't gain access to the keys in the first implementation, but I don't
> > see that as a problem- the TDE capability would still provide protection
> > against improper disposal and some other use-cases, which is useful.  I
> 
> Agreed.
> 
> > do think it'd be useful to consider how we could provide protection
> > against an attacker who has write access from being able to acquire the
> > keys, but that seems like a tractable problem.  Following that, we could
> > look at how to provide integrity checking for principal data, using one
> > of the outlined approaches or maybe something else entirely.  Lastly,
> > perhaps we can find a way to provide confidentiality and integrity for
> > other parts of the system.
> 
> Yes, we should consider it, and I want to have this discussion.  Ideally
> we could implement that now, because it might be harder later.  However,
> I don't see how we can add additional security protections without
> adding a lot more complexity.  You are right we might have better ideas
> later.

I added a Limitations section so we can consider future improvements:

https://wiki.postgresql.org/wiki/Transparent_Data_Encryption#Limitations

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

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

Re: Bug in query rewriter - hasModifyingCTE not getting set

[Tom Lane]

I wrote:
> That semantic issue doesn't get any less pressing just because the query
> was generated by rewrite.  So I now think that what we have to do is
> throw an error if we have a modifying CTE and sub_action is different
> from rule_action.  Not quite sure how to phrase the error though.

Another idea that'd avoid disallowing functionality is to try to attach
the CTEs to the rule_action not the sub_action.  This'd require adjusting
ctelevelsup in appropriate parts of the parsetree when those are
different, so it seems like it'd be a pain.  I remain unconvinced that
it's worth it.

regards, tom lane

Re: jsonb_array_elements_recursive()

[David G. Johnston]

On Sun, Feb 7, 2021 at 11:39 AM Pavel Stehule 
wrote:

>
>
> ne 7. 2. 2021 v 19:18 odesílatel Zhihong Yu  napsal:
>
>> Hi,
>>
>> bq. SELECT unnest('[[5,2],"a",[8,[3,2],6]]'::jsonb);
>>
>> Since the array without cast is not normal array (and would be rejected),
>> I wonder if the cast is needed.
>> Because casting to jsonb is the only legitimate interpretation here.
>>
>
> only until somebody does support for hstore, xml, ... some future data type
>
> Minimally now, we have json, jsonb types.
>
>
More generally, a sequence of characters has no meaning to the system
unless and until an externally supplied type is given to it allowing it to
interpret the sequence of characters in some concrete way.  The system will
never assign a concrete type to some random sequence of characters based
upon what those characters are.  Forgive the idiom, but to do otherwise
would be putting the cart before the horse.  It would also be quite
expensive and prone to, as above, different types deciding on the same
textual representation being valid input to each.

David J.

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

[John Naylor]

Here is a more polished version of the function pointer approach, now
adapted to all multibyte encodings. Using the not-yet-committed tests from
[1], I found a thinko bug that resulted in the test for nul bytes to not
only be wrong, but probably also elided by the compiler. Doing it correctly
is noticeably slower on pure ascii, but still several times faster than
before, so the conclusions haven't changed any. I'll run full measurements
later this week, but I'll share the patch now for review.

[1]
https://www.postgresql.org/message-id/11d39e63-b80a-5f8d-8043-fff04201f...@iki.fi

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


v1-0001-Add-an-ASCII-fast-path-to-multibyte-encoding-veri.patch
Description: Binary data

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Alvaro Herrera]

On 2021-Feb-07, Mark Rofail wrote:

> Changelog (operator patch):
> > - v1 (compatible with current master 2021-02-05,
> > commit c72af5c202067a9ecb0ff8df7370fb1ea8f4)
> > * add tests and documentation to array operators and gin index
> >
> Since we agreed to separate @>> and <<@ operators to prerequisite patch to
> the FK Arrays, I have rebased the patch to be applied on
> "anyarray_anyelement_operators-vX.patch"

Um, where is that other patch?


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

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Alvaro Herrera]

[Found it :-)]

On 2021-Feb-05, Mark Rofail wrote:

> We will focus on getting the operator patch through first. Should I create
> a separate commitfest ticket? or the current one suffices?
> https://commitfest.postgresql.org/32/2966/

I think the current one is fine.  In fact I would encourage you to post
both patches together as two attachment in the same email; that way, the
CF bot would pick them up correctly.  When you post them in separate
emails, it doesn't know what to do with them.  See here:
http://cfbot.cputube.org/mark-rofail.html

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

Re: [WIP] UNNEST(REFCURSOR): allowing SELECT to consume data from a REFCURSOR

[Dent John]

Hi Massimo,

Thanks for the interest, and my apologies for the late reply.

I’m not particularly abandoning it, but I don’t have particular reason to make 
further changes at the moment. Far as I’m concerned it works, and the main 
question is whether it is acceptable and useful.

I’d be happy if you have feedback that evolves it or might push it up the queue 
for commitfest review.

d.

> On 18 Jan 2021, at 23:09, Massimo Fidanza  wrote:
> 
> This is an interesting feature, but it seems that the author has abandoned 
> development, what happens now? Will this be postponed from commitfest to 
> commitfest and never be taken over by anyone?
> 
> Massimo.
> 
> Il giorno ven 6 mar 2020 alle ore 22:36 Dent John  > ha scritto:
> > On 22 Feb 2020, at 10:38, Dent John  wrote:
> > 
> >> On 18 Feb 2020, at 03:03, Thomas Munro  >> > wrote:
> >> 
> >> From the trivialities department, I see a bunch of warnings about
> >> local declaration placement (we're still using C90 rules for those by
> >> project policy):
> >> 
> >> […]
> > 
> > […]
> 
> My bad. I missed on declaration. 
> 
> Another patch attached.
> 
> d.

Re: Single transaction in the tablesync worker?

[Peter Smith]

On Sun, Feb 7, 2021 at 2:38 PM Peter Smith  wrote:
>
> On Sat, Feb 6, 2021 at 2:10 AM Petr Jelinek
>  wrote:
> >
> > Hi,
> >
> > Some minor comments about code:
> >
> > > + else if (res->status == WALRCV_ERROR && missing_ok)
> > > + {
> > > + /* WARNING. Error, but missing_ok = true. */
> > > + ereport(WARNING,
> >
> > I wonder if we need to add error code to the WalRcvExecResult and check
> > for the appropriate ones here. Because this can for example return error
> > because of timeout, not because slot is missing. Not sure if it matters
> > for current callers though (but then maybe don't call the param
> > missign_ok?).
>
> You are right. The way we are using this function has evolved beyond
> the original intention.
> Probably renaming the param to something like "error_ok" would be more
> appropriate now.
>

PSA a patch (apply on top of V28) to change the misleading param name.


Kind Regards,
Peter Smith.
Fujitsu Australia


v1-0001-ReplicationSlotDropAtPubNode-param.patch
Description: Binary data

Detecting pointer misalignment (was Re: pgsql: Implementation of subscripting for jsonb)

[Tom Lane]

[ redirecting to -hackers ]

Alexander Korotkov  writes:
>> BTW, I managed to reproduce the issue by compiling with CFLAGS="-O0
>> -fsanitize=alignment -fsanitize-trap=alignment" and the patch
>> attached.
>> I can propose the following to catch such issues earlier.  We could
>> finish (wrap attribute with macro and apply it to other places with
>> misalignment access if any) and apply the attached patch and make
>> commitfest.cputube.org check patches with CFLAGS="-O0
>> -fsanitize=alignment -fsanitize-trap=alignment".  What do you think?

> The revised patch is attached.  The attribute is wrapped into
> pg_attribute_no_sanitize_alignment() macro.  I've checked it works for
> me with gcc-10 and clang-11.

I found some time to experiment with this today.  It is really nice
to be able to detect these problems without using obsolete hardware.
However, I have a few issues:

* Why do you recommend -O0?  Seems to me we want to test the code
as we'd normally use it, ie typically -O2.

* -fsanitize-trap=alignment seems to be a clang-ism; gcc won't take it.
However, after some experimenting I found that "-fno-sanitize-recover=all"
(or "-fno-sanitize-recover=alignment" if you prefer) produces roughly
equivalent results on gcc.

* Both clang and gcc seem to be happy with the same spelling of the
function attribute, which is fortunate.  However, I seriously doubt
that bare "#ifdef __GNUC__" is going to be good enough.  At the very
least there's going to need to be a compiler version test in there,
and we might end up needing to get the configure script involved.

* I think the right place to run such a check is in some buildfarm
animals.  The cfbot only sees portions of what goes into our tree.

regards, tom lane

Re: Is Recovery actually paused?

[Yugo NAGATA]

Hi,

On Sun, 7 Feb 2021 19:27:02 +0530
Dilip Kumar  wrote:

> On Sun, Feb 7, 2021 at 6:44 PM Bharath Rupireddy
>  wrote:
> >
> > On Fri, Feb 5, 2021 at 10:14 AM Bharath Rupireddy
> >  wrote:
> > > > We can not do that, basically, under one lock we need to check the
> > > > state and set it to pause.  Because by the time you release the lock
> > > > someone might set it to RECOVERY_NOT_PAUSED then you don't want to set
> > > > it to RECOVERY_PAUSED.
> > >
> > > Got it. Thanks.
> >
> > Hi Dilip, I have one more question:
> >
> > +/* test for recovery pause, if user has requested the pause */
> > +if (((volatile XLogCtlData *) XLogCtl)->recoveryPauseState ==
> > +RECOVERY_PAUSE_REQUESTED)
> > +recoveryPausesHere(false);
> > +
> > +now = GetCurrentTimestamp();
> > +
> >
> > Do we need  now = GetCurrentTimestamp(); here? Because, I see that
> > whenever the variable now is used within the for loop in
> > WaitForWALToBecomeAvailable, it's re-calculated anyways. It's being
> > used within case XLOG_FROM_STREAM:
> >
> > Am I missing something?
> 
> Yeah, I don't see any reason for doing this, maybe it got copy pasted
> by mistake.  Thanks for observing this.

I also have a question:
 
@@ -6270,14 +6291,14 @@ RecoveryRequiresIntParameter(const char *param_name, 
int currValue, int minValue
   currValue,
   minValue)));
 
-   SetRecoveryPause(true);
+   SetRecoveryPause(RECOVERY_PAUSED);
 
ereport(LOG,
(errmsg("recovery has paused"),
 errdetail("If recovery is unpaused, 
the server will shut down."),
 errhint("You can then restart the 
server after making the necessary configuration changes.")));
 
-   while (RecoveryIsPaused())
+   while (GetRecoveryPauseState() != RECOVERY_NOT_PAUSED)
{
HandleStartupProcInterrupts();



If a user call pg_wal_replay_pause while waiting in 
RecoveryRequiresIntParameter,
the state become 'pause requested' and this never returns to 'paused'.
Should we check recoveryPauseState in this loop as in recoveryPausesHere?


Regards,
Yugo Nagata

-- 
Yugo NAGATA 

Re: Detecting pointer misalignment (was Re: pgsql: Implementation of subscripting for jsonb)

[Tom Lane]

I wrote:
> * Both clang and gcc seem to be happy with the same spelling of the
> function attribute, which is fortunate.  However, I seriously doubt
> that bare "#ifdef __GNUC__" is going to be good enough.  At the very
> least there's going to need to be a compiler version test in there,
> and we might end up needing to get the configure script involved.

After digging in gcc's release history, it seems they invented
"-fsanitize=alignment" in GCC 5, so we can make this work for gcc
by writing

#if __GNUC__ >= 5

(the likely() macro already uses a similar approach).  Can't say
if that's close enough for clang too.

regards, tom lane

Re: Is Recovery actually paused?

[Dilip Kumar]

On Mon, 8 Feb 2021 at 6:38 AM, Yugo NAGATA  wrote:

> Hi,
>
> On Sun, 7 Feb 2021 19:27:02 +0530
> Dilip Kumar  wrote:
>
> > On Sun, Feb 7, 2021 at 6:44 PM Bharath Rupireddy
> >  wrote:
> > >
> > > On Fri, Feb 5, 2021 at 10:14 AM Bharath Rupireddy
> > >  wrote:
> > > > > We can not do that, basically, under one lock we need to check the
> > > > > state and set it to pause.  Because by the time you release the
> lock
> > > > > someone might set it to RECOVERY_NOT_PAUSED then you don't want to
> set
> > > > > it to RECOVERY_PAUSED.
> > > >
> > > > Got it. Thanks.
> > >
> > > Hi Dilip, I have one more question:
> > >
> > > +/* test for recovery pause, if user has requested the pause */
> > > +if (((volatile XLogCtlData *) XLogCtl)->recoveryPauseState ==
> > > +RECOVERY_PAUSE_REQUESTED)
> > > +recoveryPausesHere(false);
> > > +
> > > +now = GetCurrentTimestamp();
> > > +
> > >
> > > Do we need  now = GetCurrentTimestamp(); here? Because, I see that
> > > whenever the variable now is used within the for loop in
> > > WaitForWALToBecomeAvailable, it's re-calculated anyways. It's being
> > > used within case XLOG_FROM_STREAM:
> > >
> > > Am I missing something?
> >
> > Yeah, I don't see any reason for doing this, maybe it got copy pasted
> > by mistake.  Thanks for observing this.
>
> I also have a question:
>
> @@ -6270,14 +6291,14 @@ RecoveryRequiresIntParameter(const char
> *param_name, int currValue, int minValue
>currValue,
>minValue)));
>
> -   SetRecoveryPause(true);
> +   SetRecoveryPause(RECOVERY_PAUSED);
>
> ereport(LOG,
> (errmsg("recovery has paused"),
>  errdetail("If recovery is
> unpaused, the server will shut down."),
>  errhint("You can then restart the
> server after making the necessary configuration changes.")));
>
> -   while (RecoveryIsPaused())
> +   while (GetRecoveryPauseState() !=
> RECOVERY_NOT_PAUSED)
> {
> HandleStartupProcInterrupts();
>
>
>
> If a user call pg_wal_replay_pause while waiting in
> RecoveryRequiresIntParameter,
> the state become 'pause requested' and this never returns to 'paused'.
> Should we check recoveryPauseState in this loop as in


I think the right fix should be that the state should never go from
‘paused’ to ‘pause requested’  so I think pg_wal_replay_pause should take
care of that.

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

Re: Single transaction in the tablesync worker?

[Peter Smith]

On Sat, Feb 6, 2021 at 6:30 PM osumi.takami...@fujitsu.com
 wrote:
>
> Hi
>
>
> On Friday, February 5, 2021 5:51 PM Amit Kapila  
> wrote:
> > On Fri, Feb 5, 2021 at 12:36 PM osumi.takami...@fujitsu.com
> >  wrote:
> > >
> > > We need to add some tests to prove the new checks of AlterSubscription()
> > work.
> > > I chose TAP tests as we need to set connect = true for the subscription.
> > > When it can contribute to the development, please utilize this.
> > > I used v28 to check my patch and works as we expect.
> > >
> >
> > Thanks for writing the tests but I don't understand why you need to set
> > connect = true for this test? I have tried below '... with connect = false' 
> > and it
> > seems to be working:
> > postgres=# CREATE SUBSCRIPTION mysub
> > postgres-#  CONNECTION 'host=localhost port=5432
> > dbname=postgres'
> > postgres-# PUBLICATION mypublication WITH (connect = false);
> > WARNING:  tables were not subscribed, you will have to run ALTER
> > SUBSCRIPTION ... REFRESH PUBLICATION to subscribe the tables CREATE
> > SUBSCRIPTION postgres=# Begin; BEGIN postgres=*# Alter Subscription
> > mysub Refresh Publication;
> > ERROR:  ALTER SUBSCRIPTION ... REFRESH is not allowed for disabled
> > subscriptions
> >
> > So, if possible lets write this test in 
> > src/test/regress/sql/subscription.sql.
> OK. I changed the place to write the tests for those.
>
>
> > I have another idea for a test case: What if we write a test such that it 
> > fails PK
> > violation on copy and then drop the subscription. Then check there shouldn't
> > be any dangling slot on the publisher? This is similar to a test in
> > subscription/t/004_sync.pl, we can use some of that framework but have a
> > separate test for this.
> I've added this PK violation test to the attached tests.
> The patch works with v28 and made no failure during regression tests.
>

I checked this patch. It applied cleanly on top of V28, and all tests passed OK.

Here are two feedback comments.

1. For the regression test there is 2 x SQL and 1 x function test. I
thought to cover all the combinations there should be another function
test. e.g.
Tests ALTER … REFRESH
Tests ALTER …. (refresh = true)
Tests ALTER … (refresh = true) in a function
Tests ALTER … REFRESH in a function  <== this combination is not being
testing ??

2. For the 004 test case I know the test is needing some PK constraint violation
# Check if DROP SUBSCRIPTION cleans up slots on the publisher side
# when the subscriber is stuck on data copy for constraint

But it is not clear to me what was the exact cause of that PK
violation. I think you must be relying on data that is leftover from
some previous test case but I am not sure which one. Can you make the
comment more detailed to say *how* the PK violation is happening - e.g
something to say which rows, in which table, and inserted by who?

--
Kind Regards,
Peter Smith.
Fujitsu Australia

Re: Is Recovery actually paused?

[Yugo NAGATA]

On Mon, 8 Feb 2021 07:51:22 +0530
Dilip Kumar  wrote:

> On Mon, 8 Feb 2021 at 6:38 AM, Yugo NAGATA  wrote:
> 
> > Hi,
> >
> > On Sun, 7 Feb 2021 19:27:02 +0530
> > Dilip Kumar  wrote:
> >
> > > On Sun, Feb 7, 2021 at 6:44 PM Bharath Rupireddy
> > >  wrote:
> > > >
> > > > On Fri, Feb 5, 2021 at 10:14 AM Bharath Rupireddy
> > > >  wrote:
> > > > > > We can not do that, basically, under one lock we need to check the
> > > > > > state and set it to pause.  Because by the time you release the
> > lock
> > > > > > someone might set it to RECOVERY_NOT_PAUSED then you don't want to
> > set
> > > > > > it to RECOVERY_PAUSED.
> > > > >
> > > > > Got it. Thanks.
> > > >
> > > > Hi Dilip, I have one more question:
> > > >
> > > > +/* test for recovery pause, if user has requested the pause */
> > > > +if (((volatile XLogCtlData *) XLogCtl)->recoveryPauseState ==
> > > > +RECOVERY_PAUSE_REQUESTED)
> > > > +recoveryPausesHere(false);
> > > > +
> > > > +now = GetCurrentTimestamp();
> > > > +
> > > >
> > > > Do we need  now = GetCurrentTimestamp(); here? Because, I see that
> > > > whenever the variable now is used within the for loop in
> > > > WaitForWALToBecomeAvailable, it's re-calculated anyways. It's being
> > > > used within case XLOG_FROM_STREAM:
> > > >
> > > > Am I missing something?
> > >
> > > Yeah, I don't see any reason for doing this, maybe it got copy pasted
> > > by mistake.  Thanks for observing this.
> >
> > I also have a question:
> >
> > @@ -6270,14 +6291,14 @@ RecoveryRequiresIntParameter(const char
> > *param_name, int currValue, int minValue
> >currValue,
> >minValue)));
> >
> > -   SetRecoveryPause(true);
> > +   SetRecoveryPause(RECOVERY_PAUSED);
> >
> > ereport(LOG,
> > (errmsg("recovery has paused"),
> >  errdetail("If recovery is
> > unpaused, the server will shut down."),
> >  errhint("You can then restart the
> > server after making the necessary configuration changes.")));
> >
> > -   while (RecoveryIsPaused())
> > +   while (GetRecoveryPauseState() !=
> > RECOVERY_NOT_PAUSED)
> > {
> > HandleStartupProcInterrupts();
> >
> >
> >
> > If a user call pg_wal_replay_pause while waiting in
> > RecoveryRequiresIntParameter,
> > the state become 'pause requested' and this never returns to 'paused'.
> > Should we check recoveryPauseState in this loop as in
> 
> 
> I think the right fix should be that the state should never go from
> ‘paused’ to ‘pause requested’  so I think pg_wal_replay_pause should take
> care of that.

It makes sense to take care of this in pg_wal_replay_pause, but I wonder
it can not handle the case that a user resume and pause again while a sleep.


-- 
Yugo NAGATA 

Re: parse mistake in ecpg connect string

[Kyotaro Horiguchi]

At Thu, 4 Feb 2021 09:25:00 +, "Wang, Shenhao"  
wrote in 
> Hi, hacker
> 
> I found in function ECPGconnect, the connect string in comment is written as:
> 
> /*--
>  * new style:
>  *:postgresql://server[:port|:/unixsocket/path:]
>  *[/db-name][?options]
>  *--
> */
> 
> But, the parse logical seems wrong, like:

Actually it looks like broken, but..
  
> [1] https://www.postgresql.org/docs/13/ecpg-connect.html#ECPG-CONNECTING

The comment and related code seem to be remnants of an ancient syntax
of hostname/socket-path style, which should have been cleaned up in
2000. I guess that the tcp: and unix: style target remains just for
backward compatibility, I'm not sure, though.  Nowadays you can do
that by using the "dbname[@hostname][:port]" style target.

EXEC SQL CONNECT TO 'postgres@/tmp:5432';
EXEC SQL CONNECT TO 'unix:postgresql://localhost:5432/postgres?host=/tmp';

FWIW, directly embedding /unixsocket/path syntax in a URL is broken in
the view of URI. It is the reason why the current connection URI takes
the way shown above. So I think we want to remove that code rather
than to fix it.

And, since the documentation is saying that the bare target
specification is somewhat unstable, I'm not sure we dare to *fix* the
ecpg syntax.

In [1]
> In practice, it is probably less error-prone to use a (single-quoted)
> string literal or a variable reference.

That being said, we might need a description about how we can specify
a unix socket directory in ecpg-connect.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/interfaces/ecpg/ecpglib/connect.c b/src/interfaces/ecpg/ecpglib/connect.c
index 6b0a3067e6..f45892304d 100644
--- a/src/interfaces/ecpg/ecpglib/connect.c
+++ b/src/interfaces/ecpg/ecpglib/connect.c
@@ -360,8 +360,7 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
 
 /*--
  * new style:
- *	:postgresql://server[:port|:/unixsocket/path:]
- *	[/db-name][?options]
+ *	:postgresql://server[:port][/db-name][?options]
  *--
  */
 offset += strlen("postgresql://");
@@ -385,41 +384,11 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
 }
 
 tmp = strrchr(dbname + offset, ':');
-if (tmp != NULL)	/* port number or Unix socket path given */
+if (tmp != NULL)	/* port number given */
 {
-	char	   *tmp2;
-
 	*tmp = '\0';
-	if ((tmp2 = strchr(tmp + 1, ':')) != NULL)
-	{
-		*tmp2 = '\0';
-		host = ecpg_strdup(tmp + 1, lineno);
-		connect_params++;
-		if (strncmp(dbname, "unix:", 5) != 0)
-		{
-			ecpg_log("ECPGconnect: socketname %s given for TCP connection on line %d\n", host, lineno);
-			ecpg_raise(lineno, ECPG_CONNECT, ECPG_SQLSTATE_SQLCLIENT_UNABLE_TO_ESTABLISH_SQLCONNECTION, realname ? realname : ecpg_gettext(""));
-			if (host)
-ecpg_free(host);
-
-			/*
-			 * port not set yet if (port) ecpg_free(port);
-			 */
-			if (options)
-ecpg_free(options);
-			if (realname)
-ecpg_free(realname);
-			if (dbname)
-ecpg_free(dbname);
-			free(this);
-			return false;
-		}
-	}
-	else
-	{
-		port = ecpg_strdup(tmp + 1, lineno);
-		connect_params++;
-	}
+	port = ecpg_strdup(tmp + 1, lineno);
+	connect_params++;
 }
 
 if (strncmp(dbname, "unix:", 5) == 0)

RE: parse mistake in ecpg connect string

[kuroda.hay...@fujitsu.com]

Dear Wang,

> the value tmp2 will always be NULL, the unix-socket path will be ignored.

I confirmed it, you're right.

> I have fixed this problem, the patch attached.

It looks good to me:-).

> I will try to fix this problem later, but it seems a little difficult to add 
> some lex/bison file rules

I think rule `connection_target` must be fixed.
Either port or unix_socket_directory can be accepted now(I followed the 
comment),
but we should discuss about it.
According to the doc, libpq's connection-URI accept both.

The attached patch contains all fixes, and pass test in my environment.
And the following line:

  EXEC SQL CONNECT TO unix:postgresql://localhost:/a:/postgres;

is precompiled to:

  { ECPGconnect(__LINE__, 0, "unix:postgresql://localhost:/a:/postgres" , NULL, 
NULL , NULL, 0); }

Is it OK?

Best Regards,
Hayato Kuroda
FUJITSU LIMITED



v2-0001-fix-strchr-strrchr-mistake.patch
Description: v2-0001-fix-strchr-strrchr-mistake.patch

RE: parse mistake in ecpg connect string

[kuroda.hay...@fujitsu.com]

Dear Horiguchi-san,

My response crossed in the e-mail with yours. Sorry.

> FWIW, directly embedding /unixsocket/path syntax in a URL is broken in
> the view of URI. It is the reason why the current connection URI takes
> the way shown above. So I think we want to remove that code rather
> than to fix it.

I didn't know such a phenomenon. If other codes follow the rule,
I agree yours.

Digress from the main topic, but the following cannot be accepted for the 
precompiler.
This should be fixed, isn't it?

EXEC SQL CONNECT TO postgres@/tmp:5432;

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

Re: Single transaction in the tablesync worker?

[Amit Kapila]

On Mon, Feb 8, 2021 at 8:06 AM Peter Smith  wrote:
>
> On Sat, Feb 6, 2021 at 6:30 PM osumi.takami...@fujitsu.com
>  wrote:
> >
> > > I have another idea for a test case: What if we write a test such that it 
> > > fails PK
> > > violation on copy and then drop the subscription. Then check there 
> > > shouldn't
> > > be any dangling slot on the publisher? This is similar to a test in
> > > subscription/t/004_sync.pl, we can use some of that framework but have a
> > > separate test for this.
> > I've added this PK violation test to the attached tests.
> > The patch works with v28 and made no failure during regression tests.
> >
>
> I checked this patch. It applied cleanly on top of V28, and all tests passed 
> OK.
>
> Here are two feedback comments.
>
> 1. For the regression test there is 2 x SQL and 1 x function test. I
> thought to cover all the combinations there should be another function
> test. e.g.
> Tests ALTER … REFRESH
> Tests ALTER …. (refresh = true)
> Tests ALTER … (refresh = true) in a function
> Tests ALTER … REFRESH in a function  <== this combination is not being
> testing ??
>

I am not sure whether there is much value in adding more to this set
of negative test cases unless it really covers a different code path
which I think won't happen if we add more tests here.

-- 
With Regards,
Amit Kapila.

Re: About to add WAL write/fsync statistics to pg_stat_wal view

[Fujii Masao]

On 2021/02/05 8:45, Masahiro Ikeda wrote:

I pgindented the patches.


Thanks for updating the patches!

+   XLogWrite, which nomally called by an
+   issue_xlog_fsync, which nomally called by an

Typo: "nomally" should be "normally"?

+   XLogFlush request(see )
+   XLogFlush request(see ),

Isn't it better to add a space character just after "request"?

+   INSTR_TIME_SET_CURRENT(duration);
+   INSTR_TIME_SUBTRACT(duration, start);
+   WalStats.m_wal_write_time = 
INSTR_TIME_GET_MICROSEC(duration);

If several cycles happen in the do-while loop, m_wal_write_time should be
updated with the sum of "duration" in those cycles instead of "duration"
in the last cycle? If yes, "+=" should be used instead of "=" when updating
m_wal_write_time?

+   INSTR_TIME_SET_CURRENT(duration);
+   INSTR_TIME_SUBTRACT(duration, start);
+   WalStats.m_wal_sync_time = 
INSTR_TIME_GET_MICROSEC(duration);

Also "=" should be "+=" in the above?

Regards,


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

Re: Is Recovery actually paused?

[Dilip Kumar]

On Mon, Feb 8, 2021 at 8:18 AM Yugo NAGATA  wrote:
>
> On Mon, 8 Feb 2021 07:51:22 +0530
> Dilip Kumar  wrote:
>
> > On Mon, 8 Feb 2021 at 6:38 AM, Yugo NAGATA  wrote:
> >
> > > Hi,
> > >
> > > On Sun, 7 Feb 2021 19:27:02 +0530
> > > Dilip Kumar  wrote:
> > >
> > > > On Sun, Feb 7, 2021 at 6:44 PM Bharath Rupireddy
> > > >  wrote:
> > > > >
> > > > > On Fri, Feb 5, 2021 at 10:14 AM Bharath Rupireddy
> > > > >  wrote:
> > > > > > > We can not do that, basically, under one lock we need to check the
> > > > > > > state and set it to pause.  Because by the time you release the
> > > lock
> > > > > > > someone might set it to RECOVERY_NOT_PAUSED then you don't want to
> > > set
> > > > > > > it to RECOVERY_PAUSED.
> > > > > >
> > > > > > Got it. Thanks.
> > > > >
> > > > > Hi Dilip, I have one more question:
> > > > >
> > > > > +/* test for recovery pause, if user has requested the pause 
> > > > > */
> > > > > +if (((volatile XLogCtlData *) XLogCtl)->recoveryPauseState ==
> > > > > +RECOVERY_PAUSE_REQUESTED)
> > > > > +recoveryPausesHere(false);
> > > > > +
> > > > > +now = GetCurrentTimestamp();
> > > > > +
> > > > >
> > > > > Do we need  now = GetCurrentTimestamp(); here? Because, I see that
> > > > > whenever the variable now is used within the for loop in
> > > > > WaitForWALToBecomeAvailable, it's re-calculated anyways. It's being
> > > > > used within case XLOG_FROM_STREAM:
> > > > >
> > > > > Am I missing something?
> > > >
> > > > Yeah, I don't see any reason for doing this, maybe it got copy pasted
> > > > by mistake.  Thanks for observing this.
> > >
> > > I also have a question:
> > >
> > > @@ -6270,14 +6291,14 @@ RecoveryRequiresIntParameter(const char
> > > *param_name, int currValue, int minValue
> > >currValue,
> > >minValue)));
> > >
> > > -   SetRecoveryPause(true);
> > > +   SetRecoveryPause(RECOVERY_PAUSED);
> > >
> > > ereport(LOG,
> > > (errmsg("recovery has paused"),
> > >  errdetail("If recovery is
> > > unpaused, the server will shut down."),
> > >  errhint("You can then restart the
> > > server after making the necessary configuration changes.")));
> > >
> > > -   while (RecoveryIsPaused())
> > > +   while (GetRecoveryPauseState() !=
> > > RECOVERY_NOT_PAUSED)
> > > {
> > > HandleStartupProcInterrupts();
> > >
> > >
> > >
> > > If a user call pg_wal_replay_pause while waiting in
> > > RecoveryRequiresIntParameter,
> > > the state become 'pause requested' and this never returns to 'paused'.
> > > Should we check recoveryPauseState in this loop as in
> >
> >
> > I think the right fix should be that the state should never go from
> > ‘paused’ to ‘pause requested’  so I think pg_wal_replay_pause should take
> > care of that.
>
> It makes sense to take care of this in pg_wal_replay_pause, but I wonder
> it can not handle the case that a user resume and pause again while a sleep.

Right, we will have to check and set in the loop.  But we should not
allow the state to go from paused to pause requested irrespective of
this.

I will make these changes and send the updated patch.

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

Re: Is Recovery actually paused?

[Bharath Rupireddy]

On Mon, Feb 8, 2021 at 9:35 AM Dilip Kumar  wrote:
> > > > If a user call pg_wal_replay_pause while waiting in
> > > > RecoveryRequiresIntParameter,
> > > > the state become 'pause requested' and this never returns to 'paused'.
> > > > Should we check recoveryPauseState in this loop as in
> > >
> > >
> > > I think the right fix should be that the state should never go from
> > > ‘paused’ to ‘pause requested’  so I think pg_wal_replay_pause should take
> > > care of that.
> >
> > It makes sense to take care of this in pg_wal_replay_pause, but I wonder
> > it can not handle the case that a user resume and pause again while a sleep.
>
> Right, we will have to check and set in the loop.  But we should not
> allow the state to go from paused to pause requested irrespective of
> this.

We can think of a state machine with the states "not paused", "pause
requested", "paused". While we can go to "not paused" from any state,
but cannot go to "pause requested" from "paused".

So, will pg_wal_replay_pause throw an error or warning or silently
return when it's  called and the state is "paused" already? Maybe we
should add better commenting in pg_wal_replay_pause why we don't set
"pause requested" when the state is already "paused".

And also, if we are adding below code in the
RecoveryRequiresIntParameter loop, it's better to make it a function,
like your earlier patch.

/*
 * If recovery pause is requested then set it paused.  While we are in
 * the loop, user might resume and pause again so set this every time.
 */
SpinLockAcquire(&XLogCtl->info_lck);
if (XLogCtl->recoveryPauseState == RECOVERY_PAUSE_REQUESTED)
XLogCtl->recoveryPauseState = RECOVERY_PAUSED;
SpinLockRelease(&XLogCtl->info_lck);

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

Re: Is Recovery actually paused?

[Dilip Kumar]

On Mon, Feb 8, 2021 at 9:49 AM Bharath Rupireddy
 wrote:
>
> On Mon, Feb 8, 2021 at 9:35 AM Dilip Kumar  wrote:
> > > > > If a user call pg_wal_replay_pause while waiting in
> > > > > RecoveryRequiresIntParameter,
> > > > > the state become 'pause requested' and this never returns to 'paused'.
> > > > > Should we check recoveryPauseState in this loop as in
> > > >
> > > >
> > > > I think the right fix should be that the state should never go from
> > > > ‘paused’ to ‘pause requested’  so I think pg_wal_replay_pause should 
> > > > take
> > > > care of that.
> > >
> > > It makes sense to take care of this in pg_wal_replay_pause, but I wonder
> > > it can not handle the case that a user resume and pause again while a 
> > > sleep.
> >
> > Right, we will have to check and set in the loop.  But we should not
> > allow the state to go from paused to pause requested irrespective of
> > this.
>
> We can think of a state machine with the states "not paused", "pause
> requested", "paused". While we can go to "not paused" from any state,
> but cannot go to "pause requested" from "paused".
>
> So, will pg_wal_replay_pause throw an error or warning or silently
 > return when it's  called and the state is "paused" already?

It should just silently return because pg_wal_replay_pause just claim
it request to pause, but it not mean that it can not pause
immediately.

 Maybe we
> should add better commenting in pg_wal_replay_pause why we don't set
> "pause requested" when the state is already "paused".



> And also, if we are adding below code in the
> RecoveryRequiresIntParameter loop, it's better to make it a function,
> like your earlier patch.
>
> /*
>  * If recovery pause is requested then set it paused.  While we are in
>  * the loop, user might resume and pause again so set this every time.
>  */
> SpinLockAcquire(&XLogCtl->info_lck);
> if (XLogCtl->recoveryPauseState == RECOVERY_PAUSE_REQUESTED)
> XLogCtl->recoveryPauseState = RECOVERY_PAUSED;
> SpinLockRelease(&XLogCtl->info_lck);

Yes, it should go back to function now as in the older versions.



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

RE: Single transaction in the tablesync worker?

[osumi.takami...@fujitsu.com]

Hello


On Mon, Feb 8, 2021 12:40 PM Amit Kapila  wrote:
> On Mon, Feb 8, 2021 at 8:06 AM Peter Smith 
> wrote:
> >
> > On Sat, Feb 6, 2021 at 6:30 PM osumi.takami...@fujitsu.com
> >  wrote:
> > >
> > > > I have another idea for a test case: What if we write a test such
> > > > that it fails PK violation on copy and then drop the subscription.
> > > > Then check there shouldn't be any dangling slot on the publisher?
> > > > This is similar to a test in subscription/t/004_sync.pl, we can
> > > > use some of that framework but have a separate test for this.
> > > I've added this PK violation test to the attached tests.
> > > The patch works with v28 and made no failure during regression tests.
> > >
> >
> > I checked this patch. It applied cleanly on top of V28, and all tests passed
> OK.
> >
> > Here are two feedback comments.
> >
> > 1. For the regression test there is 2 x SQL and 1 x function test. I
> > thought to cover all the combinations there should be another function
> > test. e.g.
> > Tests ALTER … REFRESH
> > Tests ALTER …. (refresh = true)
> > Tests ALTER … (refresh = true) in a function Tests ALTER … REFRESH in
> > a function  <== this combination is not being testing ??
> >
> 
> I am not sure whether there is much value in adding more to this set of
> negative test cases unless it really covers a different code path which I 
> think
> won't happen if we add more tests here.
Yeah, I agree. Accordingly, I didn't fix that part.


On Mon, Feb 8, 2021 11:36 AM Peter Smith  wrote:
> 2. For the 004 test case I know the test is needing some PK constraint
> violation # Check if DROP SUBSCRIPTION cleans up slots on the publisher
> side # when the subscriber is stuck on data copy for constraint
> 
> But it is not clear to me what was the exact cause of that PK violation. I 
> think
> you must be relying on data that is leftover from some previous test case but
> I am not sure which one. Can you make the comment more detailed to say
> *how* the PK violation is happening - e.g something to say which rows, in
> which table, and inserted by who?
I added some comments to clarify how the PK violation happens.
Please have a look.


Best Regards,
Takamichi Osumi


refresh_and_pk_violation_testsets_v02.patch
Description: refresh_and_pk_violation_testsets_v02.patch

Re: Is it useful to record whether plans are generic or custom?

[torikoshia]

On 2021-02-04 11:19, Kyotaro Horiguchi wrote:

At Thu, 04 Feb 2021 10:16:47 +0900, torikoshia
 wrote in

Chengxi Sun, Yamada-san, Horiguchi-san,

Thanks for all your comments.
Adding only the number of generic plan execution seems acceptable.

On Mon, Jan 25, 2021 at 2:10 PM Kyotaro Horiguchi
 wrote:
> Note that ActivePortal is the closest nested portal. So it gives the
> wrong result for nested portals.

I may be wrong, but I thought it was ok since the closest nested
portal is the portal to be executed.


After executing the inner-most portal, is_plan_type_generic has a
value for the inner-most portal and it won't be changed ever after. At
the ExecutorEnd of all the upper-portals see the value for the
inner-most portal left behind is_plan_type_generic nevertheless the
portals at every nest level are independent.


ActivePortal is used in ExecutorStart hook in the patch.
And as far as I read PortalStart(), ActivePortal is changed to the
portal to be executed before ExecutorStart().

If possible, could you tell me the specific case which causes wrong
results?


Running a plpgsql function that does PREPRE in a query that does
PREPARE?


Thanks for your explanation!

I confirmed that it in fact happened.

To avoid it, attached patch preserves the is_plan_type_generic before 
changing it and sets it back at the end of pgss_ExecutorEnd().


Any thoughts?


Regards,

--
Atsushi Torikoshidiff --git a/contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql b/contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql
index 0f63f08f7e..7fdef315ae 100644
--- a/contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql
+++ b/contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql
@@ -44,7 +44,8 @@ CREATE FUNCTION pg_stat_statements(IN showtext boolean,
 OUT blk_write_time float8,
 OUT wal_records int8,
 OUT wal_fpi int8,
-OUT wal_bytes numeric
+OUT wal_bytes numeric,
+OUT generic_calls int8
 )
 RETURNS SETOF record
 AS 'MODULE_PATHNAME', 'pg_stat_statements_1_8'
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 62cccbfa44..f5801016d6 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -77,10 +77,12 @@
 #include "storage/fd.h"
 #include "storage/ipc.h"
 #include "storage/spin.h"
+#include "tcop/pquery.h"
 #include "tcop/utility.h"
 #include "utils/acl.h"
 #include "utils/builtins.h"
 #include "utils/memutils.h"
+#include "utils/plancache.h"
 #include "utils/timestamp.h"
 
 PG_MODULE_MAGIC;
@@ -192,6 +194,7 @@ typedef struct Counters
 	int64		wal_records;	/* # of WAL records generated */
 	int64		wal_fpi;		/* # of WAL full page images generated */
 	uint64		wal_bytes;		/* total amount of WAL generated in bytes */
+	int64		generic_calls;	/* # of times generic plans executed */
 } Counters;
 
 /*
@@ -277,6 +280,10 @@ static int	exec_nested_level = 0;
 /* Current nesting depth of planner calls */
 static int	plan_nested_level = 0;
 
+/* Current and previous plan type */
+static bool	is_plan_type_generic = false;
+static bool	is_prev_plan_type_generic = false;
+
 /* Saved hook values in case of unload */
 static shmem_startup_hook_type prev_shmem_startup_hook = NULL;
 static post_parse_analyze_hook_type prev_post_parse_analyze_hook = NULL;
@@ -1034,6 +1041,23 @@ pgss_ExecutorStart(QueryDesc *queryDesc, int eflags)
 	 */
 	if (pgss_enabled(exec_nested_level) && queryDesc->plannedstmt->queryId != UINT64CONST(0))
 	{
+		/*
+		 * Since ActivePortal is not available at ExecutorEnd, we preserve
+		 * the current and previous plan type here.
+		 * Previous one is necessary since portals can be nested.
+		 */
+		Assert(ActivePortal);
+
+		is_prev_plan_type_generic = is_plan_type_generic;
+
+		if (ActivePortal->cplan)
+		{
+			if (ActivePortal->cplan->is_generic)
+is_plan_type_generic = true;
+			else
+is_plan_type_generic = false;
+		}
+
 		/*
 		 * Set up to track total elapsed time in ExecutorRun.  Make sure the
 		 * space is allocated in the per-query context so it will go away at
@@ -1122,6 +1146,9 @@ pgss_ExecutorEnd(QueryDesc *queryDesc)
    NULL);
 	}
 
+	/* Storing has done. Set is_plan_type_generic back to the original. */
+	is_plan_type_generic = is_prev_plan_type_generic;
+
 	if (prev_ExecutorEnd)
 		prev_ExecutorEnd(queryDesc);
 	else
@@ -1427,6 +1454,8 @@ pgss_store(const char *query, uint64 queryId,
 			e->counters.max_time[kind] = total_time;
 			e->counters.mean_time[kind] = total_time;
 		}
+		else if (kind == PGSS_EXEC && is_plan_type_generic)
+			e->counters.generic_calls += 1;
 		else
 		{
 			/*
@@ -1510,8 +1539,8 @@ pg_stat_statements_reset(PG_FUNCTION_ARGS)
 #define PG_STAT_STATEMENTS_COLS_V1_1	18
 #define PG_STAT_STATEMENTS_COLS_V1_2	19
 #define PG_STAT_STATEMENTS_COLS_V1_3	23
-#define PG_STAT_STATEMENTS_COLS_V1_8	32
-#define PG_STAT_STATEMENTS_COLS			32	/* maximum of above */
+#define PG_STAT_STATEMENTS_COLS_V1_8	33
+#define PG_

Re: Is Recovery actually paused?

[Yugo NAGATA]

On Mon, 8 Feb 2021 09:35:00 +0530
Dilip Kumar  wrote:

> On Mon, Feb 8, 2021 at 8:18 AM Yugo NAGATA  wrote:
> >
> > On Mon, 8 Feb 2021 07:51:22 +0530
> > Dilip Kumar  wrote:
> >
> > > On Mon, 8 Feb 2021 at 6:38 AM, Yugo NAGATA  wrote:
> > >
> > > > Hi,
> > > >
> > > > On Sun, 7 Feb 2021 19:27:02 +0530
> > > > Dilip Kumar  wrote:
> > > >
> > > > > On Sun, Feb 7, 2021 at 6:44 PM Bharath Rupireddy
> > > > >  wrote:
> > > > > >
> > > > > > On Fri, Feb 5, 2021 at 10:14 AM Bharath Rupireddy
> > > > > >  wrote:
> > > > > > > > We can not do that, basically, under one lock we need to check 
> > > > > > > > the
> > > > > > > > state and set it to pause.  Because by the time you release the
> > > > lock
> > > > > > > > someone might set it to RECOVERY_NOT_PAUSED then you don't want 
> > > > > > > > to
> > > > set
> > > > > > > > it to RECOVERY_PAUSED.
> > > > > > >
> > > > > > > Got it. Thanks.
> > > > > >
> > > > > > Hi Dilip, I have one more question:
> > > > > >
> > > > > > +/* test for recovery pause, if user has requested the 
> > > > > > pause */
> > > > > > +if (((volatile XLogCtlData *) XLogCtl)->recoveryPauseState 
> > > > > > ==
> > > > > > +RECOVERY_PAUSE_REQUESTED)
> > > > > > +recoveryPausesHere(false);
> > > > > > +
> > > > > > +now = GetCurrentTimestamp();
> > > > > > +
> > > > > >
> > > > > > Do we need  now = GetCurrentTimestamp(); here? Because, I see that
> > > > > > whenever the variable now is used within the for loop in
> > > > > > WaitForWALToBecomeAvailable, it's re-calculated anyways. It's being
> > > > > > used within case XLOG_FROM_STREAM:
> > > > > >
> > > > > > Am I missing something?
> > > > >
> > > > > Yeah, I don't see any reason for doing this, maybe it got copy pasted
> > > > > by mistake.  Thanks for observing this.
> > > >
> > > > I also have a question:
> > > >
> > > > @@ -6270,14 +6291,14 @@ RecoveryRequiresIntParameter(const char
> > > > *param_name, int currValue, int minValue
> > > >currValue,
> > > >minValue)));
> > > >
> > > > -   SetRecoveryPause(true);
> > > > +   SetRecoveryPause(RECOVERY_PAUSED);
> > > >
> > > > ereport(LOG,
> > > > (errmsg("recovery has paused"),
> > > >  errdetail("If recovery is
> > > > unpaused, the server will shut down."),
> > > >  errhint("You can then restart 
> > > > the
> > > > server after making the necessary configuration changes.")));
> > > >
> > > > -   while (RecoveryIsPaused())
> > > > +   while (GetRecoveryPauseState() !=
> > > > RECOVERY_NOT_PAUSED)
> > > > {
> > > > HandleStartupProcInterrupts();
> > > >
> > > >
> > > >
> > > > If a user call pg_wal_replay_pause while waiting in
> > > > RecoveryRequiresIntParameter,
> > > > the state become 'pause requested' and this never returns to 'paused'.
> > > > Should we check recoveryPauseState in this loop as in
> > >
> > >
> > > I think the right fix should be that the state should never go from
> > > ‘paused’ to ‘pause requested’  so I think pg_wal_replay_pause should take
> > > care of that.
> >
> > It makes sense to take care of this in pg_wal_replay_pause, but I wonder
> > it can not handle the case that a user resume and pause again while a sleep.
> 
> Right, we will have to check and set in the loop.  But we should not
> allow the state to go from paused to pause requested irrespective of
> this.

I agree with you.

-- 
Yugo NAGATA 

Re: About to add WAL write/fsync statistics to pg_stat_wal view

[Fujii Masao]

On 2021/02/08 13:01, Fujii Masao wrote:



On 2021/02/05 8:45, Masahiro Ikeda wrote:

I pgindented the patches.


Thanks for updating the patches!

+   XLogWrite, which nomally called by an
+   issue_xlog_fsync, which nomally called by an

Typo: "nomally" should be "normally"?

+   XLogFlush request(see )
+   XLogFlush request(see ),

Isn't it better to add a space character just after "request"?

+    INSTR_TIME_SET_CURRENT(duration);
+    INSTR_TIME_SUBTRACT(duration, start);
+    WalStats.m_wal_write_time = 
INSTR_TIME_GET_MICROSEC(duration);

If several cycles happen in the do-while loop, m_wal_write_time should be
updated with the sum of "duration" in those cycles instead of "duration"
in the last cycle? If yes, "+=" should be used instead of "=" when updating
m_wal_write_time?

+    INSTR_TIME_SET_CURRENT(duration);
+    INSTR_TIME_SUBTRACT(duration, start);
+    WalStats.m_wal_sync_time = INSTR_TIME_GET_MICROSEC(duration);

Also "=" should be "+=" in the above?


+   /* Send WAL statistics */
+   pgstat_send_wal();

This may cause overhead in WAL-writing by walwriter because it's called
every cycles even when walwriter needs to write more WAL next cycle
(don't need to sleep on WaitLatch)? If this is right, pgstat_send_wal()
should be called only when WaitLatch() returns with WL_TIMEOUT?

-   XLogFlush request(see )
+   XLogFlush request(see ),
+   or WAL data written out to disk by WAL receiver.

So regarding walreceiver, only wal_write, wal_write_time, wal_sync, and
wal_sync_time are updated even while the other values are not. Isn't this
confusing to users? If so, what about reporting those walreceiver stats in
pg_stat_wal_receiver?

if (endofwal)
+   {
+   /* Send WAL statistics to the stats 
collector */
+   pgstat_send_wal();
break;

You added pgstat_send_wal() so that it's called in some cases where
walreceiver exits. But ISTM that there are other walreceiver-exit cases.
For example, in the case where SIGTERM is received. Instead,
pgstat_send_wal() should be called in WalRcvDie() for those all cases?

Regards,

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

Re: pg_replication_origin_drop API potential race condition

[Amit Kapila]

On Sat, Feb 6, 2021 at 5:47 PM Amit Kapila  wrote:
>
> On Sat, Feb 6, 2021 at 3:26 PM Petr Jelinek  wrote:
> >
> > On 06/02/2021 07:29, Amit Kapila wrote:
> > > On Fri, Feb 5, 2021 at 6:45 PM Euler Taveira  wrote:
> > >> - replorigin_drop(roident, true);
> > >> + replorigin_drop_by_name(name, false /* missing_ok */ , true /* nowait 
> > >> */ );
> > >>
> > >> A modern IDE would certainly show you the function definition that 
> > >> allows you
> > >> to check what each parameter value is without having to go back and 
> > >> forth. I
> > >> saw a few occurrences of this pattern in the source code and IMO it 
> > >> could be
> > >> used when it is not obvious what that value means. Booleans are easier to
> > >> figure out, however, sometimes integer and text are not.
> > >>
> > > Fair enough, removed in the attached patch.
> >
> >
> > To be fair the logical replication framework is full of these comments
> > so it's pretty natural to add them to new code as well, but I agree with
> > Euler that it's unnecessary with any reasonable development tooling.
> >
> > The patch as posted looks good to me,
> >
>
> Thanks, but today again testing this API, I observed that we can still
> get "tuple concurrently deleted" because we are releasing the lock on
> ReplicationOriginRelationId at the end of API replorigin_drop_by_name.
> So there is no guarantee that invalidation reaches other backend doing
> the same operation. I think we need to keep the lock till the end of
> xact as we do in other drop operations (see DropTableSpace, dropdb).
>

Fixed the problem as mentioned above in the attached.

-- 
With Regards,
Amit Kapila.


v5-0001-Make-pg_replication_origin_drop-safe-against-con.patch
Description: Binary data

RE: Single transaction in the tablesync worker?

[osumi.takami...@fujitsu.com]

On Monday, February 8, 2021 1:44 PM osumi.takami...@fujitsu.com 

> On Mon, Feb 8, 2021 12:40 PM Amit Kapila 
> wrote:
> > On Mon, Feb 8, 2021 at 8:06 AM Peter Smith 
> > wrote:
> > >
> > > On Sat, Feb 6, 2021 at 6:30 PM osumi.takami...@fujitsu.com
> > >  wrote:
> > > >
> > > > > I have another idea for a test case: What if we write a test
> > > > > such that it fails PK violation on copy and then drop the 
> > > > > subscription.
> > > > > Then check there shouldn't be any dangling slot on the publisher?
> > > > > This is similar to a test in subscription/t/004_sync.pl, we can
> > > > > use some of that framework but have a separate test for this.
> > > > I've added this PK violation test to the attached tests.
> > > > The patch works with v28 and made no failure during regression tests.
> > > >
> > >
> > > I checked this patch. It applied cleanly on top of V28, and all
> > > tests passed
> > OK.
> > >
> > > Here are two feedback comments.
> > >
> > > 1. For the regression test there is 2 x SQL and 1 x function test. I
> > > thought to cover all the combinations there should be another
> > > function test. e.g.
> > > Tests ALTER … REFRESH
> > > Tests ALTER …. (refresh = true)
> > > Tests ALTER … (refresh = true) in a function Tests ALTER … REFRESH
> > > in a function  <== this combination is not being testing ??
> > >
> >
> > I am not sure whether there is much value in adding more to this set
> > of negative test cases unless it really covers a different code path
> > which I think won't happen if we add more tests here.
> Yeah, I agree. Accordingly, I didn't fix that part.
> 
> 
> On Mon, Feb 8, 2021 11:36 AM Peter Smith 
> wrote:
> > 2. For the 004 test case I know the test is needing some PK constraint
> > violation # Check if DROP SUBSCRIPTION cleans up slots on the
> > publisher side # when the subscriber is stuck on data copy for
> > constraint
> >
> > But it is not clear to me what was the exact cause of that PK
> > violation. I think you must be relying on data that is leftover from
> > some previous test case but I am not sure which one. Can you make the
> > comment more detailed to say
> > *how* the PK violation is happening - e.g something to say which rows,
> > in which table, and inserted by who?
> I added some comments to clarify how the PK violation happens.
> Please have a look.
Sorry, I had a one typo in the tests of subscription.sql in v2.
I used 'foo' for the first test of "ALTER SUBSCRIPTION mytest SET PUBLICATION 
foo WITH (refresh = true) in v02",
but I should have used 'mypub' to make this test clearly independent from other 
previous tests.
Attached the fixed version.

Best Regards,
Takamichi Osumi



refresh_and_pk_violation_testsets_v03.patch
Description: refresh_and_pk_violation_testsets_v03.patch

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

[Hou, Zhijie]

> Posting an updated set of patches.

A minor comment about doc.

+  
+Where the above target table features are determined to be, at worst,
+parallel-restricted, rather than parallel-unsafe, at least a parallel table
+scan may be used in the query plan for the INSERT
+statement. For more information about Parallel Safety, see
+.
+  

It seems does not mention that if target table is a foreign/temp table, a 
parallel table scan may be used.

So how about:

+  
+Where the target table is a foreign/temporary table or the above target 
table features
+are determined to be, at worst, parallel-restricted, rather than 
parallel-unsafe,
+at least a parallel table scan may be used in the query plan for the
+INSERT statement. For more information about Parallel 
Safety,
+see .
+  

Best regards,
houzj

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

[Greg Nancarrow]

On Mon, Feb 1, 2021 at 7:20 PM Tang, Haiying  wrote:
>
> Hi Greg,
>
> Recently, I was keeping evaluating performance of this patch(1/28 V13).
> Here I find a regression test case which is parallel insert with bitmap heap 
> scan.
> when the target table has primary key or index, then the patched performance 
> will have a 7%-19% declines than unpatched.
>
> Could you please have a look about this?
>
> I tried max_parallel_workers_per_gather=2/4/8, and I didn't tune other 
> parameters(like GUCs or other enforce parallel parameters).
>
> 1. max_parallel_workers_per_gather=2(default)
> target_tablepatched   master  %reg
> --
> without_PK_index83.683142.183-41%
> with_PK 382.824   321.10119%
> with_index  372.682   324.24615%
>
> 2. max_parallel_workers_per_gather=4
> target_tablepatched   master  %reg
> --
> without_PK_index73.189141.879 -48%
> with_PK 362.104   329.759 10%
> with_index  372.237   333.718 12%
>
> 3. max_parallel_workers_per_gather=8 (also set max_parallel_workers=16, 
> max_worker_processes = 16)
> target_tablepatched   master  %reg
> --
> without_PK_index75.072146.100 -49%
> with_PK 365.312   324.339 13%
> with_index  362.636   338.366 7%
>
> Attached test_bitmap.sql which includes my test data and sql if you want to 
> have a look.
>

Hi,

Did it actually use a parallel plan in your testing?
When I ran these tests with the Parallel INSERT patch applied, it did
not naturally choose a parallel plan for any of these cases.
So we can hardly blame the parallel insert with bitmap heap scan for
having worse performance, when based on costings, it doesn't actually
choose to use a parallel plan in this case.

Regards,
Greg Nancarrow
Fujitsu Australia

Re: Add MAIN_RELATION_CLEANUP and SECONDARY_RELATION_CLEANUP options to VACUUM

[Michael Paquier]

On Fri, Jan 29, 2021 at 06:43:44PM +, Bossart, Nathan wrote:
> I changed it to PROCESS_TOAST.

Thanks.  PROCESS_TOAST sounds good to me at the end for the option
name, so let's just go with that.

> Done.

While on it, I could not resist with changing VACOPT_SKIPTOAST to
VACOPT_PROCESS_TOAST on consistency grounds.  This is used only in
four places in the code, so that's not invasive.

What do you think?
--
Michael
From 562704e1b759d63666d03073839819669104bcca Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Mon, 8 Feb 2021 16:30:15 +0900
Subject: [PATCH v8] Add PROCESS_TOAST option to VACUUM.

---
 src/include/commands/vacuum.h|  2 +-
 src/backend/commands/vacuum.c| 19 ---
 src/backend/postmaster/autovacuum.c  |  5 +++--
 src/bin/psql/tab-complete.c  |  5 +++--
 src/bin/scripts/t/100_vacuumdb.pl|  9 -
 src/bin/scripts/vacuumdb.c   | 28 
 src/test/regress/expected/vacuum.out |  6 ++
 src/test/regress/sql/vacuum.sql  |  6 ++
 doc/src/sgml/ref/vacuum.sgml | 15 +++
 doc/src/sgml/ref/vacuumdb.sgml   | 15 +++
 10 files changed, 101 insertions(+), 9 deletions(-)

diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h
index 191cbbd004..d029da5ac0 100644
--- a/src/include/commands/vacuum.h
+++ b/src/include/commands/vacuum.h
@@ -181,7 +181,7 @@ typedef struct VacAttrStats
 #define VACOPT_FREEZE 0x08		/* FREEZE option */
 #define VACOPT_FULL 0x10		/* FULL (non-concurrent) vacuum */
 #define VACOPT_SKIP_LOCKED 0x20 /* skip if cannot get lock */
-#define VACOPT_SKIPTOAST 0x40	/* don't process the TOAST table, if any */
+#define VACOPT_PROCESS_TOAST 0x40	/* process the TOAST table, if any */
 #define VACOPT_DISABLE_PAGE_SKIPPING 0x80	/* don't skip any pages */
 
 /*
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 462f9a0f82..5228ed0bdc 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -104,6 +104,7 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel)
 	bool		freeze = false;
 	bool		full = false;
 	bool		disable_page_skipping = false;
+	bool		process_toast = true;
 	ListCell   *lc;
 
 	/* Set default value */
@@ -140,6 +141,8 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel)
 			disable_page_skipping = defGetBoolean(opt);
 		else if (strcmp(opt->defname, "index_cleanup") == 0)
 			params.index_cleanup = get_vacopt_ternary_value(opt);
+		else if (strcmp(opt->defname, "process_toast") == 0)
+			process_toast = defGetBoolean(opt);
 		else if (strcmp(opt->defname, "truncate") == 0)
 			params.truncate = get_vacopt_ternary_value(opt);
 		else if (strcmp(opt->defname, "parallel") == 0)
@@ -189,13 +192,13 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel)
 		(analyze ? VACOPT_ANALYZE : 0) |
 		(freeze ? VACOPT_FREEZE : 0) |
 		(full ? VACOPT_FULL : 0) |
-		(disable_page_skipping ? VACOPT_DISABLE_PAGE_SKIPPING : 0);
+		(disable_page_skipping ? VACOPT_DISABLE_PAGE_SKIPPING : 0) |
+		(process_toast ? VACOPT_PROCESS_TOAST : 0);
 
 	/* sanity checks on options */
 	Assert(params.options & (VACOPT_VACUUM | VACOPT_ANALYZE));
 	Assert((params.options & VACOPT_VACUUM) ||
 		   !(params.options & (VACOPT_FULL | VACOPT_FREEZE)));
-	Assert(!(params.options & VACOPT_SKIPTOAST));
 
 	if ((params.options & VACOPT_FULL) && params.nworkers > 0)
 		ereport(ERROR,
@@ -318,6 +321,15 @@ vacuum(List *relations, VacuumParams *params,
 (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
  errmsg("VACUUM option DISABLE_PAGE_SKIPPING cannot be used with FULL")));
 
+	/*
+	 * Sanity check PROCESS_TOAST option.
+	 */
+	if ((params->options & VACOPT_FULL) != 0 &&
+		(params->options & VACOPT_PROCESS_TOAST) == 0)
+		ereport(ERROR,
+(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("PROCESS_TOAST required with VACUUM FULL")));
+
 	/*
 	 * Send info about dead objects to the statistics collector, unless we are
 	 * in autovacuum --- autovacuum.c does this for itself.
@@ -1895,7 +1907,8 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
 	 * us to process it.  In VACUUM FULL, though, the toast table is
 	 * automatically rebuilt by cluster_rel so we shouldn't recurse to it.
 	 */
-	if (!(params->options & VACOPT_SKIPTOAST) && !(params->options & VACOPT_FULL))
+	if ((params->options & VACOPT_PROCESS_TOAST) != 0 &&
+		(params->options & VACOPT_FULL) == 0)
 		toast_relid = onerel->rd_rel->reltoastrelid;
 	else
 		toast_relid = InvalidOid;
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 47e60ca561..5360604933 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -2918,8 +2918,9 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map,
 		tab = palloc(sizeof(autovac_table));
 		tab->at_relid = relid;
 		tab->at_sharedrel = classForm->relisshared;
-		tab->at_params.options

Re: Extend more usecase for planning time partition pruning and init partition pruning.

[Andy Fan]

On Mon, Jan 25, 2021 at 10:21 AM Andy Fan  wrote:

>
>
> On Sun, Jan 24, 2021 at 6:34 PM Andy Fan  wrote:
>
>> Hi:
>>
>>  I recently found a use case like this.  SELECT * FROM p, q WHERE
>> p.partkey =
>>  q.colx AND (q.colx = $1 OR q.colx = $2); Then we can't do either
>> planning time
>>  partition prune or init partition prune.  Even though we have run-time
>>  partition pruning work at last, it is too late in some cases since we
>> have
>>  to init all the plan nodes in advance.  In my case, there are 10+
>>  partitioned relation in one query and the execution time is short, so the
>>  init plan a lot of plan nodes cares a lot.
>>
>> The attached patches fix this issue. It just get the "p.partkey = q.colx"
>> case in root->eq_classes or rel->joinlist (outer join), and then check if
>> there
>> is some baserestrictinfo in another relation which can be used for
>> partition
>> pruning. To make the things easier, both partkey and colx must be Var
>> expression in implementation.
>>
>> - v1-0001-Make-some-static-functions-as-extern-and-extend-C.patch
>>
>> Just some existing refactoring and extending ChangeVarNodes to be able
>> to change var->attno.
>>
>> - v1-0002-Build-some-implied-pruning-quals-to-extend-the-us.patch
>>
>> Do the real job.
>>
>> Thought?
>>
>>
>>
>> --
>> Best Regards
>> Andy Fan (https://www.aliyun.com/)
>>
>
>
> Some results from this patch.
>
> create table p (a int, b int, c character varying(8)) partition by list(c);
> create table p1  partition of p for values in ('01');
> create table p2  partition of p for values in ('02');
> create table p3  partition of p for values in ('03');
> create table q (a int, c character varying(8), b int) partition by list(c);
> create table q1  partition of q for values in ('01');
> create table q2  partition of q for values in ('02');
> create table q3  partition of q for values in ('03');
>
> Before the patch:
> postgres=# explain (costs off) select * from p inner join q on p.c = q.c
> and q.c > '02';
>  QUERY PLAN
> 
>  Hash Join
>Hash Cond: ((p.c)::text = (q.c)::text)
>->  Append
>  ->  Seq Scan on p1 p_1
>  ->  Seq Scan on p2 p_2
>  ->  Seq Scan on p3 p_3
>->  Hash
>  ->  Seq Scan on q3 q
>Filter: ((c)::text > '02'::text)
> (9 rows)
>
> After the patch:
>
>  QUERY PLAN
> 
>  Hash Join
>Hash Cond: ((p.c)::text = (q.c)::text)
>->  Seq Scan on p3 p
>->  Hash
>  ->  Seq Scan on q3 q
>Filter: ((c)::text > '02'::text)
> (6 rows)
>
>
> Before the patch:
> postgres=# explain (costs off) select * from p inner join q on p.c = q.c
> and (q.c = '02' or q.c = '01');
>  QUERY PLAN
>
> 
>  Hash Join
>Hash Cond: ((p.c)::text = (q.c)::text)
>->  Append
>  ->  Seq Scan on p1 p_1
>  ->  Seq Scan on p2 p_2
>  ->  Seq Scan on p3 p_3
>->  Hash
>  ->  Append
>->  Seq Scan on q1 q_1
>  Filter: (((c)::text = '02'::text) OR ((c)::text =
> '01'::text))
>->  Seq Scan on q2 q_2
>  Filter: (((c)::text = '02'::text) OR ((c)::text =
> '01'::text))
> (12 rows)
>
> After the patch:
>  QUERY PLAN
>
> 
>  Hash Join
>Hash Cond: ((p.c)::text = (q.c)::text)
>->  Append
>  ->  Seq Scan on p1 p_1
>  ->  Seq Scan on p2 p_2
>->  Hash
>  ->  Append
>->  Seq Scan on q1 q_1
>  Filter: (((c)::text = '02'::text) OR ((c)::text =
> '01'::text))
>->  Seq Scan on q2 q_2
>  Filter: (((c)::text = '02'::text) OR ((c)::text =
> '01'::text))
> (11 rows)
>
> Before the patch:
> postgres=# explain (costs off) select * from p left join q on p.c = q.c
> where (q.c = '02' or q.c = '01');
>  QUERY PLAN
>
> 
>  Hash Join
>Hash Cond: ((p.c)::text = (q.c)::text)
>->  Append
>  ->  Seq Scan on p1 p_1
>  ->  Seq Scan on p2 p_2
>  ->  Seq Scan on p3 p_3
>->  Hash
>  ->  Append
>->  Seq Scan on q1 q_1
>  Filter: (((c)::text = '02'::text) OR ((c)::text =
> '01'::text))
>->  Seq Scan on q2 q_2
>  Filter: (((c)::text = '02'::text) OR ((c)::text =
> '01'::text))
> (12 rows)
>
> After the patch:
>  QUERY PLAN
>
> -