SSL compression

[Michael J. Baars]

Hi All,

While I was searching for methods to send the result of a query to the other 
side of the world, because it might be nice to go there once in a while, I 
noticed
my best option, SSL compression, has been disabled as of version 14. Could 
someone please explain to me, why compression is being considered unsafe / 
insecure?

Transmissions to areas outside of Europe cost €5/mb at the moment and that 
makes SSL compression, or compression in general a vital component of d
ata transmissions.

Might the underlying reason be, that certain people have shown interest in my 
libpq/PQblockwrite algorithms (
https://www.postgresql.org/message-id/c7cccd0777f39c53b9514e3824badf276759fa87.camel%40cyberfiber.eu)
 but felt turned down and are now persuading me to trade
the algorithms against SSL compression, than just say so please. I'll see what 
I can do.

Best regards,
Mischa Baars.

Re: [PATCH] pg_stat_statements Configuration Parameters Documentation

[Julien Rouhaud]

On Mon, Nov 8, 2021 at 3:19 PM Fujii Masao  wrote:
>
> On 2021/11/08 15:12, Ken Kato wrote:
> > Hi,
> >
> > Configuration parameters for pg_stat_statements were not in the index,
> > so I added them just like auto_explain configuration parameters.
>
> Thanks for the patch! LGTM.

+1, patch looks good to me too.

Re: [PATCH] pg_stat_statements Configuration Parameters Documentation

[Fujii Masao]

On 2021/11/08 15:12, Ken Kato wrote:

Hi,

Configuration parameters for pg_stat_statements were not in the index,
so I added them just like auto_explain configuration parameters.


Thanks for the patch! LGTM.

Regards,

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

Re: Skipping logical replication transactions on subscriber side

[Greg Nancarrow]

On Mon, Nov 8, 2021 at 1:20 AM Masahiko Sawada  wrote:
>
> I've attached an updated patch. In this version patch, subscription
> worker statistics are collected per-database and handled in a similar
> way to tables and functions. I think perhaps we still need to discuss
> details of how the statistics should be handled but I'd like to share
> the patch for discussion.
>

That's for the updated patch.
Some initial comments on the v20 patch:


doc/src/sgml/monitoring.sgml

(1) wording
The word "information" seems to be missing after "showing" (otherwise
is reads "showing about errors", which isn't correct grammar).
I suggest the following change:

BEFORE:
+  At least one row per subscription, showing about errors that
+  occurred on subscription.
AFTER:
+  At least one row per subscription, showing information about
+  errors that occurred on subscription.

(2) pg_stat_reset_subscription_worker(subid Oid, relid Oid) function
documentation
The description doesn't read well. I'd suggest the following change:

BEFORE:
* Resets statistics of a single subscription worker statistics.
AFTER:
* Resets the statistics of a single subscription worker.

I think that the documentation for this function should make it clear
that a non-NULL "subid" parameter is required for both reset cases
(tablesync and apply).
Perhaps this could be done by simply changing the first sentence to say:
"Resets the statistics of a single subscription worker, for a worker
running on the subscription with subid."
(and then can remove " running on the subscription with
subid" from the last sentence)

I think that the documentation for this function should say that it
should be used in conjunction with the "pg_stat_subscription_workers"
view in order to obtain the required subid/relid values for resetting.
(and should provide a link to the documentation for that view)

Also, I think that the function documentation should make it clear how
to distinguish the tablesync vs apply worker statistics case.
e.g. the tablesync error case is indicated by a null "command" in the
information returned from the "pg_stat_subscription_workers" view
(otherwise it seems a user could only know this by looking at the server log).

Finally, there are currently no tests for this new function.

(3) pg_stat_subscription_workers
In the documentation for this, some users may not realise that "the
initial data copy" refers to "tablesync", so maybe say "the initial
data copy (tablesync)", or similar.

(4) stats_reset
"stats_reset" is currently documented as the last column of the
"pg_stat_subscription_workers" view - but it's actually no longer
included in the view.

(5) src/tools/pgindent/typedefs.list
The following current entries are bogus:
PgStat_MsgSubWorkerErrorPurge
PgStat_MsgSubWorkerPurge

The following entry is missing:
PgStat_MsgSubscriptionPurge


Regards,
Greg Nancarrow
Fujitsu Australia

RE: row filtering for logical replication

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

On Fri, Nov 5, 2021 1:14 PM Peter Smith  wrote:
> PSA new set of v37* patches.

Thanks for updating the patches.
Few comments:

1) v37-0001

I think it might be better to also show the filter expression in '\d+
tablename' command after publication description.

2) v37-0004

+   /* Scan the expression tree for referenceable objects */
+   find_expr_references_walker(expr, );
+
+   /* Remove any duplicates */
+   eliminate_duplicate_dependencies(context.addrs);
+

The 0004 patch currently use find_expr_references_walker to get all the
reference objects. I am thinking do we only need get the columns in the
expression ? I think maybe we can check the replica indentity like[1].

3) v37-0005

- no parse nodes of any kind other than Var, OpExpr, Const, BoolExpr, FuncExpr

I think there could be other node type which can also be considered as simple
expression, for exmaple T_NullIfExpr.

Personally, I think it's natural to only check the IMMUTABLE and
whether-user-defined in the new function rowfilter_walker. We can keep the
other row-filter errors which were thrown for EXPR_KIND_PUBLICATION_WHERE in
the 0001 patch.

[1]
rowfilter_expr_checker
...
if (replica_identity == REPLICA_IDENTITY_DEFAULT)
context.bms_replident = 
RelationGetIndexAttrBitmap(rel, INDEX_ATTR_BITMAP_PRIMARY_KEY);
else
context.bms_replident = 
RelationGetIndexAttrBitmap(rel, INDEX_ATTR_BITMAP_IDENTITY_KEY);

(void) rowfilter_expr_replident_walker(rfnode, 
);

...
static bool
rowfilter_expr_replident_walker(Node *node, rf_context *context)
{
if (node == NULL)
return false;

if (IsA(node, Var))
{
Oid relid = RelationGetRelid(context->rel);
Var*var = (Var *) node;
AttrNumber  attnum = var->varattno - 
FirstLowInvalidHeapAttributeNumber;

if (!bms_is_member(attnum, context->bms_replident))
{
const char *colname = get_attname(relid, attnum, false);
ereport(ERROR,

(errcode(ERRCODE_INVALID_COLUMN_REFERENCE),
errmsg("cannot add relation \"%s\" to 
publication",

RelationGetRelationName(context->rel)),
errdetail("Row filter column \"%s\" is 
not part of the REPLICA IDENTITY",
colname)));

return false;
}

return true;
}

return expression_tree_walker(node, rowfilter_expr_replident_walker,
  (void *) 
context);
}

Best regards,
Hou zj

[PATCH] pg_stat_statements Configuration Parameters Documentation

[Ken Kato]

Hi,

Configuration parameters for pg_stat_statements were not in the index,
so I added them just like auto_explain configuration parameters.

Best wishes,

--
Ken Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATIONdiff --git a/doc/src/sgml/pgstatstatements.sgml b/doc/src/sgml/pgstatstatements.sgml
index 70b28a92c1..bc9d5bdbe3 100644
--- a/doc/src/sgml/pgstatstatements.sgml
+++ b/doc/src/sgml/pgstatstatements.sgml
@@ -634,6 +634,9 @@

 
  pg_stat_statements.max (integer)
+ 
+  pg_stat_statements.max configuration parameter
+ 
 
 
 
@@ -654,6 +657,9 @@

 
  pg_stat_statements.track (enum)
+ 
+  pg_stat_statements.track configuration parameter
+ 
 
 
 
@@ -673,6 +679,9 @@

 
  pg_stat_statements.track_utility (boolean)
+ 
+  pg_stat_statements.track_utility configuration parameter
+ 
 
 
 
@@ -690,6 +699,9 @@

 
  pg_stat_statements.track_planning (boolean)
+ 
+  pg_stat_statements.track_planning configuration parameter
+ 
 
 
 
@@ -709,6 +721,9 @@

 
  pg_stat_statements.save (boolean)
+ 
+  pg_stat_statements.save configuration parameter
+ 
 
 
 

Re: Failed transaction statistics to measure the logical replication progress

[vignesh C]

On Fri, Nov 5, 2021 at 1:42 PM osumi.takami...@fujitsu.com
 wrote:
>
> On Thursday, November 4, 2021 9:54 AM  Greg Nancarrow  
> wrote:
> > On Tue, Nov 2, 2021 at 12:18 AM osumi.takami...@fujitsu.com
> >  wrote:
> > >
> > > On Thursday, October 28, 2021 11:19 PM I wrote:
> > > > I've created a new patch that extends pg_stat_subscription_workers
> > > > to include other transaction statistics.
> > > >
> > > > Note that this patch depends on v18 patch-set in [1]...
> > > Rebased based on the v19 in [1].
> > > Also, fixed documentation a little and made tests tidy.
> > > FYI, the newly included TAP test(027_worker_xact_stats.pl) is stable
> > > because I checked that 100 times of its execution in a tight loop all 
> > > passed.
> > >
> >
> > I have done some basic testing of the patch and have some initial review
> > comments:
> Thanks for your review !
>
> > (1) I think this patch needs to be in "git format-patch" format, with a 
> > proper
> > commit message that describes the purpose of the patch and the functionality
> > it adds, and any high-level design points (something like the overview 
> > given in
> > the initial post, and accounting for the subsequent discussion points and
> > updated functionality).
> Fixed.
>
> > (2) doc/src/sgml/monitoring.sgml
> > There are some grammatical issues in the current description. I suggest
> > changing it to something like:
> > BEFORE:
> > +  At least one row per subscription, showing about
> > transaction statistics and error summary that
> > AFTER:
> > +  At least one row per subscription, showing transaction
> > statistics and information about errors that
> Fixed.
>
> > (2) doc/src/sgml/monitoring.sgml
> > The current description seems a little confusing.
> > Per subscription, it shows the transaction statistics and any last error 
> > info from
> > tablesync/apply workers? If this is the case, I'd suggest the following 
> > change:
> >
> > BEFORE:
> > +   one row per subscription for transaction statistics and summary of the
> > last
> > +   error reported by workers applying logical replication changes and
> > workers
> > +   handling the initial data copy of the subscribed tables.
> > AFTER:
> > +   one row per subscription, showing corresponding transaction statistics
> > and
> > +   information about the last error reported by workers applying
> > logical replication
> > +   changes or by workers handling the initial data copy of the
> > subscribed tables.
> Fixed.
>
> > (3) xact_commit
> > I think that the "xact_commit" column should be named "xact_commit_count"
> > or "xact_commits".
> > Similarly, I think "xact_error" should be named "xact_error_count" or
> > "xact_errors", and "xact_aborts" should be named "xact_abort_count" or
> > "xact_aborts".
> I prefered *_count. Renamed.
>
> > (4) xact_commit_bytes
> >
> > +   Amount of transactions data successfully applied in this 
> > subscription.
> > +   Consumed memory for xact_commit is displayed.
> >
> > I find this description a bit confusing. "Consumed memory for xact_commit"
> > seems different to "transactions data".
> > Could the description be something like: Amount of data (in bytes) 
> > successfully
> > applied in this subscription, across "xact_commit_count"
> > transactions.
> Fixed.
>
> > (5)
> > I'd suggest some minor rewording for the following:
> >
> > BEFORE:
> > +   Number of transactions failed to be applied and caught by table
> > +   sync worker or main apply worker in this subscription.
> > AFTER:
> > +   Number of transactions that failed to be applied by the table
> > +   sync worker or main apply worker in this subscription.
> Fixed.
>
> > (6) xact_error_bytes
> > Again, it's a little confusing referring to "consumed memory" here.
> > How about rewording this, something like:
> >
> > BEFORE:
> > +   Amount of transactions data unsuccessfully applied in this
> > subscription.
> > +   Consumed memory that past failed transaction used is displayed.
> > AFTER:
> > +   Amount of data (in bytes) unsuccessfully applied in this
> > subscription by the last failed transaction.
> xact_error_bytes (and other bytes columns as well) is cumulative
> so when a new error happens, the size of this new bytes would be
> added to the same. So here we shouldn't mention just the last error.
> I simply applied your previous comments of 'xact_commit_bytes'
> to 'xact_error_bytes' description.
>
> > (7)
> > The additional information provided for "xact_abort_bytes" needs some
> > rewording, something like:
> >
> > BEFORE:
> > +   Increase logical_decoding_work_mem on the
> > publisher
> > +   so that it exceeds the size of whole streamed transaction
> > +   to suppress unnecessary consumed network bandwidth in addition to
> > change
> > +   in memory of the subscriber, if unexpected amount of streamed
> > transactions
> > +   are aborted.
> > AFTER:
> > +   In order to suppress unnecessary consumed network bandwidth,
> > increase
> > +

Re: Commitfest 2021-11 Patch Triage - Part 1

[Kyotaro Horiguchi]

At Mon, 8 Nov 2021 14:43:43 +0900, Michael Paquier  wrote 
in 
> On Fri, Nov 05, 2021 at 10:15:49PM +0100, Daniel Gustafsson wrote:
> > 2161: standby recovery fails when re-replaying due to missing directory 
> > which
> > was removed in previous replay.
> > =
> > Tom and Robert seem to be in agreement that parts of this patchset are good,
> > and that some parts are not.  The thread has stalled and the patch no longer
> > apply, so unless someone feels like picking up the good pieces this seems 
> > like
> > a contender to close for now.
> 
> That's in my area, so I have signed up as reviewer.

I noticed I'm one of the author^^; I'll also look into the comments
and try to address them.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: Question about psql meta-command with schema option doesn't use visibilityrule

[Tatsuro Yamada]

Hi David,


I have a question that is a specification of permission check
(visibilityrule) for psql meta-command with schema option.

Visibility means search_path, not permission.  If s_a is not in the 
search_paths it objects are not visible unqualified but can be seen (catalog) 
when schema qualified.


Thanks for your comments! I understood them:
 - all users can show System catalog (pg_catalog. *) is a
   specification, so it is not a bug
 - visibility and permission are not the same (I confused it before, oops)

Regards,
Tatsuro Yamada

Re: Make mesage at end-of-recovery less scary.

[Kyotaro Horiguchi]

At Fri, 22 Oct 2021 17:54:40 +, "Bossart, Nathan"  
wrote in 
> On 3/4/21, 10:50 PM, "Kyotaro Horiguchi"  wrote:
> > As the result, the following messages are emitted with the attached.
> 
> I'd like to voice my support for this effort, and I intend to help
> review the patch.  It looks like the latest patch no longer applies,
> so I've marked the commitfest entry [0] as waiting-on-author.
> 
> Nathan
> 
> [0] https://commitfest.postgresql.org/35/2490/

Sorry for being late to reply.  I rebased this to the current master.

- rebased

- use LSN_FORMAT_ARGS instead of bare shift and mask.

- v4 immediately exited walreceiver on disconnection. Maybe I wanted
  not to see a FATAL message on standby after primary dies. However
  that would be another issue and that change was plain wrong..  v5
  just removes the "end-of-WAL" part from the message, which duplicate
  to what startup emits.

- add a new error message "missing contrecord at %X/%X".  Maybe this
  should be regarded as a leftover of the contrecord patch. In the
  attached patch the "%X/%X" is the LSN of the current record. The log
  messages look like this (026_overwrite_contrecord).

LOG:  redo starts at 0/1486CB8
WARNING:  missing contrecord at 0/1FFC2E0
LOG:  consistent recovery state reached at 0/1FFC2E0
LOG:  started streaming WAL from primary at 0/200 on timeline 1
LOG:  successfully skipped missing contrecord at 0/1FFC2E0, overwritten at 
2021-11-08 14:50:11.969952+09
CONTEXT:  WAL redo at 0/228 for XLOG/OVERWRITE_CONTRECORD: lsn 0/1FFC2E0; 
time 2021-11-08 14:50:11.969952+09

While checking the behavior for the case of missing-contrecord, I
noticed that emode_for_corrupt_record() doesn't work as expected since
readSource is reset to XLOG_FROM_ANY after a read failure. We could
remember the last failed source but pg_wal should have been visited if
page read error happened so I changed the function so that it treats
XLOG_FROM_ANY the same way with XLOG_FROM_PG_WAL.

(Otherwise we see "LOG: reached end-of-WAL at .." message after
 "WARNING: missing contrecord at.." message.)

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
>From 276f59c8b37a31cb831b7753d2b107eb1d83c1fb Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi 
Date: Fri, 28 Feb 2020 15:52:58 +0900
Subject: [PATCH v5] Make End-Of-Recovery error less scary

When recovery in any type ends, we see a bit scary error message like
"invalid record length" that suggests something serious is
happening. Actually if recovery meets a record with length = 0, that
usually means it finished applying all available WAL records.

Make this message less scary as "reached end of WAL". Instead raise
the error level for other kind of WAL failure to WARNING.
---
 src/backend/access/transam/xlog.c   | 94 +++--
 src/backend/access/transam/xlogreader.c | 14 
 src/backend/replication/walreceiver.c   |  3 +-
 src/include/access/xlogreader.h |  1 +
 4 files changed, 87 insertions(+), 25 deletions(-)

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 5cda30836f..623fb01d0a 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -4477,6 +4477,7 @@ ReadRecord(XLogReaderState *xlogreader, int emode,
 	for (;;)
 	{
 		char	   *errormsg;
+		XLogRecPtr	ErrRecPtr = InvalidXLogRecPtr;
 
 		record = XLogReadRecord(xlogreader, );
 		ReadRecPtr = xlogreader->ReadRecPtr;
@@ -4494,6 +4495,16 @@ ReadRecord(XLogReaderState *xlogreader, int emode,
 			{
 abortedRecPtr = xlogreader->abortedRecPtr;
 missingContrecPtr = xlogreader->missingContrecPtr;
+ErrRecPtr = abortedRecPtr;
+			}
+			else
+			{
+/*
+ * NULL ReadRecPtr means we could not read a record at
+ * beginning. In that case EndRecPtr is storing the LSN of the
+ * record we tried to read.
+ */
+ErrRecPtr = ReadRecPtr ? ReadRecPtr : EndRecPtr;
 			}
 
 			if (readFile >= 0)
@@ -4503,13 +4514,12 @@ ReadRecord(XLogReaderState *xlogreader, int emode,
 			}
 
 			/*
-			 * We only end up here without a message when XLogPageRead()
-			 * failed - in that case we already logged something. In
-			 * StandbyMode that only happens if we have been triggered, so we
-			 * shouldn't loop anymore in that case.
+			 * If we get here for other than end-of-wal, emit the error message
+			 * right now. Otherwise the message if any is shown as a part of
+			 * the end-of-WAL message below.
 			 */
-			if (errormsg)
-ereport(emode_for_corrupt_record(emode, EndRecPtr),
+			if (!xlogreader->EndOfWAL && errormsg)
+ereport(emode_for_corrupt_record(emode, ErrRecPtr),
 		(errmsg_internal("%s", errormsg) /* already translated */ ));
 		}
 
@@ -4540,11 +4550,12 @@ ReadRecord(XLogReaderState *xlogreader, int emode,
 			/* Great, got a record */
 			return record;
 		}
-		else
+
+		/* No valid record available from this source */
+		lastSourceFailed = true;
+
+		if (!fetching_ckpt)
 		{
-			/* No valid record available from 

Re: Commitfest 2021-11 Patch Triage - Part 1

[Michael Paquier]

On Fri, Nov 05, 2021 at 10:15:49PM +0100, Daniel Gustafsson wrote:
> 2161: standby recovery fails when re-replaying due to missing directory which
> was removed in previous replay.
> =
> Tom and Robert seem to be in agreement that parts of this patchset are good,
> and that some parts are not.  The thread has stalled and the patch no longer
> apply, so unless someone feels like picking up the good pieces this seems like
> a contender to close for now.

That's in my area, so I have signed up as reviewer.

> 2518: Corruption during WAL replay
> ==
> Both Robert and Tom have given positive remarks about this, and recent review
> concerns raised turned out to have been handled.  Robert, Tom: any thoughts on
> the latest posted version?

The latest comments are from a couple of weeks ago.  I'll take a look.

> 2602: ALTER SYSTEM READ { ONLY | WRITE }
> 
> AFAIK Robert is actively working on this and have committed parts of it
> already, so there isn't much to add.  I'm personally quite excited about this
> feature so I'm looking forward to (hopefully) seeing it go in.

So am I.
--
Michael


signature.asc
Description: PGP signature

Re: XLogReadRecord() error in XlogReadTwoPhaseData()

[Michael Paquier]

On Sat, Nov 06, 2021 at 06:31:57PM -0700, Noah Misch wrote:
> As a first step, let's report the actual XLogReadRecord() error message.
> Attached.

Good catch!  This looks good.

> All the other sites that expect no error already do this.

Indeed.  Looking closer, I think that we'd better improve
DecodingContextFindStartpoint(),
pg_logical_replication_slot_advance(), XLogSendLogical() as well as
pg_logical_slot_get_changes_guts() to follow a format closer to what
you have in your patch, with an error message that describes the
context where the problem has been found, instead of just elog()'ing
what XLogReadRecord() returns.
--
Michael


signature.asc
Description: PGP signature

Re: [PROPOSAL] new diagnostic items for the dynamic sql

[Pavel Stehule]

po 8. 11. 2021 v 5:24 odesílatel Pavel Stehule 
napsal:

>
>
> po 8. 11. 2021 v 5:07 odesílatel Pavel Stehule 
> napsal:
>
>>
>>>
>>> +set_errcurrent_query (const char *query)
>>>
>>> You can remove the space prior to (.
>>> I wonder if the new field can be named current_err_query because that's
>>> what the setter implies.
>>> current_query may give the impression that the field can store normal
>>> query (which doesn't cause exception).
>>> The following code implies that only one of internalquery and
>>> current_query would be set.
>>>
>>
>> yes, I think so current_query is not a good name too. Maybe query can be
>> good enough - all in ErrorData is related to error
>>
>
> so the name of field can be query, and routine for setting errquery or
> set_errquery
>

and this part is not correct

<--><-->switch (carg->mode)
<--><-->{
<--><--><-->case RAW_PARSE_PLPGSQL_EXPR:
<--><--><--><-->errcontext("SQL expression \"%s\"", query);
<--><--><--><-->break;
<--><--><-->case RAW_PARSE_PLPGSQL_ASSIGN1:
<--><--><-->case RAW_PARSE_PLPGSQL_ASSIGN2:
<--><--><-->case RAW_PARSE_PLPGSQL_ASSIGN3:
<--><--><--><-->errcontext("PL/pgSQL assignment \"%s\"", query);
<--><--><--><-->break;
<--><--><-->default:
<--><--><--><-->set_errcurrent_query(query);
<--><--><--><-->errcontext("SQL statement \"%s\"", query);
<--><--><--><-->break;
<--><-->}
<-->}

set_errcurrent_query should be outside the switch

We want PG_SQL_TEXT for assign statements too

_t := (select ...);

Regards

Pavel

Re: [PROPOSAL] new diagnostic items for the dynamic sql

[Pavel Stehule]

po 8. 11. 2021 v 5:07 odesílatel Pavel Stehule 
napsal:

>
>>
>> +set_errcurrent_query (const char *query)
>>
>> You can remove the space prior to (.
>> I wonder if the new field can be named current_err_query because that's
>> what the setter implies.
>> current_query may give the impression that the field can store normal
>> query (which doesn't cause exception).
>> The following code implies that only one of internalquery and
>> current_query would be set.
>>
>
> yes, I think so current_query is not a good name too. Maybe query can be
> good enough - all in ErrorData is related to error
>

so the name of field can be query, and routine for setting errquery or
set_errquery


>
>
>> +   if (estate->cur_error->internalquery)
>> +   exec_assign_c_string(estate, var,
>> estate->cur_error->internalquery);
>> +   else
>> +   exec_assign_c_string(estate, var,
>> estate->cur_error->current_query);
>>
>> Cheers
>>
>

Re: removing global variable ThisTimeLineID

[Michael Paquier]

On Tue, Nov 02, 2021 at 08:45:57AM -0400, Robert Haas wrote:
> Also, XLogCtl->ThisTimeLineID isn't set until the end of recovery, so
> calling this function during recovery would be a mistake. There seem
> to be a number of interface functions in xlog.c that should only be
> called when not in recovery, and neither their names nor their
> comments make that clear. I wasn't bold enough to go label all the
> ones I think fall into that category, but maybe we should. 

I got to wonder whether it would be better to add in GetFlushRecPtr()
the same assertion as GetWALInsertionTimeLine().  All the in-core
callers of this routine already assume that, and it would be buggy if
one passes down insertTLI to get the current TLI.

> I thought about that, but I think it's pointless. I think we can just
> say that if openLogFile == -1, openLogSegNo and openLogTLI are
> meaningless. I don't think we're currently resetting openLogSegNo to
> an invalid value either, so I don't think openLogTLI should be treated
> differently.

Wouldn't it be better to at least document that as a comment rather
than letting the reader guess it, then?  I agree that this is a minor
point, though.

> I'm not opposed to introducing InvalidTimeLineID on
> principle, and I looked for a way of doing it in this patch set, but
> it didn't seem all that valuable, and I feel that someone who cares
> about it can do it as a separate effort after I commit these.

No issues from me.  I have bumped into a case just at the end of last
week where I wanted a better way to tell if a TLI is invalid rather
than leaving things to 0, FWIW.

>> The handling of XLogCtl->ThisTimeLineID still seems a bit blurry when
>> it comes to timeline switches because we don't update it while in
>> recovery when replaying a end-of-recovery record.  This could at least
>> be documented better.
> 
> We don't update it during recovery at all. There's exactly one
> assignment statement for that variable and it's here:

Indeed.  It looks like my reading was sloppy here.
--
Michael


signature.asc
Description: PGP signature

Re: postgres_fdw: commit remote (sub)transactions in parallel during pre-commit

[Fujii Masao]

On 2021/11/07 18:06, Etsuro Fujita wrote:

On Mon, Nov 1, 2021 at 3:22 PM Fujii Masao  wrote:

On 2021/10/31 18:05, Etsuro Fujita wrote:

The patch is pretty simple: if a server option added
by the patch “parallel_commit” is enabled,


Could you tell me why the parameter is necessary?
Can't we always enable the feature?


I don’t think parallel commit would cause performance degradation,
even in the case when there is just a single remote (sub)transaction
to commit when called from pgfdw_xact_callback
(pgfdw_subxact_callback), so I think it might be OK to enable it by
default.  But my concern about doing so is the remote side: during
those functions, if there are a lot of (sub)transactions on a single
remote server that need to be committed, parallel commit would
increase the remote server’s load at (sub)transaction end than serial
commit, which is the existing implementation, as the requests to
commit those (sub)transactions are sent to the remote server at the
same time; which some users might want to avoid.


Thanks for explaining this! But probably I failed to get your point.
Sorry... Whichever parallel or serial commit approach, the number of
transactions to commit on the remote server is the same, isn't it?
For example, please imagine the case where a client requests
ten transactions per second to the local server. Each transaction
accesses to the foreign table, so which means that ten transaction
commit operations per second are requested to the remote server.
Unless I'm missing something, this number doesn't change whether
the foreign transaction is committed in parallel way or not.
Thought?



I think we could extend this to abort cleanup of remote
(sub)transactions during post-abort.  Anyway, I think this is useful,
so I’ll add this to the upcoming commitfest.


Thanks!


I'll update the patch as such in the next version.


IMO it's better to implement and commit these features gradually
if possible. Which would simplify the patch and make it
easier-to-review. So I think that it's better to implement
this feature as a separate patch.



+   /* Consume whatever data is available from the socket */
+   if (!PQconsumeInput(conn))
+   pgfdw_report_error(ERROR, NULL, conn, false, sql);

Without the patch, PQconsumeInput() is not called before pgfdw_get_result()
But could you tell me why you added PQconsumeInput() there?


The reason is that there might be the result already before calling
pgfdw_get_result(), in which case PQconsumeInput() followed by
PQisBusy() would allow us to call PQgetResult() without doing
WaitLatchOrSocket(), which I think is rather expensive.


Understood. It's helpful to add the comment about why PQconsumeInput()
is called there.

Also could you tell me how much expensive it is?



When ignore_errors argument is true, the error reported by
PQconsumeInput() should be ignored?


I’m not sure about that, because the error might be caused by e.g.,
OOM in the local server, in which case I don’t think it is safe to
ignore it and continue the (sub)transaction-end processing.


But the existing code ignores the error at all, doesn't it?
If it's unsafe to do that, probably we should fix that at first?

Regards,

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

Re: [PROPOSAL] new diagnostic items for the dynamic sql

[Pavel Stehule]

>
>
>
> +set_errcurrent_query (const char *query)
>
> You can remove the space prior to (.
> I wonder if the new field can be named current_err_query because that's
> what the setter implies.
> current_query may give the impression that the field can store normal
> query (which doesn't cause exception).
> The following code implies that only one of internalquery and
> current_query would be set.
>

yes, I think so current_query is not a good name too. Maybe query can be
good enough - all in ErrorData is related to error



> +   if (estate->cur_error->internalquery)
> +   exec_assign_c_string(estate, var,
> estate->cur_error->internalquery);
> +   else
> +   exec_assign_c_string(estate, var,
> estate->cur_error->current_query);
>
> Cheers
>

Re: Schema variables - new implementation for Postgres 15

[Pavel Stehule]

ne 7. 11. 2021 v 22:36 odesílatel Tomas Vondra <
tomas.von...@enterprisedb.com> napsal:

> On 11/6/21 04:45, Pavel Stehule wrote:
> > Hi
> >
> > st 3. 11. 2021 v 14:05 odesílatel Tomas Vondra
> > mailto:tomas.von...@enterprisedb.com>>
> > napsal:
> >
> > Hi,
> >
> > I took a quick look at the latest patch version. In general the patch
> > looks pretty complete and clean, and for now I have only some basic
> > comments. The attached patch tweaks some of this, along with a couple
> > additional minor changes that I'll not discuss here.
> >
> >
> > 1) Not sure why we need to call this "schema variables". Most objects
> > are placed in a schema, and we don't say "schema tables" for example.
> > And it's CREATE VARIABLE and not CREATE SCHEMA VARIABLE, so it's a
> bit
> > inconsistent.
> >
> >
> > Yes, there is inconsistency, but I think it is necessary. The name
> > "variable" is too generic. Theoretically we can use other adjectives
> > like session variables or global variables and the name will be valid.
> > But it doesn't describe the fundamentals of design. This is similar to
> > the package's variables from PL/SQL. These variables are global,
> > session's variables too. But the usual name is "package variables". So
> > schema variables are assigned to schemes, and I think a good name can be
> > "schema variables". But it is not necessary to repeat keyword schema in
> > the CREATE COMMAND.
> >
> > My opinion is not too strong in this case, and I can accept just
> > "variables" or "session's variables" or "global variables", but I am not
> > sure if these names describe this feature well, because still they are
> > too generic. There are too many different implementations of session
> > global variables (see PL/SQL or T-SQL or DB2).
> >
>
> OK. "Session variable" seems better to me, but I'm not sure how well
> that matches other databases. I'm not sure how much should we feel
> constrained by naming in other databases, though.
>

session variables is generic term - there are big differences already -
T-SQL versus PL/SQL or SQL+ or DB2


> >
> > The docs actually use "Global variables" in one place for some
> reason.
> >
> >
> > 2) I find this a bit confusing:
> >
> > SELECT non_existent_variable;
> > test=# select s;
> > ERROR:  column "non_existent_variable" does not exist
> > LINE 1: select non_existent_variable;
> >
> > I wonder if this means using SELECT to read variables is a bad idea,
> and
> > we should have a separate command, just like we have LET (instead of
> > just using UPDATE in some way).
> >
> >
> > I am sure so I want to use variables in SELECTs. One interesting case is
> > using variables in RLS.
> >
>
> How much more complicated would it be without the SELECT?
>

It is not too complicated, just you want to introduce SELECT2. The sense of
session variables is to be used. Has no sense to hold a value on a server
without the possibility to use it.

Session variables can be used as global variables in PL/pgSQL. If you
cannot use it in SQL expressions, then you need to copy it to a local
variable, and then you can use it. That cannot work. This design is a
replacement of a untyped not nullable slow workaround based on GUC, there
is a necessity to use it in SQL.


> > I prefer to fix this error message to "column or variable ... does not
> > exist"
> >
>
> Not sure it's a good idea to make the error message more ambiguous. Most
> people won't use variables at all, and the message will be less clear
> for them.
>

Yes, there is new complexity. But it is an analogy with variables in
PL/pgSQL with all benefits and negatives. You don't want to use dynamic SQL
everywhere you use PL/pgSQL variables.

There are more cases than RLS in SQL

1. hold value in session (for interactive work or for non interactive
scripts). Sometimes you want to reuse value - we can now use CTE or
temporary tables. But in this case you have to store relation, you cannot
store value, that can be used as a query parameter.

2. allow safe and effective parametrization of SQL scripts, and copy value
from client side to server side (there is not risk of SQL injection).

run script with parameter -v xx=10

```
create temp variable xx as int;
set xx = :`xx`;
do $$
  .. -- I can work with variable xx on server side

  ...

$$

This is complement to client side variables - the advantage is possibility
to use outside psql, the are type, and the metadata can be permanent.

3. you can share value by PL environments (and by possible clients). But
this sharing is secure - the rules are the same like holding value in an
table.

Session variables increase complexity a little bit, but increases
possibilities and comfort for developers that use databases directly. The
analogy with PL/pgSQL variables is well, jut you are not limited to
PL/pgSQL scope.

Regards

Pavel



>
> regards
>
> --
> Tomas Vondra
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise 

Re: prevent immature WAL streaming

[Alexander Lakhin]

Hello Alvaro,
14.10.2021 01:09, Alvaro Herrera wrote:
>> Yea, let's go for your patch then. I've verified that at least locally it
>> passes under valgrind.
> Ah great, thanks.  Pushed then.
>
While translating messages I've noticed that the version of the patch
ported to REL9_6_STABLE..REL_13_STABLE contains a typo "sucessfully".
Please consider committing the fix.

Best regards,
Alexander
diff --git a/src/backend/access/transam/xlog.c 
b/src/backend/access/transam/xlog.c
index 6d37b82330d..3d76fad1282 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -10383,7 +10383,7 @@ VerifyOverwriteContrecord(xl_overwrite_contrecord 
*xlrec, XLogReaderState *state
 (uint32) state->overwrittenRecPtr);
 
ereport(LOG,
-   (errmsg("sucessfully skipped missing contrecord at 
%X/%X, overwritten at %s",
+   (errmsg("successfully skipped missing contrecord at 
%X/%X, overwritten at %s",
(uint32) (xlrec->overwritten_lsn >> 32),
(uint32) xlrec->overwritten_lsn,

timestamptz_to_str(xlrec->overwrite_time;
diff --git a/src/test/recovery/t/026_overwrite_contrecord.pl 
b/src/test/recovery/t/026_overwrite_contrecord.pl
index c35e3717276..4fac44e2617 100644
--- a/src/test/recovery/t/026_overwrite_contrecord.pl
+++ b/src/test/recovery/t/026_overwrite_contrecord.pl
@@ -94,7 +94,7 @@ ok($node_standby->safe_psql('postgres', 'select * from foo') 
eq 'hello',
 my $log = slurp_file($node_standby->logfile);
 like(
$log,
-   qr[sucessfully skipped missing contrecord at],
+   qr[successfully skipped missing contrecord at],
"found log line in standby");
 
 $node->stop;

Re: pg_upgrade test for binary compatibility of core data types

[Michael Paquier]

On Sun, Nov 07, 2021 at 01:22:00PM -0600, Justin Pryzby wrote:
> That may be good enough for test.sh, but if the kludges were moved to a .sql
> script which was also run by the buildfarm (in stead of its hardcoded 
> kludges), then
> it might be necessary to handle the additional stuff my patch did, like:

> [...]
>
> Or, maybe it's guaranteed that the animals all run latest version of old
> branches, in which case I think some of the BF's existing logic could be
> dropped, which would help to reconcile these two scripts:

I am pretty sure that it is safe to assume that all buildfarm animals
run the top of the stable branch they are testing, at least on the
community side.  An advantage of moving all those SQLs to a script
that can be process with psql thanks to the \if metacommands you have
added is that buildfarm clients are not required to immediately update
their code to work properly.  Considering as well that we should
minimize the amount of duplication between all those things, I'd like
to think that we'd better apply 0002 and consider a backpatch to allow
the buildfarm to catch up on it.  It should at least allow to remove a
good chunk of the object cleanup done directly by the buildfarm.

>> This is for the basics in terms of fixing test.sh and what should be
>> backpatched.  In this aspect patches 0001 and 0002 were a bit
>> incorrect.  I am not sure that 0003 is that interesting as designed as
>> we would miss any new core types introduced.
> 
> We wouldn't miss new core types, because of the 2nd part of type_sanity which
> tests that each core type was included in the "manytypes" table.

+-- XML might be disabled at compile-time
+AND oid != ALL(ARRAY['xml', 'gtsvector', 'pg_node_tree',
'pg_ndistinct', 'pg_dependencies', 'pg_mcv_list',
'pg_brin_bloom_summary', 'pg_brin_minmax_multi_summary']::regtype[])

I believe that this comment is incomplete, applying only to the first
element listed in this array.  I guess that this had better document
why those catalogs are part of the list?  Good to see that adding a
reg* in core would immediately be noticed though, as far as I
understand this SQL.
--
Michael


signature.asc
Description: PGP signature

Re: Question about psql meta-command with schema option doesn't use visibilityrule

[David G. Johnston]

On Sunday, November 7, 2021, Tatsuro Yamada 
wrote:

>
> I have a question that is a specification of permission check
> (visibilityrule) for psql meta-command with schema option.
>
> From the above results, I expected "\dX s_a.*" doesn't show any info
> as same as "\dX". but info is displayed. I'm wondering this behavior.
>
> I'm maybe missing something, but if this is a problem, I'll send a
> patch. Any comments are welcome!
>
>
Visibility means search_path, not permission.  If s_a is not in the
search_paths it objects are not visible unqualified but can be seen
(catalog) when schema qualified.

David J.

Re: Question about psql meta-command with schema option doesn't use visibilityrule

[David G. Johnston]

On Sunday, November 7, 2021, Tatsuro Yamada 
wrote:

>
> According to the source code [1], there is no check if a schema
> option is added. As a result, a role that is not granted can see
> other roles' object names.
> We might say it's okay because it's a name, not contents (data),
> but It seems not preferable, I think.
>

No, we are not interested in changing this long-standing documented
behavior.  The contents of the catalogs are visible to all.  So even if
this was something to consider, psql is not the correct scope.

David J.

Re: jsonb crash

[David Rowley]

On Mon, 8 Nov 2021 at 15:38, Tom Lane  wrote:
>
> David Rowley  writes:
> > I've now pushed the fix to restrict v14 to only allow Memoize when the
> > left and right types are the same.  For master, since it's possible to
> > add a field to RestrictInfo, I've changed that to cache the left and
> > right hash equality operators.
>
> If you were going to push this fix before 14.1, you should have done it
> days ago.  At this point it's not possible to get a full set of buildfarm
> results before the wrap.  The valgrind and clobber-cache animals, in
> particular, are unlikely to report back in time.

Sorry, I was under the impression that it was ok until you'd done the
stamp for the release. As far as I can see, that's not done yet.

Do you want me to back out the change I made to 14?

David

Question about psql meta-command with schema option doesn't use visibilityrule

[Tatsuro Yamada]

Hi,

I have a question that is a specification of permission check
(visibilityrule) for psql meta-command with schema option.

According to the source code [1], there is no check if a schema
option is added. As a result, a role that is not granted can see
other roles' object names.
We might say it's okay because it's a name, not contents (data),
but It seems not preferable, I think.

The following is a reproducer using \dX commands.
Note: It is not only \dX but also \d because it uses the same
permission check function (processSQLNamePattern).

The reproduction procedure (including some results):

-- Create role a, b as non-superuser
create role a nosuperuser;
create role b nosuperuser;
grant CREATE on database postgres to a;

-- Create schema s_a, table hoge, and its extend stats by role a
set role a;
create schema s_a;
create table s_a.hoge(a int, b int);
create statistics s_a.hoge_ext on a,b from s_a.hoge;
set search_path to public, s_a;

-- Run \dX and \dX s_a.* by role a: OK (since schema s_a was created by role a)
\dX
   List of extended statistics
 Schema |   Name   |   Definition   | Ndistinct | Dependencies |   MCV
+--++---+--+-
 s_a| hoge_ext | a, b FROM hoge | defined   | defined  | defined
(1 row)

\dX s_a.*
   List of extended statistics
 Schema |   Name   |   Definition   | Ndistinct | Dependencies |   MCV
+--++---+--+-
 s_a| hoge_ext | a, b FROM hoge | defined   | defined  | defined
(1 row)

-- Run \dX by role b: OK
--  (not displayed is fine since role b can't see info of role a)
reset role;
set role b;
\dX
 List of extended statistics
 Schema | Name | Definition | Ndistinct | Dependencies | MCV
+--++---+--+-
(0 rows)

-- Run \dX with schema by role b: OK?? (It should be NG?)
-- this case is a point in my question
\dX s_a.*
 List of extended statistics
 Schema |   Name   | Definition | Ndistinct | Dependencies |   MCV
+--++---+--+-
 s_a| hoge_ext | a, b FROM s_a.hoge | defined   | defined  | defined
(1 row)

-- clean-up
reset role;
drop schema s_a cascade;
revoke CREATE on DATABASE postgres FROM a;
drop role a;
drop role b;


From the above results, I expected "\dX s_a.*" doesn't show any info
as same as "\dX". but info is displayed. I'm wondering this behavior.

I'm maybe missing something, but if this is a problem, I'll send a
patch. Any comments are welcome!


[1]: processSQLNamePattern in src/fe_utils/string_utils.c
if (schemabuf.len > 2)
{
/* We have a schema pattern, so constrain the schemavar */

/* Optimize away a "*" pattern */
if (strcmp(schemabuf.data, "^(.*)$") != 0 && schemavar)
{
WHEREAND();
appendPQExpBuffer(buf, "%s OPERATOR(pg_catalog.~) ", schemavar);
appendStringLiteralConn(buf, schemabuf.data, conn);
if (PQserverVersion(conn) >= 12)
appendPQExpBufferStr(buf, " COLLATE pg_catalog.default");
appendPQExpBufferChar(buf, '\n');
}
}
else
{
/* No schema pattern given, so select only visible objects */
if (visibilityrule)
{
WHEREAND();
appendPQExpBuffer(buf, "%s\n", visibilityrule);
}
}



Thanks,
Tatsuro Yamada

Re: jsonb crash

[Tom Lane]

David Rowley  writes:
> I've now pushed the fix to restrict v14 to only allow Memoize when the
> left and right types are the same.  For master, since it's possible to
> add a field to RestrictInfo, I've changed that to cache the left and
> right hash equality operators.

If you were going to push this fix before 14.1, you should have done it
days ago.  At this point it's not possible to get a full set of buildfarm
results before the wrap.  The valgrind and clobber-cache animals, in
particular, are unlikely to report back in time.

regards, tom lane

Re: Commitfest 2021-11 Patch Triage - Part 1

[Kyotaro Horiguchi]

Thanks for the summary.

At Fri, 5 Nov 2021 22:15:49 +0100, Daniel Gustafsson  wrote in 
> 2490: Make message at end-of-recovery less scary
> 
> This thread stalled, but has had recent interest.  The patch no longer applies
> so while the patch has support it will be marked as returned with feedback
> during this CF unless revived.

I'll soon repost a rebased version.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: jsonb crash

[David Rowley]

On Sat, 6 Nov 2021 at 11:38, Justin Pryzby  wrote:
>
> On Tue, Oct 26, 2021 at 07:07:01PM +1300, David Rowley wrote:
> > Does anyone have any thoughts on the proposed fixes?
>
> I don't have any thoughts, but I want to be sure it isn't forgotten.

Not forgotten. I was just hoping to get some feedback.

I've now pushed the fix to restrict v14 to only allow Memoize when the
left and right types are the same.  For master, since it's possible to
add a field to RestrictInfo, I've changed that to cache the left and
right hash equality operators.

This does not fix the binary / logical issue mentioned by Tom.  I have
ideas about allowing Memoize to operate in a binary equality mode or
logical equality mode.  I'll need to run in binary mode when there are
lateral vars or when any join operator is not hashable.

David

Re: Allow escape in application_name

[Kyotaro Horiguchi]

At Sun, 7 Nov 2021 13:35:39 +0900, Fujii Masao  
wrote in 
> 
> 
> On 2021/11/05 12:17, Kyotaro Horiguchi wrote:
> If possible, I'd like to see this change as a separate patch
> and commt it first because this is the description for
> the existing parameter postgres_fdw.application_name.

Fair enough.

> >> I'd like to hear more opinions about this from others.
> >> But *if* there is actually no use case, I'd like not to add
> >> the feature, to make the code simpler.
> > I think padding is useful because it alingns the significant content
> > of log lines by equating the length of the leading fixed
> > In short, I'm for to removing it by +0.7.
> 
> So our current consensus is to remove the padding part
> from postgres_fdw.application_name.

I think so.

> >> +  case 'u':
> >> +  Assert(MyProcPort != NULL);
> >>
> >> Isn't it enough to perform this assertion check only once
> >> at the top of parse_pgfdw_appname()?
> > Yeah, in either way, we should treat them in the same way.
> > 
> >>> We can force parse_pgfdw_appname() not to be called if MyProcPort does
> >>> not exist,
> >>> but I don't think it is needed now.
> >>
> >> Yes.
> > (I assume you said "it is needed now".)  I'm not sure how to force
> > that but if it means a NULL MyProcPort cuases a crash, I think
> > crashing server is needlessly too aggressive as the penatly.
> 
> I said "Yes" for Kuroda-san's comment "I don't think it is
> needed now". So I meant that "it is NOT needed now".
> Sorry for unclear comment..
> 
> His idea was to skip calling parse_pgfdw_appname() if
> MyProcPort is NULL, so as to prevent parse_pgfdw_appname()
> from seeing NULL pointer of MyProcPort. But he thought
> it's not necessary now, and I agree with him because
> the idea would cause more confusing behavior.
> 
> 
> > It seems to me that postgres-fdw asumes a valid user id, but doesn't
> > make no use of databsae, server port, and process id.  What I thought
> > here is that making it an assertion is too much. So just ignoring the
> > replacement is also fine to me.
> > That being said, if we are eager not to have unused code paths, it is
> > reasonable enough.  I don't object strongly to replace it with an
> > assertion.
> 
> So no one strongly objects to the addition of assertion?

It seems to me so.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: lastOverflowedXid does not handle transaction ID wraparound

[Kyotaro Horiguchi]

At Sat, 6 Nov 2021 19:16:09 +0300, Alexander Korotkov  
wrote in 
> Pushed!

Thanks!

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: GiST operator class for bool

[Tomas Vondra]

On 11/7/21 20:53, Tomas Vondra wrote:

Hi,

On 11/7/21 17:44, Tom Lane wrote:

Tomas Vondra  writes:

Pushed, after adding some simple EXPLAIN to the regression test.


skink is reporting that this has some valgrind issues [1].
I suspect sloppy conversion between bool and Datum, but
didn't go looking.



It's actually a bit worse than that :-( The opclass is somewhat confused 
about the type it should use for storage. The gbtree_ninfo struct says 
it's using gbtreekey4, the SQL script claims the params are gbtreekey8, 
and it should actually use gbtreekey2. Sorry for not noticing that.


The attached patch fixes the valgrind error for me.



I've pushed the fix, hopefully that'll make skink happy.

What surprised me a bit is that the opclass used gbtreekey4 storage, the 
equality support proc was defined as using gbtreekey8


FUNCTION7gbt_bool_same (gbtreekey8, gbtreekey8, internal),

yet the gistvalidate() did not report this. Turns out this is because

ok = check_amproc_signature(procform->amproc, INTERNALOID, false,
3, 3, opckeytype, opckeytype,
INTERNALOID);

i.e. with exact=false, so these type differences are ignored. Changing 
it to true reports the issue (and no other issues in check-world).


But maybe there are reasons to keep using false?

regards

--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Companydiff --git a/src/backend/access/gist/gistvalidate.c b/src/backend/access/gist/gistvalidate.c
index b885fa2b25..31cc4f0a21 100644
--- a/src/backend/access/gist/gistvalidate.c
+++ b/src/backend/access/gist/gistvalidate.c
@@ -131,7 +131,7 @@ gistvalidate(Oid opclassoid)
 			2, 2, INTERNALOID, INTERNALOID);
 break;
 			case GIST_EQUAL_PROC:
-ok = check_amproc_signature(procform->amproc, INTERNALOID, false,
+ok = check_amproc_signature(procform->amproc, INTERNALOID, true,
 			3, 3, opckeytype, opckeytype,
 			INTERNALOID);
 break;

Re: [PATCH] postgres_fdw: suppress explicit casts in text:text comparisons (was: column option to override foreign types)

[Dian M Fay]

On Tue Nov 2, 2021 at 7:10 PM EDT, Tom Lane wrote:
> I wrote:
> > Now that I've looked this over I'm starting to feel uncomfortable
> > again, because we can't actually be quite sure about how the remote
> > parser's heuristic will act.
>
> Actually ... we could make that a lot safer by insisting that the
> other input be a plain Var, which'd necessarily be a column of the
> foreign table. That would still cover most cases of practical
> interest, I think, and it would remove any question of whether
> implicit coercions had snuck in. It's more restrictive than I'd
> really like, but I think it's less likely to cause problems.

Here's v6! I started with restricting cast suppression to Const-Var
comparisons as you suggested. A few tests did regress (relative to the
unrestricted version) right out of the gate with comparisons to varchar
columns, since those become RelabelType nodes instead of Vars. After
reading the notes on RelabelType in primnodes.h, I *think* that that
"dummy" coercion is distinct from the operator input type coercion
you're talking about here:

> What we're checking is that leftType and rightType match, but that
> condition is applied to the inputs *after implicit type coercion to
> the operator's input types*. We can't be entirely sure about what our
> parser saw to begin with. Perhaps it'd be a good idea to strip any
> implicit coercions on the non-Const input before checking its type.

I allowed RelabelTypes over Vars to suppress casts as well. It's working
for me so far and the varchar comparison tests are back to passing, sans
casts.
From e0fe4e4b15403ffab2518f945c13ce0e22bb89a9 Mon Sep 17 00:00:00 2001
From: Dian M Fay 
Date: Sun, 7 Nov 2021 17:53:17 -0500
Subject: [PATCH v6] Suppress explicit casts of safe Consts in postgres_fdw

Comparisons between Consts of UNKNOWN type (string literals or NULLs)
and Vars can rely on the remote server's operator resolution heuristic
to match the Const to the remote column type instead of forcing the type
from the local schema definition. This allows the local schema to
diverge usefully from the remote, for example by declaring a remote enum
column as text and eliminating the need to keep the enum synchronized
across databases.
---
 contrib/postgres_fdw/deparse.c|  81 ++--
 .../postgres_fdw/expected/postgres_fdw.out| 125 --
 contrib/postgres_fdw/sql/postgres_fdw.sql |  18 +++
 3 files changed, 177 insertions(+), 47 deletions(-)

diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c
index d98bd66681..2868d95a03 100644
--- a/contrib/postgres_fdw/deparse.c
+++ b/contrib/postgres_fdw/deparse.c
@@ -2695,9 +2695,14 @@ deparseVar(Var *node, deparse_expr_cxt *context)
  * Deparse given constant value into context->buf.
  *
  * This function has to be kept in sync with ruleutils.c's get_const_expr.
- * As for that function, showtype can be -1 to never show "::typename" 
decoration,
- * or +1 to always show it, or 0 to show it only if the constant wouldn't be 
assumed
- * to be the right type by default.
+ *
+ * As in that function, showtype can be -1 to never show "::typename"
+ * decoration, +1 to always show it, or 0 to show it only if the constant
+ * wouldn't be assumed to be the right type by default.
+ *
+ * In addition, this code allows showtype to be -2 to indicate that we should
+ * not show "::typename" decoration if the constant is printed as an untyped
+ * literal or NULL (while in other cases, behaving as for showtype == 0).
  */
 static void
 deparseConst(Const *node, deparse_expr_cxt *context, int showtype)
@@ -2707,6 +2712,7 @@ deparseConst(Const *node, deparse_expr_cxt *context, int 
showtype)
booltypIsVarlena;
char   *extval;
boolisfloat = false;
+   boolisstring = false;
boolneedlabel;
 
if (node->constisnull)
@@ -2762,13 +2768,14 @@ deparseConst(Const *node, deparse_expr_cxt *context, 
int showtype)
break;
default:
deparseStringLiteral(buf, extval);
+   isstring = true;
break;
}
 
pfree(extval);
 
-   if (showtype < 0)
-   return;
+   if (showtype == -1)
+   return; /* never print type 
label */
 
/*
 * For showtype == 0, append ::typename unless the constant will be
@@ -2788,7 +2795,13 @@ deparseConst(Const *node, deparse_expr_cxt *context, int 
showtype)
needlabel = !isfloat || (node->consttypmod >= 0);
break;
default:
-   needlabel = true;
+   if (showtype == -2)
+   {
+   /* label unless we printed it as an untyped 
string */
+   needlabel = !isstring;
+   }
+ 

Re: Commitfest 2021-11 Patch Triage - Part 1

[Alvaro Herrera]

On 2021-Nov-05, Daniel Gustafsson wrote:

> 2716: fix spinlock contention in LogwrtResult
> =
> This addresses a bottleneck which definitely seems like one we want to fix, I
> don't have a hard time imagining it impacting other production usecases then
> the reported one.  The goalposts moved early in the thread, and while there 
> has
> been work on it the patch has stalled on Andres' review which raised some
> concerns.  Are you still pursuing this Alvaro?

I am, but not in the immediate future.  I am very absorbed by trying to
make MERGE work again for resubmission, so I haven't had time to invest
in these other patches.  I hope I'll be able to return with a new
version of this for the January commitfest.

-- 
Álvaro Herrera PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"On the other flipper, one wrong move and we're Fatal Exceptions"
(T.U.X.: Term Unit X  - http://www.thelinuxreview.com/TUX/)

Re: Schema variables - new implementation for Postgres 15

[Tomas Vondra]

On 11/6/21 04:45, Pavel Stehule wrote:

Hi

st 3. 11. 2021 v 14:05 odesílatel Tomas Vondra 
mailto:tomas.von...@enterprisedb.com>> 
napsal:


Hi,

I took a quick look at the latest patch version. In general the patch
looks pretty complete and clean, and for now I have only some basic
comments. The attached patch tweaks some of this, along with a couple
additional minor changes that I'll not discuss here.


1) Not sure why we need to call this "schema variables". Most objects
are placed in a schema, and we don't say "schema tables" for example.
And it's CREATE VARIABLE and not CREATE SCHEMA VARIABLE, so it's a bit
inconsistent.


Yes, there is inconsistency, but I think it is necessary. The name 
"variable" is too generic. Theoretically we can use other adjectives 
like session variables or global variables and the name will be valid. 
But it doesn't describe the fundamentals of design. This is similar to 
the package's variables from PL/SQL. These variables are global, 
session's variables too. But the usual name is "package variables". So 
schema variables are assigned to schemes, and I think a good name can be 
"schema variables". But it is not necessary to repeat keyword schema in 
the CREATE COMMAND.


My opinion is not too strong in this case, and I can accept just 
"variables" or "session's variables" or "global variables", but I am not 
sure if these names describe this feature well, because still they are 
too generic. There are too many different implementations of session 
global variables (see PL/SQL or T-SQL or DB2).




OK. "Session variable" seems better to me, but I'm not sure how well 
that matches other databases. I'm not sure how much should we feel 
constrained by naming in other databases, though.




The docs actually use "Global variables" in one place for some reason.


2) I find this a bit confusing:

SELECT non_existent_variable;
test=# select s;
ERROR:  column "non_existent_variable" does not exist
LINE 1: select non_existent_variable;

I wonder if this means using SELECT to read variables is a bad idea, and
we should have a separate command, just like we have LET (instead of
just using UPDATE in some way).


I am sure so I want to use variables in SELECTs. One interesting case is 
using variables in RLS.




How much more complicated would it be without the SELECT?

I prefer to fix this error message to "column or variable ... does not 
exist"




Not sure it's a good idea to make the error message more ambiguous. Most 
people won't use variables at all, and the message will be less clear 
for them.



regards

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

Re: Schema variables - new implementation for Postgres 15

[Tomas Vondra]

On 11/6/21 16:40, Pavel Stehule wrote:



so 6. 11. 2021 v 15:57 odesílatel Justin Pryzby > napsal:


On Sat, Nov 06, 2021 at 04:45:19AM +0100, Pavel Stehule wrote:
 > st 3. 11. 2021 v 14:05 odesílatel Tomas Vondra
mailto:tomas.von...@enterprisedb.com>> napsal:
 > > 1) Not sure why we need to call this "schema variables". Most
objects
 > > are placed in a schema, and we don't say "schema tables" for
example.
 > > And it's CREATE VARIABLE and not CREATE SCHEMA VARIABLE, so
it's a bit
 > > inconsistent.

+1

At least the error messages need to be consistent.
It doesn't make sense to have both of these:

+               elog(ERROR, "cache lookup failed for schema variable
%u", varid);
+               elog(ERROR, "cache lookup failed for variable %u",
varid);

 > Yes, there is inconsistency, but I think it is necessary. The name
 > "variable" is too generic. Theoretically we can use other
adjectives like
 > session variables or global variables and the name will be valid.
But it
 > doesn't describe the fundamentals of design. This is similar to the
 > package's variables from PL/SQL. These variables are global,
session's
 > variables too. But the usual name is "package variables". So schema
 > variables are assigned to schemes, and I think a good name can be
"schema
 > variables". But it is not necessary to repeat keyword schema in
the CREATE
 > COMMAND.
 >
 > My opinion is not too strong in this case, and I can accept just
 > "variables" or "session's variables" or "global variables", but I
am not
 > sure if these names describe this feature well, because still
they are too
 > generic. There are too many different implementations of session
global
 > variables (see PL/SQL or T-SQL or DB2).

I would prefer "session variable".

To me, this feature seems similar to a CTE (which exists for a single
statement), or a temporary table (which exists for a single
transaction).  So
"session" conveys a lot more of its meaning than "schema".


It depends on where you are looking. There are two perspectives - data 
and metadata. And if I use data perspective, then it is session related. 
If I use metadata perspective, then it can be persistent or temporal 
like tables.


I think you mean "temporary" not "temporal". This really confused me for 
a while, because temporal means "involving time" (e.g. a table with 
from/to timestamp range, etc).


I see strong similarity with Global Temporary Tables - but 
I think naming "local temporary tables" and "global temporary tables" 
can be not intuitive or messy for a lot of people too.


Right, it's a bit like global temporary tables, in the sense that 
there's a shared definition but local (session) state.


Anyway, if people will try to find this feature on Google, then 
probably use keywords "session variables", so maybe my preference of

more technical terminology is obscure and not practical, and the name
"session variables" can be more practical for other people.

Hmmm, maybe.


If I use the system used for GTT - then the exact name can be "Global
Session Variable". Can we use this name? Or shortly just Session
Variables because we don't support local session variables now.


So a "local variable" would be defined just for a given session, just 
like a temporary table? Wouldn't that have the same issues with catalog 
bloat as temporary tables?


I'd probably vote for "session variables". We can call it local/global 
session variables in the future, if we end up implementing that.



regards

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

Re: GiST operator class for bool

[Tomas Vondra]

Hi,

On 11/7/21 17:44, Tom Lane wrote:

Tomas Vondra  writes:

Pushed, after adding some simple EXPLAIN to the regression test.


skink is reporting that this has some valgrind issues [1].
I suspect sloppy conversion between bool and Datum, but
didn't go looking.



It's actually a bit worse than that :-( The opclass is somewhat confused 
about the type it should use for storage. The gbtree_ninfo struct says 
it's using gbtreekey4, the SQL script claims the params are gbtreekey8, 
and it should actually use gbtreekey2. Sorry for not noticing that.


The attached patch fixes the valgrind error for me.


regards

--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Companydiff --git a/contrib/btree_gist/btree_bool.c b/contrib/btree_gist/btree_bool.c
index 25ce1e2b77..1be246ea5e 100644
--- a/contrib/btree_gist/btree_bool.c
+++ b/contrib/btree_gist/btree_bool.c
@@ -72,7 +72,7 @@ static const gbtree_ninfo tinfo =
 {
 	gbt_t_bool,
 	sizeof(bool),
-	4,			/* sizeof(gbtreekey4) */
+	2,			/* sizeof(gbtreekey2) */
 	gbt_boolgt,
 	gbt_boolge,
 	gbt_booleq,
diff --git a/contrib/btree_gist/btree_gist--1.6--1.7.sql b/contrib/btree_gist/btree_gist--1.6--1.7.sql
index 0e68356982..1085216501 100644
--- a/contrib/btree_gist/btree_gist--1.6--1.7.sql
+++ b/contrib/btree_gist/btree_gist--1.6--1.7.sql
@@ -4,6 +4,21 @@
 \echo Use "ALTER EXTENSION btree_gist UPDATE TO '1.7'" to load this file. \quit
 
 -- This upgrade scripts add support for bool.
+CREATE FUNCTION gbtreekey2_in(cstring)
+RETURNS gbtreekey2
+AS 'MODULE_PATHNAME', 'gbtreekey_in'
+LANGUAGE C IMMUTABLE STRICT PARALLEL SAFE;
+
+CREATE FUNCTION gbtreekey2_out(gbtreekey2)
+RETURNS cstring
+AS 'MODULE_PATHNAME', 'gbtreekey_out'
+LANGUAGE C IMMUTABLE STRICT PARALLEL SAFE;
+
+CREATE TYPE gbtreekey2 (
+	INTERNALLENGTH = 2,
+	INPUT  = gbtreekey2_in,
+	OUTPUT = gbtreekey2_out
+);
 
 -- Define the GiST support methods
 CREATE FUNCTION gbt_bool_consistent(internal,bool,int2,oid,internal)
@@ -32,11 +47,11 @@ AS 'MODULE_PATHNAME'
 LANGUAGE C IMMUTABLE STRICT;
 
 CREATE FUNCTION gbt_bool_union(internal, internal)
-RETURNS gbtreekey8
+RETURNS gbtreekey2
 AS 'MODULE_PATHNAME'
 LANGUAGE C IMMUTABLE STRICT;
 
-CREATE FUNCTION gbt_bool_same(gbtreekey8, gbtreekey8, internal)
+CREATE FUNCTION gbt_bool_same(gbtreekey2, gbtreekey2, internal)
 RETURNS internal
 AS 'MODULE_PATHNAME'
 LANGUAGE C IMMUTABLE STRICT;
@@ -57,6 +72,6 @@ AS
 	FUNCTION	4	gbt_decompress (internal),
 	FUNCTION	5	gbt_bool_penalty (internal, internal, internal),
 	FUNCTION	6	gbt_bool_picksplit (internal, internal),
-	FUNCTION	7	gbt_bool_same (gbtreekey8, gbtreekey8, internal),
+	FUNCTION	7	gbt_bool_same (gbtreekey2, gbtreekey2, internal),
 	FUNCTION	9   gbt_bool_fetch (internal),
-	STORAGE		gbtreekey8;
+	STORAGE		gbtreekey2;

Re: pg_upgrade test for binary compatibility of core data types

[Justin Pryzby]

On Mon, Oct 11, 2021 at 02:38:12PM +0900, Michael Paquier wrote:
> On Fri, Oct 01, 2021 at 04:58:41PM +0900, Michael Paquier wrote:
> > I was looking at this CF entry, and what you are doing in 0004 to move
> > the tweaks from pg_upgrade's test.sh to a separate SQL script that
> > uses psql's meta-commands like \if to check which version we are on is
> > really interesting.  The patch does not apply anymore, so this needs a
> > rebase.  The entry has been switched as waiting on author by Tom, but
> > you did not update it after sending the new versions in [1].  I am
> > wondering if we could have something cleaner than just a set booleans
> > as you do here for each check, as that does not help with the
> > readability of the tests.
> 
> And so, I am back at this thread, looking at the set of patches
> proposed from 0001 to 0004.  The patches are rather messy and mix many
> things and concepts, but there are basically four things that stand
> out:
> - test.sh is completely broken when using PG >= 14 as new version
> because of the removal of the test tablespace.  Older versions of
> pg_regress don't support --make-tablespacedir so I am fine to stick a
> couple of extra mkdirs for testtablespace/, expected/ and sql/ to
> allow the script to work properly for major upgrades as a workaround,
> but only if we use an old version.  We need to do something here for
> HEAD and REL_14_STABLE.
> - The script would fail when using PG <= 11 as old version because of
> WITH OIDS relations.  We need to do something down to REL_12_STABLE.
> I did not like much the approach taken to stick 4 ALTER TABLE queries
> though (the patch was actually failing here for me), so instead I have
> borrowed what the buildfarm has been doing with a DO block.  That
> works fine, and that's more portable.
> - Not using --extra-float-digits with PG <= 11 as older version causes
> a bunch of diffs in the dumps, making the whole unreadable.  The patch
> was doing that unconditionally for *all version*, which is not good.
> We should only do that on the versions that need it, and we know the
> old version number before taking any dumps so that's easy to check.
> - The addition of --wal-segsize and --allow-group-access breaks the
> script when using PG < 10 at initdb time as these got added in 11.
> With 10 getting EOL'd next year and per the lack of complaints, I am
> not excited to do anything here and I'd rather leave this out so as we
> keep coverage for those options across *all* major versions upgraded
> from 11~.  The buildfarm has tests down to 9.2, but for devs my take
> is that this is enough for now.

Michael handled those in fa66b6d.
Note that the patch assumes that the "old version" being pg_upgraded has
commit 97f73a978: "Work around cross-version-upgrade issues created by commit 
9e38c2bb5."

That may be good enough for test.sh, but if the kludges were moved to a .sql
script which was also run by the buildfarm (in stead of its hardcoded kludges), 
then
it might be necessary to handle the additional stuff my patch did, like:

+   DROP TRANSFORM FOR integer LANGUAGE sql 
CASCADE;"
+   DROP FUNCTION boxarea(box);"
+   DROP FUNCTION funny_dup17();"
+   DROP TABLE abstime_tbl;"
+   DROP TABLE reltime_tbl;"
+   DROP TABLE tinterval_tbl;"
+   DROP AGGREGATE 
first_el_agg_any(anyelement);"
+   DROP AGGREGATE 
array_cat_accum(anyarray);"
+   DROP OPERATOR @#@(NONE,bigint);"

Or, maybe it's guaranteed that the animals all run latest version of old
branches, in which case I think some of the BF's existing logic could be
dropped, which would help to reconcile these two scripts:

my $missing_funcs = q{drop function if exists 
public.boxarea(box);
 
  drop function if exists public.funny_dup17(); 

   
..  

   
$prstmt = join(';', 

   
'drop operator @#@ (NONE, bigint)', 

   
.. 

Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.

[Andres Freund]

Hi,

On 2021-11-05 08:54:37 -0400, Robert Haas wrote:
> On Thu, Nov 4, 2021 at 6:46 PM Andres Freund  wrote:
> > What about extending GRANT to allow to grant rights on commands? Yes, it'd 
> > be
> > a bit of work to make that work in the catalogs, but it doesn't seem too 
> > hard
> > to tackle.
> 
> I think that there aren't too many commands where the question is just
> whether you can execute the command or not. CHECKPOINT is one that
> does work that way, but if it's VACUUM or ANALYZE the question will be
> whether you can run it on a particular table; if it's ALTER SYSTEM it
> will be whether you can run it for that GUC; and so on. CHECKPOINT is
> one of the few commands that has no target.

I don't know if that's really such a big deal. It's useful to be able to grant
the right to do a system wide ANALYZE etc to a role that can't otherwise do
anything with the table. Even for ALTER SYSTEM etc it seems like it'd be
helpful, because it allows to constrain an admin tool to "legitimate" admin
paths, without allowing, say, UPDATE pg_proc.

Greetings,

Andres Freund

Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.

[Andres Freund]

On 2021-11-05 08:42:58 -0400, Robert Haas wrote:
> On Thu, Nov 4, 2021 at 7:38 PM Jeff Davis  wrote:
> > It seems like this specific approach has been mostly shot down already.
> >  But out of curiosity, are you intending to run CHECKPOINT during
> > bootstrap or something?
> 
> Imagine a system with corruption in pg_proc. Right now, that won't
> prevent you from successfully executing a checkpoint. With this
> approach, it might.

Exactly. It wouldn't matter if checkpoints weren't something needed to
potentially bring the system back into a sane state, but ...

Re: amcheck's verify_heapam(), and HOT chain verification

[Mark Dilger]

> On Nov 6, 2021, at 3:09 PM, Peter Geoghegan  wrote:
> 
> I am quite willing to help out with all this, if you're interested.

Yes, I am quite interested, though I will have to alternate between this work 
and the various patch sets that I've already submitted for this development 
cycle.

I think we need a corruption generating framework that can be deployed on the 
buildfarm.  What I have in mind is inspired by your comments about the "freeze 
the dead" bug.  We need to be able to build versions of the database with known 
bugs enabled, both real-world bugs from the past and contrived bugs we write 
only for this purpose, so as to create non-trivial corruption on the build 
farm.  I think it would be sufficient if such builds were run less frequently, 
perhaps triggered by commits to a corruption testing branch?  We could keep the 
old bugs alive on that branch while periodically merging in everything else 
from HEAD?  That seems a tad tiresome, but maybe it wouldn't be too bad if the 
intentional bugs were limited to just a few backend files, and as much as 
possible in code at the end of the file or in separate files to reduce merge 
conflicts?

I'm cc'ing Andrew to get his thoughts about the buildfarm integration

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

Re: GiST operator class for bool

[Tom Lane]

Tomas Vondra  writes:
> Pushed, after adding some simple EXPLAIN to the regression test.

skink is reporting that this has some valgrind issues [1].
I suspect sloppy conversion between bool and Datum, but
didn't go looking.

==1805451== VALGRINDERROR-BEGIN
==1805451== Uninitialised byte(s) found during client check request
==1805451==at 0x59EFEA: PageAddItemExtended (bufpage.c:346)
==1805451==by 0x2100B9: gistfillbuffer (gistutil.c:46)
==1805451==by 0x2050F9: gistplacetopage (gist.c:562)
==1805451==by 0x20546B: gistinserttuples (gist.c:1277)
==1805451==by 0x205BB5: gistinserttuple (gist.c:1230)
==1805451==by 0x206067: gistdoinsert (gist.c:885)
==1805451==by 0x2084FB: gistBuildCallback (gistbuild.c:829)
==1805451==by 0x23B572: heapam_index_build_range_scan 
(heapam_handler.c:1694)
==1805451==by 0x208E7D: table_index_build_scan (tableam.h:1756)
==1805451==by 0x208E7D: gistbuild (gistbuild.c:309)
==1805451==by 0x2D10C8: index_build (index.c:2983)
==1805451==by 0x2D2A7D: index_create (index.c:1232)
==1805451==by 0x383E67: DefineIndex (indexcmds.c:1128)
==1805451==  Address 0x10cab1e4 is 12 bytes inside a block of size 16 
client-defined
==1805451==at 0x712AC5: palloc0 (mcxt.c:1118)
==1805451==by 0x1E0A07: index_form_tuple (indextuple.c:146)
==1805451==by 0x210BA8: gistFormTuple (gistutil.c:582)
==1805451==by 0x2084C2: gistBuildCallback (gistbuild.c:813)
==1805451==by 0x23B572: heapam_index_build_range_scan 
(heapam_handler.c:1694)
==1805451==by 0x208E7D: table_index_build_scan (tableam.h:1756)
==1805451==by 0x208E7D: gistbuild (gistbuild.c:309)
==1805451==by 0x2D10C8: index_build (index.c:2983)
==1805451==by 0x2D2A7D: index_create (index.c:1232)
==1805451==by 0x383E67: DefineIndex (indexcmds.c:1128)
==1805451==by 0x5AED2E: ProcessUtilitySlow (utility.c:1535)
==1805451==by 0x5AE262: standard_ProcessUtility (utility.c:1066)
==1805451==by 0x5AE33A: ProcessUtility (utility.c:527)
==1805451== 
==1805451== VALGRINDERROR-END

regards, tom lane

[1] 
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=skink=2021-11-06%2023%3A56%3A57

Re: Commitfest 2021-11 Patch Triage - Part 1

[Daniel Gustafsson]

> On 6 Nov 2021, at 02:12, Andy Fan  wrote:
> 
>> 1741: Index Skip Scan
>> =
>> An often requested feature which has proven hard to reach consensus on an
>> implementation for.  The thread(s) have stalled since May,
> 
> This statement is not accurate.  Peter started a new thread in [1] for this
> topic last month and then we had a discussion. Thanks for taking
> care of this.

Aha, I had missed that.  Thanks to Dimitry for attaching this thread to the CF
entry to make that clearer going forward.

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

Re: Commitfest 2021-11 Patch Triage - Part 1

[Daniel Gustafsson]

> On 6 Nov 2021, at 17:20, Tomas Vondra  wrote:
> 
> On 11/5/21 22:15, Daniel Gustafsson wrote:
>> ...
>> 1651: GROUP BY optimization
>> ===
>> This is IMO a desired optimization, and after all the heavy lifting by Tomas
>> the patch seems to be in pretty good shape.
> 
> I agree it's desirable.  To move the patch forward, I need some feedback on 
> the
> costing questions,


Tom or David perhaps have some thoughts on that?

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

Re: Commitfest 2021-11 Patch Triage - Part 1

[Daniel Gustafsson]

> On 7 Nov 2021, at 10:26, Etsuro Fujita  wrote:
> 
> On Sat, Nov 6, 2021 at 6:16 AM Daniel Gustafsson  wrote:
>> 2601: Fast COPY FROM command for the foreign tables
>> ===
>> This approach taken in this patch has stabilized and the benchmarks posted 
>> are
>> very promising.  It seems pretty uncontroversial to add COPY to the FDW API 
>> SQL
>> spec wise.  Amit, Justin and Takayuki-san has done a lot of review and the
>> patch is marked Ready for Committer.  Any committer interested in taking this
>> on?  Tomas?
> 
> I’d like to work on this unless Tomas (or anyone else) want.

Great, thanks for picking it up!

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

Re: [PROPOSAL] new diagnostic items for the dynamic sql

[Zhihong Yu]

On Sun, Nov 7, 2021 at 5:23 AM Dinesh Chemuduru 
wrote:

> Hi Pavel,
>
> On Sun, 7 Nov 2021 at 12:53, Pavel Stehule 
> wrote:
>
>> Hi
>>
>> pá 5. 11. 2021 v 19:27 odesílatel Dinesh Chemuduru <
>> dinesh.ku...@migops.com> napsal:
>>
>>> Hi Daniel,
>>>
>>> Thank you for your follow up, and attaching a new patch which addresses
>>> Pavel's comments.
>>> Let me know If I miss anything here.
>>>
>>>
>>> On Thu, 4 Nov 2021 at 17:40, Daniel Gustafsson  wrote:
>>>
 > On 9 Sep 2021, at 08:23, Dinesh Chemuduru 
 wrote:

 > Let me try to fix the suggested case(I tried to fix this case in the
 past, but this time I will try to spend more time on this), and will submit
 a new patch.

 This CF entry is marked Waiting on Author, have you had the chance to
 prepare a
 new version of the patch addressing the comments from Pavel?

>>>
>> I think the functionality is correct. I am not sure about implementation
>>
>>
> Thank you for your time in validating this patch.
>
>
>> 1. Is it necessary to introduce a new field "current_query"? Cannot be
>> used "internalquery" instead? Introducing a new field opens some questions
>> - what is difference between internalquery and current_query, and where and
>> when have to be used first and when second? ErrorData is a fundamental
>> generic structure for Postgres, and can be confusing to enhance it by one
>> field just for one purpose, that is not used across Postgres.
>>
>> Internalquery is the one, which was generated by another command.
> For example, "DROP  CASCADE"(current_query) will generate many
> internal query statements for each of the dependent objects.
>
> At this moment, we do save the current_query in PG_CONTEXT.
> As we know, PG_CONTEXT returns the whole statements as stacktrace and it's
> hard to fetch the required SQL from it.
>
> I failed to see another way to access the current_query apart from the
> PG_CONTEXT.
> Hence, we have introduced the new member "current_query" to the
> "ErrorData" object.
>
> We tested the internalquery for this purpose, but there are few(10+ unit
> test) test cases that failed.
> This is also another reason we added this new member to the "ErrorData",
> and with the current patch all test cases are successful,
> and we are not disturbing the existing functionality.
>
>
>> 2. The name set_current_err_query is not consistent with names in elog.c
>> - probably something like errquery or set_errquery or set_errcurrent_query
>> can be more consistent with other names.
>>
>> Updated as per your suggestion
>
> Please find the new patch version.
>
>
>> Regards
>>
>> Pavel
>>
>>
>>
 --
 Daniel Gustafsson   https://vmware.com/

 Hi,

+set_errcurrent_query (const char *query)

You can remove the space prior to (.
I wonder if the new field can be named current_err_query because that's
what the setter implies.
current_query may give the impression that the field can store normal query
(which doesn't cause exception).
The following code implies that only one of internalquery and current_query
would be set.

+   if (estate->cur_error->internalquery)
+   exec_assign_c_string(estate, var,
estate->cur_error->internalquery);
+   else
+   exec_assign_c_string(estate, var,
estate->cur_error->current_query);

Cheers

Re: Skipping logical replication transactions on subscriber side

[Masahiko Sawada]

On Fri, Nov 5, 2021 at 12:57 AM vignesh C  wrote:
>
> On Fri, Oct 29, 2021 at 10:55 AM Masahiko Sawada  
> wrote:
> >
> > On Thu, Oct 28, 2021 at 7:40 PM Amit Kapila  wrote:
> > >
> > > On Thu, Oct 28, 2021 at 10:36 AM Masahiko Sawada  
> > > wrote:
> > > >
> > > > On Wed, Oct 27, 2021 at 7:02 PM Amit Kapila  
> > > > wrote:
> > > > >
> > > > > On Thu, Oct 21, 2021 at 10:29 AM Masahiko Sawada 
> > > > >  wrote:
> > > > > >
> > > > > >
> > > > > > I've attached updated patches.
> > > >
> > > > Thank you for the comments!
> > > >
> > > > >
> > > > > Few comments:
> > > > > ==
> > > > > 1. Is the patch cleaning tablesync error entries except via vacuum? If
> > > > > not, can't we send a message to remove tablesync errors once tablesync
> > > > > is successful (say when we reset skip_xid or when tablesync is
> > > > > finished) or when we drop subscription? I think the same applies to
> > > > > apply worker. I think we may want to track it in some way whether an
> > > > > error has occurred before sending the message but relying completely
> > > > > on a vacuum might be the recipe of bloat. I think in the case of a
> > > > > drop subscription we can simply send the message as that is not a
> > > > > frequent operation. I might be missing something here because in the
> > > > > tests after drop subscription you are expecting the entries from the
> > > > > view to get cleared
> > > >
> > > > Yes, I think we can have tablesync worker send a message to drop stats
> > > > once tablesync is successful. But if we do that also when dropping a
> > > > subscription, I think we need to do that only the transaction is
> > > > committed since we can drop a subscription that doesn't have a
> > > > replication slot and rollback the transaction. Probably we can send
> > > > the message only when the subscritpion does have a replication slot.
> > > >
> > >
> > > Right. And probably for apply worker after updating skip xid.
> >
> > I'm not sure it's better to drop apply worker stats after resetting
> > skip xid (i.g., after skipping the transaction). Since the view is a
> > cumulative view and has last_error_time, I thought we can have the
> > apply worker stats until the subscription gets dropped. Since the
> > error reporting message could get lost, no entry in the view doesn’t
> > mean the worker doesn’t face an issue.
> >
> > >
> > > > In other cases, we can remember the subscriptions being dropped and
> > > > send the message to drop the statistics of them after committing the
> > > > transaction but I’m not sure it’s worth having it.
> > > >
> > >
> > > Yeah, let's not go to that extent. I think in most cases subscriptions
> > > will have corresponding slots.
> >
> > Agreed.
> >
> > >
> > >  FWIW, we completely
> > > > rely on pg_stat_vacuum_stats() for cleaning up the dead tables and
> > > > functions. And we don't expect there are many subscriptions on the
> > > > database.
> > > >
> > >
> > > True, but we do send it for the database, so let's do it for the cases
> > > you explained in the first paragraph.
> >
> > Agreed.
> >
> > I've attached a new version patch. Since the syntax of skipping
> > transaction id is under the discussion I've attached only the error
> > reporting patch for now.
>
> Thanks for the updated patch, few comments:
> 1) This check and return can be moved above CreateTemplateTupleDesc so
> that the tuple descriptor need not be created if there is no worker
> statistics
> +   BlessTupleDesc(tupdesc);
> +
> +   /* Get subscription worker stats */
> +   wentry = pgstat_fetch_subworker(subid, subrelid);
> +
> +   /* Return NULL if there is no worker statistics */
> +   if (wentry == NULL)
> +   PG_RETURN_NULL();
> +
> +   /* Initialise values and NULL flags arrays */
> +   MemSet(values, 0, sizeof(values));
> +   MemSet(nulls, 0, sizeof(nulls));
>
> 2) "NULL for the main apply worker" is mentioned as "null for the main
> apply worker" in case of pg_stat_subscription view, we can mention it
> similarly.
> +  
> +   OID of the relation that the worker is synchronizing; NULL for the
> +   main apply worker
> +  
>
> 3) Variable assignment can be done during declaration and this the
> assignment can be removed
> +   i = 0;
> +   /* subid */
> +   values[i++] = ObjectIdGetDatum(subid);
>
> 4) I noticed that the worker error is still present when queried from
> pg_stat_subscription_workers even after conflict is resolved in the
> subscriber and the worker proceeds with applying the other
> transactions, should this be documented somewhere?
>
> 5) This needs to be aligned, the columns in select have used TAB, we
> should align it using spaces.
> +CREATE VIEW pg_stat_subscription_workers AS
> +SELECT
> +   w.subid,
> +   s.subname,
> +   w.subrelid,
> +   w.relid,
> +   w.command,
> +   w.xid,
> +   w.error_count,
> +   w.error_message,
> +   w.last_error_time,
> +   w.stats_reset
>

Re: Skipping logical replication transactions on subscriber side

[Masahiko Sawada]

On Wed, Nov 3, 2021 at 12:41 PM Amit Kapila  wrote:
>
> On Tue, Nov 2, 2021 at 2:17 PM Masahiko Sawada  wrote:
> >
> > On Tue, Nov 2, 2021 at 2:35 PM Amit Kapila  wrote:
> > >
> > > On Mon, Nov 1, 2021 at 7:18 AM Masahiko Sawada  
> > > wrote:
> > > >
> > > > On Fri, Oct 29, 2021 at 8:20 PM Amit Kapila  
> > > > wrote:
> > > > >
> > > > >
> > > > > Fair enough. So statistics can be removed either by vacuum or drop
> > > > > subscription. Also, if we go by this logic then there is no harm in
> > > > > retaining the stat entries for tablesync errors. Why have different
> > > > > behavior for apply and tablesync workers?
> > > >
> > > > My understanding is that the subscription worker statistics entry
> > > > corresponds to workers (but not physical workers since the physical
> > > > process is changed after restarting). So if the worker finishes its
> > > > jobs, it is no longer necessary to show errors since further problems
> > > > will not occur after that. Table sync worker’s job finishes when
> > > > completing table copy (unless table sync is performed again by REFRESH
> > > > PUBLICATION) whereas apply worker’s job finishes when the subscription
> > > > is dropped.
> > > >
> > >
> > > Actually, I am not very sure how users can use the old error
> > > information after we allowed skipping the conflicting xid. Say, if
> > > they want to add/remove some constraints on the table based on
> > > previous errors then they might want to refer to errors of both the
> > > apply worker and table sync worker.
> >
> > I think that in general, statistics should be retained as long as a
> > corresponding object exists on the database, like other cumulative
> > statistic views. So I’m concerned that an entry of a cumulative stats
> > view is automatically removed by a non-stats-related function (i.g.,
> > ALTER SUBSCRIPTION SKIP). Which seems a new behavior for cumulative
> > stats views.
> >
> > We can retain the stats entries for table sync worker but what I want
> > to avoid is that the view shows many old entries that will never be
> > updated. I've sometimes seen cases where the user mistakenly restored
> > table data on the subscriber before creating a subscription, failed
> > table sync on many tables due to unique violation, and truncated
> > tables on the subscriber. I think that unlike the stats entries for
> > apply worker, retaining the stats entries for table sync could be
> > harmful since it’s likely to be a large amount (even hundreds of
> > entries). Especially, it could lead to bloat the stats file since it
> > has an error message. So if we do that, I'd like to provide a function
> > for users to remove (not reset) stats entries manually.
> >
>
> If we follow the idea of keeping stats at db level (in
> PgStat_StatDBEntry) as discussed above then I think we already have a
> way to remove stat entries via pg_stat_reset which removes the stats
> corresponding to tables, functions and after this patch corresponding
> to subscriptions as well for the current database. Won't that be
> sufficient? I see your point but I think it may be better if we keep
> the same behavior for stats of apply and table sync workers.

Make sense.

>
> Following the tables, functions, I thought of keeping the name of the
> reset function similar to "pg_stat_reset_single_table_counters" but I
> feel the currently used name "pg_stat_reset_subscription_worker" in
> the patch is better. Do let me know what you think?

Yeah, I also tend to prefer pg_stat_reset_subscription_worker name
since "single" isn't clear in the context of subscription worker.  And
the behavior of the reset function for subscription workers is also
different from pg_stat_reset_single_xxx_counters.

I've attached an updated patch. In this version patch, subscription
worker statistics are collected per-database and handled in a similar
way to tables and functions. I think perhaps we still need to discuss
details of how the statistics should be handled but I'd like to share
the patch for discussion.

Regards,

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


v20-0001-Add-a-subscription-worker-statistics-view-pg_sta.patch
Description: Binary data

Re: [PROPOSAL] new diagnostic items for the dynamic sql

[Dinesh Chemuduru]

Hi Pavel,

On Sun, 7 Nov 2021 at 12:53, Pavel Stehule  wrote:

> Hi
>
> pá 5. 11. 2021 v 19:27 odesílatel Dinesh Chemuduru <
> dinesh.ku...@migops.com> napsal:
>
>> Hi Daniel,
>>
>> Thank you for your follow up, and attaching a new patch which addresses
>> Pavel's comments.
>> Let me know If I miss anything here.
>>
>>
>> On Thu, 4 Nov 2021 at 17:40, Daniel Gustafsson  wrote:
>>
>>> > On 9 Sep 2021, at 08:23, Dinesh Chemuduru 
>>> wrote:
>>>
>>> > Let me try to fix the suggested case(I tried to fix this case in the
>>> past, but this time I will try to spend more time on this), and will submit
>>> a new patch.
>>>
>>> This CF entry is marked Waiting on Author, have you had the chance to
>>> prepare a
>>> new version of the patch addressing the comments from Pavel?
>>>
>>
> I think the functionality is correct. I am not sure about implementation
>
>
Thank you for your time in validating this patch.


> 1. Is it necessary to introduce a new field "current_query"? Cannot be
> used "internalquery" instead? Introducing a new field opens some questions
> - what is difference between internalquery and current_query, and where and
> when have to be used first and when second? ErrorData is a fundamental
> generic structure for Postgres, and can be confusing to enhance it by one
> field just for one purpose, that is not used across Postgres.
>
> Internalquery is the one, which was generated by another command.
For example, "DROP  CASCADE"(current_query) will generate many
internal query statements for each of the dependent objects.

At this moment, we do save the current_query in PG_CONTEXT.
As we know, PG_CONTEXT returns the whole statements as stacktrace and it's
hard to fetch the required SQL from it.

I failed to see another way to access the current_query apart from the
PG_CONTEXT.
Hence, we have introduced the new member "current_query" to the "ErrorData"
object.

We tested the internalquery for this purpose, but there are few(10+ unit
test) test cases that failed.
This is also another reason we added this new member to the "ErrorData",
and with the current patch all test cases are successful,
and we are not disturbing the existing functionality.


> 2. The name set_current_err_query is not consistent with names in elog.c -
> probably something like errquery or set_errquery or set_errcurrent_query
> can be more consistent with other names.
>
> Updated as per your suggestion

Please find the new patch version.


> Regards
>
> Pavel
>
>
>
>>> --
>>> Daniel Gustafsson   https://vmware.com/
>>>
>>>
diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
index e5c1356d8c..f402c9f012 100644
--- a/doc/src/sgml/plpgsql.sgml
+++ b/doc/src/sgml/plpgsql.sgml
@@ -2995,6 +2995,16 @@ GET STACKED DIAGNOSTICS variable { = | := } text
  the name of the schema related to exception
 
+
+ PG_SQL_TEXT
+ text
+ invalid sql statement, if any
+
+
+ PG_ERROR_LOCATION
+ text
+ invalid dynamic sql statement's text cursor position, if any
+
 
  PG_EXCEPTION_DETAIL
  text
diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index 0568ae123f..121507b90b 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -2868,6 +2868,7 @@ _SPI_error_callback(void *arg)
 errcontext("PL/pgSQL assignment \"%s\"", query);
 break;
 			default:
+set_errcurrent_query(query);
 errcontext("SQL statement \"%s\"", query);
 break;
 		}
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index f33729513a..a070de54b0 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -621,6 +621,8 @@ errfinish(const char *filename, int lineno, const char *funcname)
 		pfree(edata->constraint_name);
 	if (edata->internalquery)
 		pfree(edata->internalquery);
+	if (edata->current_query)
+		pfree(edata->current_query);
 
 	errordata_stack_depth--;
 
@@ -1599,6 +1601,8 @@ CopyErrorData(void)
 		newedata->constraint_name = pstrdup(newedata->constraint_name);
 	if (newedata->internalquery)
 		newedata->internalquery = pstrdup(newedata->internalquery);
+	if (newedata->current_query)
+		newedata->current_query = pstrdup(newedata->current_query);
 
 	/* Use the calling context for string allocation */
 	newedata->assoc_context = CurrentMemoryContext;
@@ -1639,6 +1643,8 @@ FreeErrorData(ErrorData *edata)
 		pfree(edata->constraint_name);
 	if (edata->internalquery)
 		pfree(edata->internalquery);
+	if (edata->current_query)
+		pfree(edata->current_query);
 	pfree(edata);
 }
 
@@ -1718,6 +1724,8 @@ ThrowErrorData(ErrorData *edata)
 	newedata->internalpos = edata->internalpos;
 	if (edata->internalquery)
 		newedata->internalquery = pstrdup(edata->internalquery);
+	if (edata->current_query)
+		newedata->current_query = pstrdup(edata->current_query);
 
 	MemoryContextSwitchTo(oldcontext);
 	recursion_depth--;
@@ -1784,6 +1792,8 @@ 

Re: Slow standby snapshot

[Andrey Borodin]

Sorry for so late reply. I've been thinking about possible approaches.
KnownAssignedXids over hashtable in fact was implemented long before and 
rejected [0].

> 3 авг. 2021 г., в 22:35, Andres Freund  написал(а):
> 
> On 2021-08-03 10:33:50 +0500, Andrey Borodin wrote:
>>> 3 авг. 2021 г., в 03:01, Andres Freund  написал(а):
>>> On 2021-08-03 00:07:23 +0300, Michail Nikolaev wrote:
 The main idea is simple optimistic optimization - store offset to next
 valid entry. So, in most cases, we could just skip all the gaps.
 Of course, it adds some additional impact for workloads without long
 (few seconds) transactions but it is almost not detectable (because of
 CPU caches).
>>> 
>>> I'm doubtful that that's really the right direction. For workloads that
>>> are replay heavy we already often can see the cost of maintaining the
>>> known xids datastructures show up significantly - not surprising, given
>>> the datastructure. And for standby workloads with active primaries the
>>> cost of searching through the array in all backends is noticeable as
>>> well.  I think this needs a bigger data structure redesign.
>> 
>> KnownAssignedXids implements simple membership test idea. What kind of
>> redesign would you suggest? Proposed optimisation makes it close to optimal,
>> but needs eventual compression.
> 
> Binary searches are very ineffecient on modern CPUs (unpredictable memory
> accesses, unpredictable branches). We constantly need to do binary searches
> during replay to remove xids from the array. I don't see how you can address
> that with just the current datastructure.
Current patch addresses another problem. In presence of old enough transaction 
enumeration of KnownAssignedXids with shared lock prevents adding new 
transactions with exclusive lock. And recovery effectively pauses.

Binary searches can consume 10-15 cache misses, which is unreasonable amount of 
memory waits. But that's somewhat different problem.
Also binsearch is not that expensive when we compress KnownAssignedXids often.

>> Maybe use a hashtable of running transactions? It will be slightly faster
>> when adding\removing single transactions. But much worse when doing
>> KnownAssignedXidsRemove().
> 
> Why would it be worse for KnownAssignedXidsRemove()? Were you intending to
> write KnownAssignedXidsGet[AndSetXmin]()?
I was thinking about inefficient KnownAssignedXidsRemovePreceding() in 
hashtable. But, probably, this is not so frequent operation.

>> Maybe use a tree? (AVL\RB or something like that) It will be slightly 
>> better, because it does not need eventual compression like exiting array.
> 
> I'm not entirely sure what datastructure would work best. I can see something
> like a radix tree work well, or a skiplist style approach. Or a hashtable:
> 
> I'm not sure that we need to care as much about the cost of
> KnownAssignedXidsGetAndSetXmin() - for one, the caching we now have makes that
> less frequent. But more importantly, it'd not be hard to maintain an
> occasionally (or opportunistically) maintained denser version for
> GetSnapshotData() - there's only a single writer, making the concurrency
> issues a lot simpler.

I've been prototyping Radix tree for a while.
Here every 4 xids are summarized my minimum Xid and number of underlying Xids. 
Of cause 4 is arbitrary number, summarization area must be of cacheline size.
┌───┐   
│ 1 / 9 │   
├───┴┐  
│└┐ 
│ └┐
│  └┐   
▼   └───▶   
┌───┐   
│ 1 / 3 | 5 / 0 | 9 / 3 | D / 3 │   
├───┬───┬┬──┴┐  
│   └─┐ └───┐└┐  └─┐
│ └─┐   └──┐  └┐   └─┐  
│   └─┐└──┐└┐└┐ 
▼ └─┐ └──┐  └───┐ └┐
┌───▼┴─▶┴──▶───┴───▶
│ 1 | 2 |   | 4 |   |   |   |   | 9 |   | B | C | D | E | F |  │
└──┘
Bottom layer is current array (TransactionId *KnownAssignedXids).
When we remove Xid we need theoretical minimum of cachelines touched. I'd say 
5-7 instead of 10-15 of binsearch (in case of millions of entries in 
KnownAssignedXids).
Enumerating running Xids is not that difficult too: we will need to scan O(xip) 
memory, not whole KnownAssignedXids array.

But the overall complexity of this approach does not seem good to me.

All in all, I think using proposed "KnownAssignedXidsNext" patch solves real 

Re: Commitfest 2021-11 Patch Triage - Part 1

[Etsuro Fujita]

On Sat, Nov 6, 2021 at 6:16 AM Daniel Gustafsson  wrote:
> 2601: Fast COPY FROM command for the foreign tables
> ===
> This approach taken in this patch has stabilized and the benchmarks posted are
> very promising.  It seems pretty uncontroversial to add COPY to the FDW API 
> SQL
> spec wise.  Amit, Justin and Takayuki-san has done a lot of review and the
> patch is marked Ready for Committer.  Any committer interested in taking this
> on?  Tomas?

I’d like to work on this unless Tomas (or anyone else) want.

Best regards,
Etsuro Fujita

Re: postgres_fdw: commit remote (sub)transactions in parallel during pre-commit

[Etsuro Fujita]

On Tue, Nov 2, 2021 at 7:47 AM David Zhang  wrote:
> Followed your instructions, I performed some basic tests to compare the
> performance between before and after. In my testing environment (two
> foreign servers on the same local machine), the performance varies,
> sometimes the time spent on RELEASE and COMMIT without patch are close
> to after patched, but seems it always perform better after patched. Then
> I ran a 1-millions tuples insert, 5 times average is something like below,
>
> Before
>  RELEASE 0.171 ms, COMMIT 1.861 ms
>
> After
>  RELEASE 0.147 ms, COMMIT 1.305 ms

Good to know!  Thanks for testing!

Best regards,
Etsuro Fujita

Re: postgres_fdw: commit remote (sub)transactions in parallel during pre-commit

[Etsuro Fujita]

On Mon, Nov 1, 2021 at 3:22 PM Fujii Masao  wrote:
> On 2021/10/31 18:05, Etsuro Fujita wrote:
> > The patch is pretty simple: if a server option added
> > by the patch “parallel_commit” is enabled,
>
> Could you tell me why the parameter is necessary?
> Can't we always enable the feature?

I don’t think parallel commit would cause performance degradation,
even in the case when there is just a single remote (sub)transaction
to commit when called from pgfdw_xact_callback
(pgfdw_subxact_callback), so I think it might be OK to enable it by
default.  But my concern about doing so is the remote side: during
those functions, if there are a lot of (sub)transactions on a single
remote server that need to be committed, parallel commit would
increase the remote server’s load at (sub)transaction end than serial
commit, which is the existing implementation, as the requests to
commit those (sub)transactions are sent to the remote server at the
same time; which some users might want to avoid.

> > I think we could extend this to abort cleanup of remote
> > (sub)transactions during post-abort.  Anyway, I think this is useful,
> > so I’ll add this to the upcoming commitfest.
>
> Thanks!

I'll update the patch as such in the next version.

> +   /* Consume whatever data is available from the socket */
> +   if (!PQconsumeInput(conn))
> +   pgfdw_report_error(ERROR, NULL, conn, false, sql);
>
> Without the patch, PQconsumeInput() is not called before pgfdw_get_result()
> But could you tell me why you added PQconsumeInput() there?

The reason is that there might be the result already before calling
pgfdw_get_result(), in which case PQconsumeInput() followed by
PQisBusy() would allow us to call PQgetResult() without doing
WaitLatchOrSocket(), which I think is rather expensive.

> When ignore_errors argument is true, the error reported by
> PQconsumeInput() should be ignored?

I’m not sure about that, because the error might be caused by e.g.,
OOM in the local server, in which case I don’t think it is safe to
ignore it and continue the (sub)transaction-end processing.

Thanks for reviewing!

Best regards,
Etsuro Fujita

Re: [PROPOSAL] new diagnostic items for the dynamic sql

[Pavel Stehule]

Hi

pá 5. 11. 2021 v 19:27 odesílatel Dinesh Chemuduru 
napsal:

> Hi Daniel,
>
> Thank you for your follow up, and attaching a new patch which addresses
> Pavel's comments.
> Let me know If I miss anything here.
>
>
> On Thu, 4 Nov 2021 at 17:40, Daniel Gustafsson  wrote:
>
>> > On 9 Sep 2021, at 08:23, Dinesh Chemuduru 
>> wrote:
>>
>> > Let me try to fix the suggested case(I tried to fix this case in the
>> past, but this time I will try to spend more time on this), and will submit
>> a new patch.
>>
>> This CF entry is marked Waiting on Author, have you had the chance to
>> prepare a
>> new version of the patch addressing the comments from Pavel?
>>
>
I think the functionality is correct. I am not sure about implementation

1. Is it necessary to introduce a new field "current_query"? Cannot be used
"internalquery" instead? Introducing a new field opens some questions -
what is difference between internalquery and current_query, and where and
when have to be used first and when second? ErrorData is a fundamental
generic structure for Postgres, and can be confusing to enhance it by one
field just for one purpose, that is not used across Postgres.

2. The name set_current_err_query is not consistent with names in elog.c -
probably something like errquery or set_errquery or set_errcurrent_query
can be more consistent with other names.

Regards

Pavel



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

Re: to be a multirange or not be, that's the question

[David G. Johnston]

On Saturday, November 6, 2021, Jaime Casanova 
wrote:

> Ok, subject was a bit philosophical but this message I just found is
> quite confusing.
>
> """
> regression=# select cast(null as anyrange) &> cast(null as anymultirange);
> ERROR:  argument declared anymultirange is not a multirange type but type
> anymultirange
> """
>
>
I get that it is hard to parse but it is unambiguously correct.  It does
seem to presume that one understands how polymorphic pseudo-types work (the
supplied query was written without that knowledge applied).  I can envision
some improvements here though it would depend a lot on the actual
implementation.  I’m also curious as to why we get as far the operator
invocation when casting literals to polymorphic pseudo-types is supposedly
an invalid thing to do.

David J.

to be a multirange or not be, that's the question

[Jaime Casanova]

Ok, subject was a bit philosophical but this message I just found is
quite confusing.

"""
regression=# select cast(null as anyrange) &> cast(null as anymultirange);
ERROR:  argument declared anymultirange is not a multirange type but type 
anymultirange
"""

-- 
Jaime Casanova
Director de Servicios Profesionales
SystemGuards - Consultores de PostgreSQL