Re: SQL-standard function body
On 27.04.21 04:44, Justin Pryzby wrote: On Thu, Apr 22, 2021 at 04:04:18PM -0400, Jeff Janes wrote: This commit break line continuation prompts for unbalanced parentheses in the psql binary. Skimming through this thread, I don't see that this is intentional or has been noticed before. with psql -X Before: jjanes=# asdf ( jjanes(# Now: jjanes=# asdf ( jjanes-# I've looked through the parts of the commit that change psql, but didn't see an obvious culprit. I haven't studied it in detail, but it probably needs something like this. Yeah, fixed like that.
Re: Skip temporary table schema name from explain-verbose output.
On Wed, Apr 28, 2021 at 7:56 PM Tom Lane wrote:
>
> Greg Stark writes:
> >> On Tue, Apr 27, 2021 at 7:08 PM Bharath Rupireddy
> >> >> Make sense, we would lose the ability to differentiate temporary
> >> tables from the auto_explain logs.
>
> > There's no useful differentiation that can be done with the temp
> > schema name.
>
I see.
> Agreed.
>
> > I would say it makes sense to remove them -- except perhaps it makes
> > it harder to parse explain output.
>
> I don't think we should remove them. However, it could make sense to
> print the "pg_temp" alias instead of the real schema name when we
> are talking about myTempNamespace. Basically try to make that alias
> a bit less leaky.
+1, let's replace it by "pg_temp" -- did the same in that attached 0001 patch.
Also, I am wondering if we need a similar kind of handling in psql
'\d' meta-command as well? I did trial changes in the 0002 patch, but
I am not very sure about it & a bit skeptical for code change as
well. Do let me know if you have any suggestions/thoughts or if we
don't want to, so please ignore that patch, thanks.
Regards,
Amul
From 4d0556ae5395f3e28fe9616c51ea971f63d6a927 Mon Sep 17 00:00:00 2001
From: Amul Sul
Date: Thu, 29 Apr 2021 02:15:37 -0400
Subject: [PATCH 2/2] WIP-POC-PSQL-change temp table description
---
src/bin/psql/describe.c | 13 +
src/fe_utils/string_utils.c | 8
2 files changed, 21 insertions(+)
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 3e39fdb5452..ea27f09b8a9 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -1584,6 +1584,15 @@ describeTableDetails(const char *pattern, bool verbose, bool showSystem)
nspname = PQgetvalue(res, i, 1);
relname = PQgetvalue(res, i, 2);
+ /* Replace internal temp schema name */
+ if (pset.sversion >= 14)
+ {
+ if (strncmp(nspname, "pg_temp_", 8) == 0)
+nspname = "pg_temp";
+ else if (strncmp(nspname, "pg_toast_temp_", 14) == 0)
+nspname = "pg_toast_temp";
+ }
+
if (!describeOneTableDetails(nspname, relname, oid, verbose))
{
PQclear(res);
@@ -2396,6 +2405,10 @@ describeOneTableDetails(const char *schemaname,
char *schemaname = PQgetvalue(result, 0, 0);
char *relname = PQgetvalue(result, 0, 1);
+ /* Replace internal temporary schema name */
+ if (strncmp(schemaname, "pg_temp_", 8) == 0)
+schemaname = "pg_temp";
+
printfPQExpBuffer(&tmpbuf, _("Owning table: \"%s.%s\""),
schemaname, relname);
printTableAddFooter(&cont, tmpbuf.data);
diff --git a/src/fe_utils/string_utils.c b/src/fe_utils/string_utils.c
index 5b206c7481d..67c7a883b33 100644
--- a/src/fe_utils/string_utils.c
+++ b/src/fe_utils/string_utils.c
@@ -913,6 +913,14 @@ processSQLNamePattern(PGconn *conn, PQExpBuffer buf, const char *pattern,
{
WHEREAND();
appendPQExpBuffer(buf, "%s OPERATOR(pg_catalog.~) ", schemavar);
+
+ /*
+ * Internal temporary schema name could be different, strip out "$"
+ * from pattern to relax the match.
+ */
+ if (strcmp(schemabuf.data, "^(pg_temp)$") == 0 ||
+strcmp(schemabuf.data, "^(pg_toast_temp)$") == 0)
+schemabuf.data[schemabuf.len-1] = '\0';
appendStringLiteralConn(buf, schemabuf.data, conn);
if (PQserverVersion(conn) >= 12)
appendPQExpBufferStr(buf, " COLLATE pg_catalog.default");
--
2.18.0
From 39260208613fe1d8613cd90d6766859fb6104f56 Mon Sep 17 00:00:00 2001
From: Amul Sul
Date: Thu, 29 Apr 2021 01:00:06 -0400
Subject: [PATCH 1/2] Hide internal temp schema name
---
contrib/postgres_fdw/postgres_fdw.c | 4 +++-
src/backend/commands/explain.c | 5 +++--
2 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index e201b5404e6..80cd4334b1e 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -2794,9 +2794,11 @@ postgresExplainForeignScan(ForeignScanState *node, ExplainState *es)
relname = get_rel_name(rte->relid);
if (es->verbose)
{
+ Oid namespaceOid;
char *namespace;
- namespace = get_namespace_name(get_rel_namespace(rte->relid));
+ namespaceOid = get_rel_namespace(rte->relid);
+ namespace = get_namespace_name_or_temp(namespaceOid);
appendStringInfo(relations, "%s.%s",
quote_identifier(namespace),
quote_identifier(relname));
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 3d5198e2345..16f2bf04026 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -3747,7 +3747,8 @@ ExplainTargetRel(Plan *plan, Index rti, ExplainState *es)
Assert(rte->rtekind == RTE_RELATION);
objectname = get_rel_name(rte->relid);
if (es->verbose)
-namespace = get_namespace_name(get_rel_namespace(rte->relid));
+namespace =
+ get_namespace_name_or_temp(get_rel_namespace(rte->relid));
objecttag = "Relation Name";
break;
case T
Re: [PATCH] Identify LWLocks in tracepoints
On 14.04.21 15:20, Peter Eisentraut wrote:
On 12.04.21 07:46, Craig Ringer wrote:
> To use systemtap semaphores (the _ENABLED macros) you need to run
dtrace
> -g to generate a probes.o then link that into postgres.
>
> I don't think we do that. I'll double check soon.
We do that. (It's -G.)
Huh. I could've sworn we didn't. My mistake, it's there in
src/backend/Makefile .
In that case I'll amend the patch to use semaphore guards.
This whole thread is now obviously moved to consideration for PG15, but
I did add an open item about this particular issue
(https://wiki.postgresql.org/wiki/PostgreSQL_14_Open_Items, search for
"dtrace"). So if you could produce a separate patch that adds the
_ENABLED guards targeting PG14 (and PG13), that would be helpful.
Here is a proposed patch for this.
From cae610f0203ba485770e4d274378f3ab198dcc27 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Thu, 29 Apr 2021 09:26:40 +0200
Subject: [PATCH] Prevent lwlock dtrace probes from unnecessary work
If dtrace is compiled in but disabled, the lwlock dtrace probes still
evaluate their arguments. Since PostgreSQL 13, T_NAME(lock) does
nontrivial work, so it should be avoided if not needed. To fix, make
these calls conditional on the *_ENABLED() macro corresponding to each
probe.
Reported-by: Craig Ringer
Discussion:
https://www.postgresql.org/message-id/cagry4nwxkus_rvxfw-ugrzbyxpffm5kjwkt5o+0+stuga5b...@mail.gmail.com
---
src/backend/storage/lmgr/lwlock.c | 36 ---
1 file changed, 24 insertions(+), 12 deletions(-)
diff --git a/src/backend/storage/lmgr/lwlock.c
b/src/backend/storage/lmgr/lwlock.c
index 975d547f34..55b9d7970e 100644
--- a/src/backend/storage/lmgr/lwlock.c
+++ b/src/backend/storage/lmgr/lwlock.c
@@ -1318,7 +1318,8 @@ LWLockAcquire(LWLock *lock, LWLockMode mode)
#endif
LWLockReportWaitStart(lock);
- TRACE_POSTGRESQL_LWLOCK_WAIT_START(T_NAME(lock), mode);
+ if (TRACE_POSTGRESQL_LWLOCK_WAIT_START_ENABLED())
+ TRACE_POSTGRESQL_LWLOCK_WAIT_START(T_NAME(lock), mode);
for (;;)
{
@@ -1340,7 +1341,8 @@ LWLockAcquire(LWLock *lock, LWLockMode mode)
}
#endif
- TRACE_POSTGRESQL_LWLOCK_WAIT_DONE(T_NAME(lock), mode);
+ if (TRACE_POSTGRESQL_LWLOCK_WAIT_DONE_ENABLED())
+ TRACE_POSTGRESQL_LWLOCK_WAIT_DONE(T_NAME(lock), mode);
LWLockReportWaitEnd();
LOG_LWDEBUG("LWLockAcquire", lock, "awakened");
@@ -1349,7 +1351,8 @@ LWLockAcquire(LWLock *lock, LWLockMode mode)
result = false;
}
- TRACE_POSTGRESQL_LWLOCK_ACQUIRE(T_NAME(lock), mode);
+ if (TRACE_POSTGRESQL_LWLOCK_ACQUIRE_ENABLED())
+ TRACE_POSTGRESQL_LWLOCK_ACQUIRE(T_NAME(lock), mode);
/* Add lock to list of locks held by this backend */
held_lwlocks[num_held_lwlocks].lock = lock;
@@ -1400,14 +1403,16 @@ LWLockConditionalAcquire(LWLock *lock, LWLockMode mode)
RESUME_INTERRUPTS();
LOG_LWDEBUG("LWLockConditionalAcquire", lock, "failed");
- TRACE_POSTGRESQL_LWLOCK_CONDACQUIRE_FAIL(T_NAME(lock), mode);
+ if (TRACE_POSTGRESQL_LWLOCK_CONDACQUIRE_FAIL_ENABLED())
+ TRACE_POSTGRESQL_LWLOCK_CONDACQUIRE_FAIL(T_NAME(lock),
mode);
}
else
{
/* Add lock to list of locks held by this backend */
held_lwlocks[num_held_lwlocks].lock = lock;
held_lwlocks[num_held_lwlocks++].mode = mode;
- TRACE_POSTGRESQL_LWLOCK_CONDACQUIRE(T_NAME(lock), mode);
+ if (TRACE_POSTGRESQL_LWLOCK_CONDACQUIRE_ENABLED())
+ TRACE_POSTGRESQL_LWLOCK_CONDACQUIRE(T_NAME(lock), mode);
}
return !mustwait;
}
@@ -1479,7 +1484,8 @@ LWLockAcquireOrWait(LWLock *lock, LWLockMode mode)
#endif
LWLockReportWaitStart(lock);
- TRACE_POSTGRESQL_LWLOCK_WAIT_START(T_NAME(lock), mode);
+ if (TRACE_POSTGRESQL_LWLOCK_WAIT_START_ENABLED())
+
TRACE_POSTGRESQL_LWLOCK_WAIT_START(T_NAME(lock), mode);
for (;;)
{
@@ -1497,7 +1503,8 @@ LWLockAcquireOrWait(LWLock *lock, LWLockMode mode)
Assert(nwaiters < MAX_BACKENDS);
}
#endif
- TRACE_POSTGRESQL_LWLOCK_WAIT_DONE(T_NAME(lock), mode);
+ if (TRACE_POSTGRESQL_LWLOCK_WAIT_DONE_ENABLED())
+ TRACE_POSTGRESQL_LWLOCK_WAIT_DONE(T_NAME(lock),
mode);
LWLockReportWaitEnd();
LOG_LWDEBUG("LWLockAcquireOrWait", lock, "awakened");
@@ -1527,7 +1534,8 @@ LWLockAcquireOrWait(LWLock *lock, LWLockMode mode)
/* Fai
Re: Replication slot stats misgivings
On Thu, Apr 29, 2021 at 11:14 AM Masahiko Sawada wrote: > > On Thu, Apr 29, 2021 at 11:55 AM Amit Kapila wrote: > > > > On Thu, Apr 29, 2021 at 4:58 AM Masahiko Sawada > > wrote: > > > > > > On Thu, Apr 29, 2021 at 5:41 AM Tom Lane wrote: > > > > > > > > It seems that the test case added by f5fc2f5b2 is still a bit > > > > unstable, even after c64dcc7fe: > > > > > > Hmm, I don't see the exact cause yet but there are two possibilities: > > > some transactions were really spilled, > > > > > > > This is the first test and inserts just one small record, so how it > > can lead to spill of data. Do you mean to say that may be some > > background process has written some transaction which leads to a spill > > of data? > > Not sure but I thought that the logical decoding started to decodes > from a relatively old point for some reason and decoded incomplete > transactions that weren’t shown in the result. > > > > > > and it showed the old stats due > > > to losing the drop (and create) slot messages. > > > > > > > Yeah, something like this could happen. Another possibility here could > > be that before the stats collector has processed drop and create > > messages, we have enquired about the stats which lead to it giving us > > the old stats. Note, that we don't wait for 'drop' or 'create' message > > to be delivered. So, there is a possibility of the same. What do you > > think? > > Yeah, that could happen even if any message didn't get dropped. > > > > > > For the former case, it > > > seems to better to create the slot just before the insertion and > > > setting logical_decoding_work_mem to the default (64MB). For the > > > latter case, maybe we can use a different name slot than the name used > > > in other tests? > > > > > > > How about doing both of the above suggestions? Alternatively, we can > > wait for both 'drop' and 'create' message to be delivered but that > > might be overkill. > > Agreed. Attached the patch doing both things. Having a different slot name should solve the problem. The patch looks good to me. Regards, Vignesh
Re: [HACKERS] logical decoding of two-phase transactions
On Mon, Apr 26, 2021 at 9:22 PM vignesh C wrote:
>
> On Wed, Apr 21, 2021 at 12:13 PM Peter Smith wrote:
> >
> > On Tue, Apr 20, 2021 at 3:45 PM Peter Smith wrote:
> > >
> > > Please find attached the latest patch set v73`*
> > >
> > > Differences from v72* are:
> > >
> > > * Rebased to HEAD @ today (required because v72-0001 no longer applied
> > > cleanly)
> > >
> > > * Minor documentation correction for protocol messages for Commit
> > > Prepared ('K')
> > >
> > > * Non-functional code tidy (mostly proto.c) to reduce overloading
> > > different meanings to same member names for prepare/commit times.
> >
> >
> > Please find attached a re-posting of patch set v73*
> >
> > This is the same as yesterday's v73 but with a contrib module compile
> > error fixed.
>
> Thanks for the updated patch, few comments:
Thanks for your feedback comments, My replies are inline below.
> 1) Should "final_lsn not set in begin message" be "prepare_lsn not set
> in begin message"
> +logicalrep_read_begin_prepare(StringInfo in,
> LogicalRepPreparedTxnData *begin_data)
> +{
> + /* read fields */
> + begin_data->prepare_lsn = pq_getmsgint64(in);
> + if (begin_data->prepare_lsn == InvalidXLogRecPtr)
> + elog(ERROR, "final_lsn not set in begin message");
>
OK. Updated in v74.
> 2) Should "These commands" be "ALTER SUBSCRIPTION ... REFRESH
> PUBLICATION and ALTER SUBSCRIPTION ... SET/ADD PUBLICATION ..." as
> copy_data cannot be specified with alter subscription .. drop
> publication.
> + These commands also cannot be executed with copy_data =
> true
> + when the subscription has two_phase commit enabled. See
> + column subtwophasestate of
> +to know the actual
> two-phase state.
OK. Updated in v74. While technically more correct, I think rewording
it as suggested makes the doc harder to understand. But I have
reworded it slightly to account for the fact that the copy_data
setting is not possible with the DROP.
>
> 3) Byte1('A') should be Byte1('r') as we
> have defined LOGICAL_REP_MSG_ROLLBACK_PREPARED as r.
> +Rollback Prepared
> +
> +
> +
> +
> +
> +
> +Byte1('A')
> +
> +Identifies this message as the rollback of a
> two-phase transaction message.
> +
> +
OK. Updated in v74.
>
> 4) Should "Check if the prepared transaction with the given GID and
> lsn is around." be
> "Check if the prepared transaction with the given GID, lsn & timestamp
> is around."
> +/*
> + * LookupGXact
> + * Check if the prepared transaction with the given GID
> and lsn is around.
> + *
> + * Note that we always compare with the LSN where prepare ends because that
> is
> + * what is stored as origin_lsn in the 2PC file.
> + *
> + * This function is primarily used to check if the prepared transaction
> + * received from the upstream (remote node) already exists. Checking only GID
> + * is not sufficient because a different prepared xact with the same GID can
> + * exist on the same node. So, we are ensuring to match origin_lsn and
> + * origin_timestamp of prepared xact to avoid the possibility of a match of
> + * prepared xact from two different nodes.
> + */
OK. Updated in v74.
>
> 5) Should we change "The LSN of the prepare." to "The LSN of the begin
> prepare."
> +Begin Prepare
> +
> +
> +
> +
> +
> +
> +Byte1('b')
> +
> +Identifies this message as the beginning of a
> two-phase transaction message.
> +
> +
> +
> +
> +Int64
> +
> +The LSN of the prepare.
> +
> +
>
Not updated. The PG Docs is correct as-is I think.
>
> 6) Similarly in cases of "Commit Prepared" and "Rollback Prepared"
Not updated. AFAIK these are correct – it really is LSN of the PREPARE
just like it says.
>
> 7) No need to initialize has_subrels as we will always assign the
> value returned by HeapTupleIsValid
> +HasSubscriptionRelations(Oid subid)
> +{
> + Relationrel;
> + int nkeys = 0;
> + ScanKeyData skey[2];
> + SysScanDesc scan;
> + boolhas_subrels = false;
> +
> + rel = table_open(SubscriptionRelRelationId, AccessShareLock);
OK. Updated in v74.
>
> 8) We could include errhint, like errhint("Option \"two_phase\"
> specified more than once") to specify a more informative error
> message.
> + else if (strcmp(defel->defname, "two_phase") == 0)
> + {
> + if (two_phase_option_given)
> + ereport(ERROR,
> +
> (errcode(ERRCODE_SYNTAX_ERROR),
> +errmsg("conflicting
> or redundant options")));
> + two_phase_option_given = true;
> +
> + data->two_phase = defGetBoolean(defel);
> + }
>
Not updated. Yes, maybe it would be better like you say, but the code
would then be inconsistent with every other option in this function.
Perhaps your idea can be raised as a separate
Re: [HACKERS] logical decoding of two-phase transactions
On Tue, Apr 27, 2021 at 1:41 PM vignesh C wrote:
>
> On Wed, Apr 21, 2021 at 12:13 PM Peter Smith wrote:
> >
> > On Tue, Apr 20, 2021 at 3:45 PM Peter Smith wrote:
> > >
> > > Please find attached the latest patch set v73`*
> > >
> > > Differences from v72* are:
> > >
> > > * Rebased to HEAD @ today (required because v72-0001 no longer applied
> > > cleanly)
> > >
> > > * Minor documentation correction for protocol messages for Commit
> > > Prepared ('K')
> > >
> > > * Non-functional code tidy (mostly proto.c) to reduce overloading
> > > different meanings to same member names for prepare/commit times.
> >
> >
> > Please find attached a re-posting of patch set v73*
>
> Few comments when I was having a look at the tests added:
Thanks for your feedback comments. My replies are inline below.
> 1) Can the below:
> +# check inserts are visible. 22 should be rolled back. 21 should be
> committed.
> +$result = $node_subscriber->safe_psql('postgres', "SELECT count(*)
> FROM tab_full where a IN (21);");
> +is($result, qq(1), 'Rows committed are on the subscriber');
> +$result = $node_subscriber->safe_psql('postgres', "SELECT count(*)
> FROM tab_full where a IN (22);");
> +is($result, qq(0), 'Rows rolled back are not on the subscriber');
>
> be changed to:
> $result = $node_subscriber->safe_psql('postgres', "SELECT a FROM
> tab_full where a IN (21,22);");
> is($result, qq(21), 'Rows committed are on the subscriber');
>
> And Test count need to be reduced to "use Test::More tests => 19"
>
OK. Updated in v74.
> 2) we can change tx to transaction:
> +# check the tx state is prepared on subscriber(s)
> +$result = $node_B->safe_psql('postgres', "SELECT count(*) FROM
> pg_prepared_xacts;");
> +is($result, qq(1), 'transaction is prepared on subscriber B');
> +$result = $node_C->safe_psql('postgres', "SELECT count(*) FROM
> pg_prepared_xacts;");
> +is($result, qq(1), 'transaction is prepared on subscriber C');
>
OK. Updated in v74
> 3) There are few more instances present in the same file, those also
> can be changed.
OK. I found no others in the same file, but there were similar cases
in the 021 TAP test. Those were also updated in v74/
>
> 4) Can the below:
> check inserts are visible at subscriber(s).
> # 22 should be rolled back.
> # 21 should be committed.
> $result = $node_B->safe_psql('postgres', "SELECT count(*) FROM
> tab_full where a IN (21);");
> is($result, qq(1), 'Rows committed are present on subscriber B');
> $result = $node_B->safe_psql('postgres', "SELECT count(*) FROM
> tab_full where a IN (22);");
> is($result, qq(0), 'Rows rolled back are not present on subscriber B');
> $result = $node_C->safe_psql('postgres', "SELECT count(*) FROM
> tab_full where a IN (21);");
> is($result, qq(1), 'Rows committed are present on subscriber C');
> $result = $node_C->safe_psql('postgres', "SELECT count(*) FROM
> tab_full where a IN (22);");
> is($result, qq(0), 'Rows rolled back are not present on subscriber C');
>
> be changed to:
> $result = $node_B->safe_psql('postgres', "SELECT a FROM tab_full where
> a IN (21,22);");
> is($result, qq(21), 'Rows committed are on the subscriber');
> $result = $node_C->safe_psql('postgres', "SELECT a FROM tab_full where
> a IN (21,22);");
> is($result, qq(21), 'Rows committed are on the subscriber');
>
> And Test count need to be reduced to "use Test::More tests => 27"
>
OK. Updated in v74.
> 5) should we change "Two phase commit" to "Two phase commit state" :
> + /*
> +* Binary, streaming, and two_phase are only supported
> in v14 and
> +* higher
> +*/
> if (pset.sversion >= 14)
> appendPQExpBuffer(&buf,
> ", subbinary
> AS \"%s\"\n"
> - ", substream
> AS \"%s\"\n",
> + ", substream
> AS \"%s\"\n"
> + ",
> subtwophasestate AS \"%s\"\n",
>
> gettext_noop("Binary"),
> -
> gettext_noop("Streaming"));
> +
> gettext_noop("Streaming"),
> +
> gettext_noop("Two phase commit"));
>
Not updated. I think the column name is already the longest one and
this just makes it even longer - far too long IMO. I am not sure what
is better having the “state” suffix. After all, booleans are also
states. Anyway, I did not make this change now but if people feel
strongly about it then I can revisit it.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
Re: [HACKERS] logical decoding of two-phase transactions
On Tue, Apr 27, 2021 at 6:17 PM vignesh C wrote:
>
> On Wed, Apr 21, 2021 at 12:13 PM Peter Smith wrote:
> >
> > On Tue, Apr 20, 2021 at 3:45 PM Peter Smith wrote:
> > >
> > > Please find attached the latest patch set v73`*
> > >
> > > Differences from v72* are:
> > >
> > > * Rebased to HEAD @ today (required because v72-0001 no longer applied
> > > cleanly)
> > >
> > > * Minor documentation correction for protocol messages for Commit
> > > Prepared ('K')
> > >
> > > * Non-functional code tidy (mostly proto.c) to reduce overloading
> > > different meanings to same member names for prepare/commit times.
> >
> >
> > Please find attached a re-posting of patch set v73*
> >
> > This is the same as yesterday's v73 but with a contrib module compile
> > error fixed.
>
> Few comments on
> v73-0002-Add-prepare-API-support-for-streaming-transactio.patch patch:
Thanks for your feedback comments. My replies are inline below.
> 1) There are slight differences in error message in case of Alter
> subscription ... drop publication, we can keep the error message
> similar:
> postgres=# ALTER SUBSCRIPTION mysub drop PUBLICATION mypub WITH
> (refresh = false, copy_data=true, two_phase=true);
> ERROR: unrecognized subscription parameter: "copy_data"
> postgres=# ALTER SUBSCRIPTION mysub drop PUBLICATION mypub WITH
> (refresh = false, two_phase=true, streaming=true);
> ERROR: cannot alter two_phase option
OK. Updated in v74.
>
> 2) We are sending txn->xid twice, I felt we should send only once in
> logicalrep_write_stream_prepare:
> + /* transaction ID */
> + Assert(TransactionIdIsValid(txn->xid));
> + pq_sendint32(out, txn->xid);
> +
> + /* send the flags field */
> + pq_sendbyte(out, flags);
> +
> + /* send fields */
> + pq_sendint64(out, prepare_lsn);
> + pq_sendint64(out, txn->end_lsn);
> + pq_sendint64(out, txn->u_op_time.prepare_time);
> + pq_sendint32(out, txn->xid);
> +
>
OK. Updated in v74.
> 3) We could remove xid and return prepare_data->xid
> +TransactionId
> +logicalrep_read_stream_prepare(StringInfo in,
> LogicalRepPreparedTxnData *prepare_data)
> +{
> + TransactionId xid;
> + uint8 flags;
> +
> + xid = pq_getmsgint(in, 4);
>
OK. Updated in v74.
> 4) Here comments can be above apply_spooled_messages for better readability
> + /*
> +* 1. Replay all the spooled operations - Similar code as for
> +* apply_handle_stream_commit (i.e. non two-phase stream commit)
> +*/
> +
> + ensure_transaction();
> +
> + nchanges = apply_spooled_messages(xid, prepare_data.prepare_lsn);
> +
>
Not done. It was deliberately commented this way because the part
below the comment is what is in apply_handle_stream_commit.
> 5) Similarly this below comment can be above PrepareTransactionBlock
> + /*
> +* 2. Mark the transaction as prepared. - Similar code as for
> +* apply_handle_prepare (i.e. two-phase non-streamed prepare)
> +*/
> +
> + /*
> +* BeginTransactionBlock is necessary to balance the
> EndTransactionBlock
> +* called within the PrepareTransactionBlock below.
> +*/
> + BeginTransactionBlock();
> + CommitTransactionCommand();
> +
> + /*
> +* Update origin state so we can restart streaming from correct
> position
> +* in case of crash.
> +*/
> + replorigin_session_origin_lsn = prepare_data.end_lsn;
> + replorigin_session_origin_timestamp = prepare_data.prepare_time;
> +
> + PrepareTransactionBlock(gid);
> + CommitTransactionCommand();
> +
> + pgstat_report_stat(false);
>
Not done. It is deliberately commented this way because the part below
the comment is what is in apply_handle_prepare.
> 6) There is a lot of common code between apply_handle_stream_prepare
> and apply_handle_prepare, if possible try to have a common function to
> avoid fixing at both places.
> + /*
> +* 2. Mark the transaction as prepared. - Similar code as for
> +* apply_handle_prepare (i.e. two-phase non-streamed prepare)
> +*/
> +
> + /*
> +* BeginTransactionBlock is necessary to balance the
> EndTransactionBlock
> +* called within the PrepareTransactionBlock below.
> +*/
> + BeginTransactionBlock();
> + CommitTransactionCommand();
> +
> + /*
> +* Update origin state so we can restart streaming from correct
> position
> +* in case of crash.
> +*/
> + replorigin_session_origin_lsn = prepare_data.end_lsn;
> + replorigin_session_origin_timestamp = prepare_data.prepare_time;
> +
> + PrepareTransactionBlock(gid);
> + CommitTransactionCommand();
> +
> + pgstat_report_stat(false);
> +
> + store_flush_position(prepare_data.end_lsn);
>
Not done. If you diff those functions there are really only ~ 10
statements in common so I felt it is more readable to kee
Re: Replication slot stats misgivings
On Wed, Apr 28, 2021 at 5:01 PM Amit Kapila wrote:
>
> On Wed, Apr 28, 2021 at 4:51 PM Masahiko Sawada wrote:
> >
> > On Wed, Apr 28, 2021 at 6:39 PM Amit Kapila wrote:
>
> @@ -1369,7 +1369,7 @@ ReorderBufferIterTXNNext(ReorderBuffer *rb,
> ReorderBufferIterTXNState *state)
> * Update the total bytes processed before releasing the current set
> * of changes and restoring the new set of changes.
> */
> - rb->totalBytes += rb->size;
> + rb->totalBytes += entry->txn->total_size;
> if (ReorderBufferRestoreChanges(rb, entry->txn, &entry->file,
> &state->entries[off].segno))
>
> I have not tested this but won't in the above change you need to check
> txn->toptxn for subtxns?
>
Now, I am able to reproduce this issue:
Create table t1(c1 int);
select pg_create_logical_replication_slot('s', 'test_decoding');
Begin;
insert into t1 values(1);
savepoint s1;
insert into t1 select generate_series(1, 10);
commit;
postgres=# select count(*) from pg_logical_slot_peek_changes('s1', NULL, NULL);
count
15
(1 row)
postgres=# select * from pg_stat_replication_slots;
slot_name | spill_txns | spill_count | spill_bytes | stream_txns |
stream_count | stream_bytes | total_txns | total_bytes |
stats_reset
---++-+-+-+--+--++-+--
s1| 0 | 0 | 0 | 0 |
0 |0 | 2 |13200672 | 2021-04-29
14:33:55.156566+05:30
(1 row)
select * from pg_stat_reset_replication_slot('s1');
Now reduce the logical decoding work mem to allow spilling.
postgres=# set logical_decoding_work_mem='64kB';
SET
postgres=# select count(*) from pg_logical_slot_peek_changes('s1', NULL, NULL);
count
15
(1 row)
postgres=# select * from pg_stat_replication_slots;
slot_name | spill_txns | spill_count | spill_bytes | stream_txns |
stream_count | stream_bytes | total_txns | total_bytes |
stats_reset
---++-+-+-+--+--++-+--
s1| 1 | 202 |1320 | 0 |
0 |0 | 2 | 672 | 2021-04-29
14:35:21.836613+05:30
(1 row)
You can notice that after we have allowed spilling the 'total_bytes'
stats is showing a different value. The attached patch fixes the issue
for me. Let me know what do you think about this?
--
With Regards,
Amit Kapila.
use_total_size_v3.patch
Description: Binary data
Re: pg_hba.conf.sample wording improvement
On Thu, Apr 29, 2021 at 7:08 AM Peter Eisentraut wrote: > > On 28.04.21 16:09, Alvaro Herrera wrote: > > Looking at it now, I wonder how well do the "hostno" options work. If I > > say "hostnogssenc", is an SSL-encrypted socket good? If I say > > "hostnossl", is a GSS-encrypted socket good? If so, how does that make > > sense? > > I think for example if you want to enforce SSL connections, then writing > "hostnossl ... reject" would be sensible. That would also reject > GSS-encrypted connections, but that would be what you want in that scenario. I'd say the interface has become a lot less well-matching now that we have two separate settings for it. For example right now it's more complex to say "reject anything not encrypted", which I bet is what a lot of people would want. They don't particularly care if it's gss encrypted or ssl encrypted. Perhaps what we want to do (obviously not for 14) is to allow you to specify more than one entry in the first column, so you could say "hostssl,hostgssenc" on the same row? That would give some strange results with the "no" mappings, but it might work if used right? -- Magnus Hagander Me: https://www.hagander.net/ Work: https://www.redpill-linpro.com/
Re: [PATCH] We install pg_regress and isolationtester but not pg_isolation_regress
On Thu, Apr 29, 2021 at 4:41 AM Tom Lane wrote: > > Michael Paquier writes: > > On Wed, Apr 28, 2021 at 12:44:45PM +0300, Aleksander Alekseev wrote: > >> I just wanted to let you know that TimescaleDB uses > >> pg_isolation_regress and occasionally there are reports from some > >> suffering/puzzled users/developers, e.g. [1]. Not 100% sure if it > >> makes investing the time into backpatching worth it. However if > >> someone could do it, it would be nice. > > > FWIW, I am not really sure that this is important enough to justify a > > back-patch, even it is true that there have been cases in the past > > where extra binaries got added in minor releases. > > Yeah, I think adding a binary in a minor release is a Big Deal to > packagers. I doubt that the case here justifies that. +1. Given the number of complaints from people lacking it since the binary was first created, I can't see how that's a priority that justifies that. -- Magnus Hagander Me: https://www.hagander.net/ Work: https://www.redpill-linpro.com/
Re: [BUG] "FailedAssertion" reported when streaming in logical replication
On Thu, Apr 29, 2021 at 12:09 PM tanghy.f...@fujitsu.com wrote: > > > On Thursday, April 29, 2021 3:18 PM, Dilip Kumar wrote > > >I tried to think about how to write a test case for this scenario, but > >I think it will not be possible to generate an automated test case for this. > >Basically, we need 2 concurrent transactions and out of that, > >we need one transaction which just has processed only one change i.e > >XLOG_HEAP2_NEW_CID and another transaction should overflow the logical > >decoding work mem, so that we select the wrong transaction which > >doesn't have the base snapshot. But how to control that the > >transaction which is performing the DDL just write the > >XLOG_HEAP2_NEW_CID wal and before it writes any other WAL we should > >get the WAl from other transaction which overflows the buffer. > > Thanks for your updating. > Actually, I tried to make the automated test for the problem, too. But made > no process on this. > Agreed on your opinion " not be possible to generate an automated test case > for this ". Thanks for trying this out. > If anyone figure out a good solution for the test automation of this case. > Please be kind to share that with us. Thanks. +1 -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
function for testing that causes the backend to terminate
For testing unusual situations I'd like to be able to cause a backend to terminate due to something like a segfault. Do we currently have this in testing ? Dave Cramer
Re: function for testing that causes the backend to terminate
On Thu, Apr 29, 2021 at 4:27 PM Dave Cramer wrote: > For testing unusual situations I'd like to be able to cause a backend to > terminate due to something like a segfault. Do we currently have this in > testing ? Well, you could use pg_terminate_backend which sends SIGTERM to the backend. However, we don't have a function that sends SIGSEGV yet, you could signal the backend with SIGSEGV directly, if possible. With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com
Re: pg_dump new feature: exporting functions only. Bad or good idea ?
The following review has been posted through the commitfest application: make installcheck-world: tested, failed Implements feature: tested, passed Spec compliant: tested, passed Documentation:not tested Few minor comments : - The latest patch has some hunk failures - Regression with master has many failures with/without the patch, it is difficult to tell if the patch is causing any failures. - This is probably intended behaviour that --functions-only switch is also dumping stored procedures? - If i create a procedure with LANGUAGE plpgsql SECURITY INVOKER It is not including "SECURITY INVOKER" in the dump. That's probably because INVOKER is default security rights.
Re: [BUG] "FailedAssertion" reported when streaming in logical replication
On Tue, Apr 27, 2021 at 5:18 PM Dilip Kumar wrote: > > I have modified the patch based on the above comments. > The patch looks good to me. I have slightly modified the comments and commit message. See, what you think of the attached? I think we can leave the test for this as there doesn't seem to be an easy way to automate it. -- With Regards, Amit Kapila. v4-0001-Fix-the-bugs-in-selecting-the-transaction-for-str.patch Description: Binary data
Re: Replication slot stats misgivings
On Thu, Apr 29, 2021 at 3:06 PM Amit Kapila wrote:
>
> On Wed, Apr 28, 2021 at 5:01 PM Amit Kapila
wrote:
> >
> > On Wed, Apr 28, 2021 at 4:51 PM Masahiko Sawada
wrote:
> > >
> > > On Wed, Apr 28, 2021 at 6:39 PM Amit Kapila
wrote:
> >
> > @@ -1369,7 +1369,7 @@ ReorderBufferIterTXNNext(ReorderBuffer *rb,
> > ReorderBufferIterTXNState *state)
> > * Update the total bytes processed before releasing the current set
> > * of changes and restoring the new set of changes.
> > */
> > - rb->totalBytes += rb->size;
> > + rb->totalBytes += entry->txn->total_size;
> > if (ReorderBufferRestoreChanges(rb, entry->txn, &entry->file,
> > &state->entries[off].segno))
> >
> > I have not tested this but won't in the above change you need to check
> > txn->toptxn for subtxns?
> >
>
> Now, I am able to reproduce this issue:
> Create table t1(c1 int);
> select pg_create_logical_replication_slot('s', 'test_decoding');
> Begin;
> insert into t1 values(1);
> savepoint s1;
> insert into t1 select generate_series(1, 10);
> commit;
>
> postgres=# select count(*) from pg_logical_slot_peek_changes('s1', NULL,
NULL);
> count
>
> 15
> (1 row)
>
> postgres=# select * from pg_stat_replication_slots;
> slot_name | spill_txns | spill_count | spill_bytes | stream_txns |
> stream_count | stream_bytes | total_txns | total_bytes |
> stats_reset
>
---++-+-+-+--+--++-+--
> s1| 0 | 0 | 0 | 0 |
> 0 |0 | 2 |13200672 | 2021-04-29
> 14:33:55.156566+05:30
> (1 row)
>
> select * from pg_stat_reset_replication_slot('s1');
>
> Now reduce the logical decoding work mem to allow spilling.
> postgres=# set logical_decoding_work_mem='64kB';
> SET
> postgres=# select count(*) from pg_logical_slot_peek_changes('s1', NULL,
NULL);
> count
>
> 15
> (1 row)
>
> postgres=# select * from pg_stat_replication_slots;
> slot_name | spill_txns | spill_count | spill_bytes | stream_txns |
> stream_count | stream_bytes | total_txns | total_bytes |
> stats_reset
>
---++-+-+-+--+--++-+--
> s1| 1 | 202 |1320 | 0 |
> 0 |0 | 2 | 672 | 2021-04-29
> 14:35:21.836613+05:30
> (1 row)
>
> You can notice that after we have allowed spilling the 'total_bytes'
> stats is showing a different value. The attached patch fixes the issue
> for me. Let me know what do you think about this?
I found one issue with the following scenario when testing with
logical_decoding_work_mem as 64kB:
BEGIN;
INSERT INTO t1 values(generate_series(1,1));
SAVEPOINT s1;
INSERT INTO t1 values(generate_series(1,1));
COMMIT;
SELECT count(*) FROM pg_logical_slot_get_changes('regression_slot1', NULL,
NULL, 'include-xids', '0', 'skip-empty-xacts', '1');
select * from pg_stat_replication_slots;
slot_name | spill_txns | spill_count | spill_bytes | stream_txns |
stream_count | stream_bytes | total_txns | total_bytes |
stats_reset
--++-+-+-+--+--++-+--
regression_slot1 | 6 | 154 | 9130176 | 0 |
0 |0 | 1 | *4262016* | 2021-04-29
17:50:00.080663+05:30
(1 row)
Same thing works fine with logical_decoding_work_mem as 64MB:
select * from pg_stat_replication_slots;
slot_name | spill_txns | spill_count | spill_bytes | stream_txns |
stream_count | stream_bytes | total_txns | total_bytes |
stats_reset
--++-+-+-+--+--++-+--
regression_slot1 | 6 | 154 | 9130176 | 0 |
0 |0 | 1 | *264* | 2021-04-29
17:50:00.080663+05:30
(1 row)
The patch required one change:
- rb->totalBytes += rb->size;
+ if (entry->txn->toptxn)
+ rb->totalBytes += entry->txn->toptxn->total_size;
+ else
+ rb->totalBytes += entry->txn->*total_size*;
The above should be changed to:
- rb->totalBytes += rb->size;
+ if (entry->txn->toptxn)
+ rb->totalBytes += entry->txn->toptxn->total_size;
+ else
+ rb->totalBytes += entry->txn->*size*;
Attached patch fixes the issue.
Thoughts?
Regards,
Vignesh
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index c27f710053..cdf46a36af 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -1369,7 +1369,10 @@ ReorderBufferIterTXNNext(ReorderBuffer *rb, ReorderBufferIterTXNState *state)
Remove post-increment in function quote_identifier of pg_upgrade
Hi,
The function quote_identifier has extra post-increment operation as
highlighted below,
char *
quote_identifier(const char *s)
{
char *result = pg_malloc(strlen(s) * 2 + 3);
char *r = result;
*r++ = '"';
while (*s)
{
if (*s == '"')
*r++ = *s;
*r++ = *s;
s++;
}
*r++ = '"';
**r++ = '\0';*
return result;
}
I think *r = '\0' is enough here. Per precedence table the precedence of
postfix increment operator is higher. The above statement increments 'r'
pointer address but returns the original un-incremented pointer address,
which is then dereferenced. Correct me if I am wrong here.
If my understanding is correct then '++' is not needed in the
above highlighted statement which is leading to overhead.
Find an attached patch which does the same. This can be backported till v96.
Thanks & Regards,
Vaibhav Dalvi
[image: image.png]
diff --git a/src/bin/pg_upgrade/util.c b/src/bin/pg_upgrade/util.c
index fc20472..dc000d0 100644
--- a/src/bin/pg_upgrade/util.c
+++ b/src/bin/pg_upgrade/util.c
@@ -198,7 +198,7 @@ quote_identifier(const char *s)
s++;
}
*r++ = '"';
- *r++ = '\0';
+ *r = '\0';
return result;
}
Re: Unresolved repliaction hang and stop problem.
Cc'ing Lukasz Biegaj because of the pgsql-general thread. On 2021-Apr-29, Amit Kapila wrote: > On Wed, Apr 28, 2021 at 7:36 PM Alvaro Herrera > wrote: > > ... It's strange that replication worked for them on pg10 though and > > broke on 13. What did we change anything to make it so? > > No idea but probably if the other person can share the exact test case > which he sees working fine on PG10 but not on PG13 then it might be a > bit easier to investigate. Ah, noticed now that Krzysztof posted links to these older threads, where a problem is described: https://www.postgresql.org/message-id/flat/CANDwggKYveEtXjXjqHA6RL3AKSHMsQyfRY6bK%2BNqhAWJyw8psQ%40mail.gmail.com https://www.postgresql.org/message-id/flat/8bf8785c-f47d-245c-b6af-80dc1eed40db%40unitygroup.com Krzysztof said "after upgrading to pg13 we started having problems", which implicitly indicates that the same thing worked well in pg10 --- but if the problem has been correctly identified, then this wouldn't have worked in pg10 either. So something in the story doesn't quite match up. Maybe it's not the same problem after all, or maybe they weren't doing X in pg10 which they are attempting in pg13. Krzysztof, Lukasz, maybe you can describe more? -- Álvaro Herrera Valdivia, Chile
Re: Remove post-increment in function quote_identifier of pg_upgrade
On Thu, Apr 29, 2021 at 06:35:28PM +0530, Vaibhav Dalvi wrote:
> Hi,
>
> The function quote_identifier has extra post-increment operation as
> highlighted below,
>
> char *
> quote_identifier(const char *s)
> {
>char *result = pg_malloc(strlen(s) * 2 + 3);
>char *r = result;
>
>*r++ = '"';
>while (*s)
>{
> if (*s == '"')
> *r++ = *s;
> *r++ = *s;
> s++;
>}
>*r++ = '"';
>**r++ = '\0';*
>
>return result;
> }
>
> I think *r = '\0' is enough here. Per precedence table the precedence of
> postfix increment operator is higher. The above statement increments 'r'
> pointer address but returns the original un-incremented pointer address,
> which is then dereferenced. Correct me if I am wrong here.
>
> If my understanding is correct then '++' is not needed in the
> above highlighted statement which is leading to overhead.
I don't think the integer increment during pg_upgrade is a meaningful overhead.
You could check the compiler's assembly output it may be the same even without
the ++.
I'd suggest to leave it as it's currently written, since the idiom on every
other line is *r++ = ..., it'd be strange to write it differently here, and
could end up being confusing or copied+pasted somewhere else.
> Find an attached patch which does the same. This can be backported till v96.
In any case, think it would not be backpatched, since it's essentially
cosmetic.
> diff --git a/src/bin/pg_upgrade/util.c b/src/bin/pg_upgrade/util.c
> index fc20472..dc000d0 100644
> --- a/src/bin/pg_upgrade/util.c
> +++ b/src/bin/pg_upgrade/util.c
> @@ -198,7 +198,7 @@ quote_identifier(const char *s)
> s++;
> }
> *r++ = '"';
> - *r++ = '\0';
> + *r = '\0';
>
> return result;
> }
Re: Remove post-increment in function quote_identifier of pg_upgrade
Justin Pryzby writes: > On Thu, Apr 29, 2021 at 06:35:28PM +0530, Vaibhav Dalvi wrote: >> If my understanding is correct then '++' is not needed in the >> above highlighted statement which is leading to overhead. > I don't think the integer increment during pg_upgrade is a meaningful > overhead. > You could check the compiler's assembly output it may be the same even without > the ++. Yeah: if the increment actually costs something, I'd expect the compiler to optimize it away. But on a lot of machine architectures, a pointer post-increment is basically free anyhow. > I'd suggest to leave it as it's currently written, since the idiom on every > other line is *r++ = ..., it'd be strange to write it differently here, and > could end up being confusing or copied+pasted somewhere else. I agree --- cosmetically, this change isn't an improvement. (On the other hand, if it were written the other way already, I'd also argue to leave it like that. Basically, this sort of change is just not worth troubling over. It doesn't improve things meaningfully and it creates back-patching hazards.) regards, tom lane
Re: Addition of authenticated ID to pg_stat_activity
On Tue, Apr 27, 2021 at 8:25 PM Andres Freund wrote: > > Hi, > > On 2021-04-27 12:40:29 -0400, Stephen Frost wrote: > > So, what fields are people really looking at when querying > > pg_stat_activity interactively? User, database, pid, last query, > > transaction start, query start, state, wait event info, maybe backend > > xmin/xid? I doubt most people looking at pg_stat_activity interactively > > actually care about the non-user backends (autovacuum, et al). > > Not representative, but I personally am about as often interested in one > of the non-connection processes as the connection > ones. E.g. investigating what is autovacuum's bottleneck, are > checkpointer / wal writer / bgwriter io bound or keeping up, etc. I definitely use it all the time to monitor autovacuum all the time. The others as well regularly, but autovacuum continuously. I also see a lot of people doing things like "from pg_stat_activity where query like '%mytablename%'" where they'd want both any regular queries and any autovacuums currently processing the table. I'd say client address is also pretty common to identify which set of app servers connections are coming in from -- but client port and client hostname are a lot less interesting. But it'd be kind of weird to split those out. For *interactive use* I'd find pretty much all other columns interesting and commonly used. Probably not that interested in the oids of the database and user, but again they are the cheap ones. We could get rid of the joints if we only showed the oids, but in interactive use it's really the names that are interesting. But if we're just trying to save column count, I'd say get rid of datid and usesysid. I'd hold everything else as interesting. -- Magnus Hagander Me: https://www.hagander.net/ Work: https://www.redpill-linpro.com/
Re: tool to migrate database
concepts similar and related to migration/replication are applied with the software tcapture , please give a look at https://www.tcapture.net/ Il giorno gio 29 apr 2021 alle ore 16:34 Bruce Momjian ha scritto: > On Tue, Mar 23, 2021 at 09:49:57AM +0100, Joel Jacobson wrote: > > I recently read an interesting real-life story from a very big company, > Adyen, > > and how they upgraded their 50 terrabyte PostgreSQL database. The > article is > > from 2018 but I still think it's relevant: > > > > https://medium.com/adyen/ > > updating-a-50-terabyte-postgresql-database-f64384b799e7 > > > > There might be other good tools I don't know of, I'm not an expert on > upgrades. > > Hopefully other pghackers can fill in. > > This is not an appropriate topic for the hackers email list, which is > for internal server development discussion. The 'general' or 'admin' > lists would be better. > > -- > Bruce Momjian https://momjian.us > EDB https://enterprisedb.com > > If only the physical world exists, free will is an illusion. > > > > > >
Re: pg_hba.conf.sample wording improvement
Greetings, * Magnus Hagander (mag...@hagander.net) wrote: > On Thu, Apr 29, 2021 at 7:08 AM Peter Eisentraut > wrote: > > On 28.04.21 16:09, Alvaro Herrera wrote: > > > Looking at it now, I wonder how well do the "hostno" options work. If I > > > say "hostnogssenc", is an SSL-encrypted socket good? If I say > > > "hostnossl", is a GSS-encrypted socket good? If so, how does that make > > > sense? > > > > I think for example if you want to enforce SSL connections, then writing > > "hostnossl ... reject" would be sensible. That would also reject > > GSS-encrypted connections, but that would be what you want in that scenario. > > I'd say the interface has become a lot less well-matching now that we > have two separate settings for it. For example right now it's more > complex to say "reject anything not encrypted", which I bet is what a > lot of people would want. They don't particularly care if it's gss > encrypted or ssl encrypted. I'm not really sure that I agree it's such an issue, particularly since you have to come up with a way to specify the auth method to use somehow too as we haven't got any fallback mechanism or anything like that. While you might use cert-based auth or SCRAM for TLS connections, it isn't the case that you can use SCRAM with a GSS encrypted connection. > Perhaps what we want to do (obviously not for 14) is to allow you to > specify more than one entry in the first column, so you could say > "hostssl,hostgssenc" on the same row? That would give some strange > results with the "no" mappings, but it might work if used right? In general, I'm not against the idea of giving more options but I'm just not sure that it's a real use-case when you consider that the auth method also has to be specified. I also don't recall anyone showing up asking about how they could specify "encrypted but I don't care how". Thanks, Stephen signature.asc Description: PGP signature
Re: Remove post-increment in function quote_identifier of pg_upgrade
On 2021-Apr-29, Tom Lane wrote: > (On the other hand, if it were written the other way already, I'd also > argue to leave it like that. Basically, this sort of change is just not > worth troubling over. It doesn't improve things meaningfully and it > creates back-patching hazards.) This argument applies equally well to the patch at http://postgr.es/m/CAAJ_b94M_1YoybQpNjmD+ZFZkUT2OpoP5xnFiWM+X=xh-nx...@mail.gmail.com so if we reject this one, we should reject that one too. CC'ed patch author. -- Álvaro Herrera Valdivia, Chile
Re: Remove redundant variable from transformCreateStmt
I'd do it like this. Note I removed an if/else block in addition to
your changes.
I couldn't convince myself that this is worth pushing though; either we
push it to all branches (which seems unwarranted) or we create
back-patching hazards.
--
Álvaro Herrera39°49'30"S 73°17'W
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index 9dd30370da..2f20d81470 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -176,7 +176,6 @@ transformCreateStmt(CreateStmt *stmt, const char *queryString)
Oid namespaceid;
Oid existing_relid;
ParseCallbackState pcbstate;
- bool is_foreign_table = IsA(stmt, CreateForeignTableStmt);
/*
* We must not scribble on the passed-in CreateStmt, so copy it. (This is
@@ -227,16 +226,8 @@ transformCreateStmt(CreateStmt *stmt, const char *queryString)
/* Set up CreateStmtContext */
cxt.pstate = pstate;
- if (IsA(stmt, CreateForeignTableStmt))
- {
- cxt.stmtType = "CREATE FOREIGN TABLE";
- cxt.isforeign = true;
- }
- else
- {
- cxt.stmtType = "CREATE TABLE";
- cxt.isforeign = false;
- }
+ cxt.isforeign = IsA(stmt, CreateForeignTableStmt);
+ cxt.stmtType = cxt.isforeign ? "CREATE FOREIGN TABLE" : "CREATE TABLE";
cxt.relation = stmt->relation;
cxt.rel = NULL;
cxt.inhRelations = stmt->inhRelations;
@@ -333,8 +324,11 @@ transformCreateStmt(CreateStmt *stmt, const char *queryString)
/*
* Postprocess check constraints.
+ *
+ * For regular tables all constraints can be marked valid immediately,
+ * because the table must be empty. Not so for foreign tables.
*/
- transformCheckConstraints(&cxt, !is_foreign_table ? true : false);
+ transformCheckConstraints(&cxt, !cxt.isforeign);
/*
* Postprocess extended statistics.
Re: default_tablespace doc and partitioned rels
On 2021-Apr-16, Justin Pryzby wrote: > If this parameter is set when a partitioned table is created, the partitioned > table's tablespace will be set to the given tablespace, and which will be the > default tablespace for partitions create in the future, even if > default_tablespace itself has since been changed. Pushed with very similar wording: + +If this parameter is set to a value other than the empty string +when a partitioned table is created, the partitioned table's +tablespace will be set to that value, which will be used as +the default tablespace for partitions created in the future, +even if default_tablespace has changed since then. + I made it a separate paragraph at the end, because I noticed that I had added the note in an inappropriate place in the earlier commit; the second paragraph in particular is more general than this one. Also looking at that one I realized that we need to talk about the value being "not the empty string". I hope it's clear enough now, but if you or anybody have further suggestion on improving this, I'm listening. Thanks -- Álvaro Herrera Valdivia, Chile "If you have nothing to say, maybe you need just the right tool to help you not say it." (New York Times, about Microsoft PowerPoint)
Re: Remove redundant variable from transformCreateStmt
Alvaro Herrera writes: > I'd do it like this. Note I removed an if/else block in addition to > your changes. > I couldn't convince myself that this is worth pushing though; either we > push it to all branches (which seems unwarranted) or we create > back-patching hazards. Yeah ... an advantage of the if/else coding is that it'd likely be simple to extend to cover additional statement types, should we ever wish to do that. The rendering you have here is nice and compact, but it would not scale up well. regards, tom lane
Included xid in restoring reorder buffer changes from disk log message
Hi, While debugging one of the logical decoding issues, I found that xid was not included in restoring reorder buffer changes from disk log messages. Attached a patch for it. I felt including the XID will be helpful in debugging. Thoughts? Regards, Vignesh From 9d3ee45b7b2c0d625af888579035a0fb9a1e512c Mon Sep 17 00:00:00 2001 From: vignesh Date: Thu, 29 Apr 2021 21:38:09 +0530 Subject: [PATCH] Included xid in restoring reorder buffer changes from disk. Included xid in restoring reorder buffer changes from disk. --- src/backend/replication/logical/reorderbuffer.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c index cdf46a36af..b00f7f4801 100644 --- a/src/backend/replication/logical/reorderbuffer.c +++ b/src/backend/replication/logical/reorderbuffer.c @@ -1381,9 +1381,10 @@ ReorderBufferIterTXNNext(ReorderBuffer *rb, ReorderBufferIterTXNState *state) dlist_head_element(ReorderBufferChange, node, &entry->txn->changes); - elog(DEBUG2, "restored %u/%u changes from disk", + elog(DEBUG2, "restored %u/%u changes of XID %u from disk", (uint32) entry->txn->nentries_mem, - (uint32) entry->txn->nentries); + (uint32) entry->txn->nentries, + (uint32) entry->txn->xid); Assert(entry->txn->nentries_mem); /* txn stays the same */ -- 2.25.1
Re: Enhanced error message to include hint messages for redundant options error
On Mon, Apr 26, 2021 at 9:24 PM Alvaro Herrera wrote:
>
> On 2021-Apr-26, Bharath Rupireddy wrote:
>
> > I agree that we can just be clear about the problem. Looks like the
> > majority of the errors "conflicting or redundant options" are for
> > redundant options. So, wherever "conflicting or redundant options"
> > exists: 1) change the message to "option \"%s\" specified more than
> > once" and remove parser_errposition if it's there because the option
> > name in the error message would give the info with which user can
> > point to the location
>
> Hmm, I would keep the parser_errposition() even if the option name is
> mentioned in the error message. There's no harm in being a little
> redundant, with both the option name and the error cursor showing the
> same thing.
>
> > 2) change the message to something like "option \"%s\" is conflicting
> > with option \"%s\"".
>
> Maybe, but since these would all be special cases, I think we'd need to
> discuss them individually. I would suggest that in order not to stall
> this patch, these cases should all stay as "redundant or conflicting
> options" -- that is, avoid any further change apart from exactly the
> thing you came here to change. You can submit a 0002 patch to change
> those other errors. That way, even if those changes end up rejected for
> whatever reason, you still got your 0001 done (which would change the
> bulk of "conflicting or redundant" error to the "option %s already
> specified" error). Some progress is better than none.
Thanks for the comments, please find the attached v3 patch which has
the change for the first part. I will make changes for 002 and post it
soon.
Thoughts?
Regards,
Vignesh
From 6ed153cdb45a0d41d3889dc7d80b3a743eb66725 Mon Sep 17 00:00:00 2001
From: vignesh
Date: Mon, 26 Apr 2021 18:40:36 +0530
Subject: [PATCH v3] Enhance error message.
Enhanced error message, so that the user can easily identify the error.
---
contrib/file_fdw/file_fdw.c | 6 +--
src/backend/catalog/aclchk.c| 4 +-
src/backend/commands/copy.c | 23 +-
src/backend/commands/dbcommands.c | 28 ++--
src/backend/commands/extension.c| 8 ++--
src/backend/commands/foreigncmds.c | 4 +-
src/backend/commands/functioncmds.c | 12 +++---
src/backend/commands/publicationcmds.c | 4 +-
src/backend/commands/sequence.c | 18
src/backend/commands/subscriptioncmds.c | 18
src/backend/commands/tablecmds.c| 2 +-
src/backend/commands/typecmds.c | 14 +++---
src/backend/commands/user.c | 48 ++---
src/backend/parser/parse_utilcmd.c | 2 +-
src/backend/replication/pgoutput/pgoutput.c | 10 ++---
src/backend/replication/walsender.c | 6 +--
src/test/regress/expected/copy2.out | 24 ++-
src/test/regress/expected/foreign_data.out | 4 +-
src/test/regress/expected/publication.out | 2 +-
19 files changed, 119 insertions(+), 118 deletions(-)
diff --git a/contrib/file_fdw/file_fdw.c b/contrib/file_fdw/file_fdw.c
index 2c2f149fb0..10aa2fca28 100644
--- a/contrib/file_fdw/file_fdw.c
+++ b/contrib/file_fdw/file_fdw.c
@@ -292,8 +292,7 @@ file_fdw_validator(PG_FUNCTION_ARGS)
if (force_not_null)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("conflicting or redundant options"),
- errhint("Option \"force_not_null\" supplied more than once for a column.")));
+ errmsg("option \"%s\" specified more than once", def->defname)));
force_not_null = def;
/* Don't care what the value is, as long as it's a legal boolean */
(void) defGetBoolean(def);
@@ -304,8 +303,7 @@ file_fdw_validator(PG_FUNCTION_ARGS)
if (force_null)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("conflicting or redundant options"),
- errhint("Option \"force_null\" supplied more than once for a column.")));
+ errmsg("option \"%s\" specified more than once", def->defname)));
force_null = def;
(void) defGetBoolean(def);
}
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index e1573eb398..7885587bfc 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -923,7 +923,7 @@ ExecAlterDefaultPrivilegesStmt(ParseState *pstate, AlterDefaultPrivilegesStmt *s
if (dnspnames)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("conflicting or redundant options"),
+ errmsg("option \"%s\" specified more than once", defel->defname),
parser_errposition(pstate, defel->location)));
dnspnames = defel;
}
@@ -932,7 +932,7 @@ ExecAlterDefaultPrivilegesStmt(ParseState *pstate, AlterDefaultPrivilegesStmt *s
if (drolespecs)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("conflicting or redundant options"),
+ errmsg("option \"%s\" specified more than once", defel->d
Re: Enhanced error message to include hint messages for redundant options error
On 2021-Apr-29, vignesh C wrote: > Thanks for the comments, please find the attached v3 patch which has > the change for the first part. Looks good to me. I would only add parser_errposition() to the few error sites missing that. -- Álvaro Herrera39°49'30"S 73°17'W "Uno puede defenderse de los ataques; contra los elogios se esta indefenso"
Re: Race condition in InvalidateObsoleteReplicationSlots()
On 2021-Apr-07, Andres Freund wrote:
> I'm also confused by the use of ConditionVariableTimedSleep(timeout =
> 10). Why do we need a timed sleep here in the first place? And why with
> such a short sleep?
I was scared of the possibility that a process would not set the CV for
whatever reason, causing checkpointing to become stuck. Maybe that's
misguided thinking if CVs are reliable enough.
> I also noticed that the code is careful to use CHECK_FOR_INTERRUPTS(); -
> but is aware it's running in checkpointer. I don't think CFI does much
> there? If we are worried about needing to check for interrupts, more
> work is needed.
Hmm .. yeah, doing CFI seems pretty useless. I think that should just
be removed. If checkpointer gets USR2 (request for shutdown) it's not
going to affect the behavior of CFI anyway.
I attach a couple of changes to your 0001. It's all cosmetic; what
looks not so cosmetic is the change of "continue" to "break" in helper
routine; if !s->in_use, we'd loop infinitely. The other routine
already checks that before calling the helper; since you hold
ReplicationSlotControlLock at that point, it should not be possible to
drop it in between. Anyway, it's a trivial change to make, so it should
be correct.
I also added a "continue" at the bottom of one block; currently that
doesn't change any behavior, but if we add code at the other block, it
might not be what's intended.
> After this I don't see a reason to have SAB_Inquire - as far as I can
> tell it's practically impossible to use without race conditions? Except
> for raising an error - which is "builtin"...
Hmm, interesting ... If not needed, yeah let's get rid of that.
Are you getting this set pushed, or would you like me to handle it?
(There seems to be some minor conflict in 13)
--
Álvaro Herrera Valdivia, Chile
"Pido que me den el Nobel por razones humanitarias" (Nicanor Parra)
>From 7f31b0ec12e52b6c967047384353895538161840 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera
Date: Thu, 29 Apr 2021 13:19:54 -0400
Subject: [PATCH] Alvaro's edits
---
src/backend/replication/slot.c | 18 +-
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index d28330cbd8..cd6f75b3e9 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -1172,19 +1172,17 @@ InvalidateObsoleteReplicationSlot(ReplicationSlot *s, XLogRecPtr oldestLSN)
while (true)
{
- XLogRecPtr restart_lsn = InvalidXLogRecPtr;
+ XLogRecPtr restart_lsn;
bool slot_conflicts;
NameData slotname;
int active_pid = 0;
Assert(LWLockHeldByMeInMode(ReplicationSlotControlLock, LW_SHARED));
- CHECK_FOR_INTERRUPTS();
-
slot_conflicts = false;
if (!s->in_use)
- continue;
+ break;
/*
* Check if the slot needs to be invalidated. If it needs to be
@@ -1205,12 +1203,16 @@ InvalidateObsoleteReplicationSlot(ReplicationSlot *s, XLogRecPtr oldestLSN)
slotname = s->data.name;
active_pid = s->active_pid;
- /* check if we can acquire it */
+ /*
+ * If the slot can be acquired, do so and mark it invalidated
+ * immediately. Otherwise we'll signal the owning process, below,
+ * and retry.
+ */
if (active_pid == 0)
{
MyReplicationSlot = s;
s->active_pid = MyProcPid;
-s->data.invalidated_at = s->data.restart_lsn;
+s->data.invalidated_at = restart_lsn;
s->data.restart_lsn = InvalidXLogRecPtr;
}
}
@@ -1262,6 +1264,7 @@ InvalidateObsoleteReplicationSlot(ReplicationSlot *s, XLogRecPtr oldestLSN)
/* re-acquire for next loop iteration */
LWLockAcquire(ReplicationSlotControlLock, LW_SHARED);
+ continue;
}
else
{
@@ -1286,7 +1289,6 @@ InvalidateObsoleteReplicationSlot(ReplicationSlot *s, XLogRecPtr oldestLSN)
break;
}
-
}
Assert(!released_lock == LWLockHeldByMeInMode(ReplicationSlotControlLock, LW_SHARED));
@@ -1313,8 +1315,6 @@ restart:
{
ReplicationSlot *s = &ReplicationSlotCtl->replication_slots[i];
- CHECK_FOR_INTERRUPTS();
-
if (!s->in_use)
continue;
--
2.20.1
Re: Patch to allow pg_filedump to support reading of pg_filenode.map
Thanks for the feedback, Justin. I've gone ahead and switched to use
memcmp. I also refactored it to:
1. Don't assume that any file with first 4 bytes matching the
relmapper magic number is a pg_relnode.map file
2. Don't assume the pg_relnode.map file is uncorrupted and intact; perform
a check of the first 4 bytes against the reference magic number
3. Provide a flag (-m) for users to have their file interpreted as a
pg_relnode.map file
I hope this is more palatable to everyone :)
--Richard
On Wed, Apr 28, 2021 at 9:42 PM Justin Pryzby wrote:
> This is separate from the postgresql server repo.
> https://git.postgresql.org/gitweb/?p=pg_filedump.git
>
> +#define RELMAPPER_FILEMAGIC 0x592717
> +char magic_buffer[8];
>
> ...
>
> + if ( (int) magic_buffer & RELMAPPER_FILEMAGIC ) {
>
> This is doing bitwise arithmetic on a pointer, which seems badly wrong.
> I think it breaks normal use of pg_filedump - unless you happen to get a
> magic_buffer without those bits set. The segfault seems to confirm that,
> as
> does gcc:
>
> pg_filedump.c:2041:8: warning: cast from pointer to integer of different
> size [-Wpointer-to-int-cast]
> 2041 | if ( (int) magic_buffer & RELMAPPER_FILEMAGIC ) {
>
> I think it probably means to do memcmp, instead ??
>
> --
> Justin
>
add_filenode_support_v2.patch
Description: Binary data
Re: Remove redundant variable from transformCreateStmt
On 2021-Apr-29, Tom Lane wrote: > Alvaro Herrera writes: > > I'd do it like this. Note I removed an if/else block in addition to > > your changes. > > > I couldn't convince myself that this is worth pushing though; either we > > push it to all branches (which seems unwarranted) or we create > > back-patching hazards. > > Yeah ... an advantage of the if/else coding is that it'd likely be > simple to extend to cover additional statement types, should we ever > wish to do that. The rendering you have here is nice and compact, > but it would not scale up well. That makes sense. But that part is not in Amul's patch -- he was only on about removing the is_foreign_table Boolean. If I remove the if/else block change, does the rest of the patch looks something we'd want to have? I kinda agree that the redundant variable is "ugly". Is it worth removing? My hunch is no. -- Álvaro Herrera39°49'30"S 73°17'W
Re: Patch to allow pg_filedump to support reading of pg_filenode.map
I think you should be able to avoid crashing if passed a non-relmapper file. Make sure not to loop over more mappings than exist in the relmapper file of the given size. I guess you should warn if the number of mappings is too large for the file's size. And then "cap" the number of mappings to the maximum possible number. -- Justin
Re: Patch to allow pg_filedump to support reading of pg_filenode.map
On Thu, Apr 29, 2021 at 12:05 PM Justin Pryzby wrote: > I think you should be able to avoid crashing if passed a non-relmapper > file. > Make sure not to loop over more mappings than exist in the relmapper file > of > the given size. > > I guess you should warn if the number of mappings is too large for the > file's > size. And then "cap" the number of mappings to the maximum possible > number. > Ah, thanks for the tip. That's right -- I can't assume the user's input is a valid file. Updated patch here. --Richard > > -- > Justin > add_filenode_support_v3.patch Description: Binary data
Re: function for testing that causes the backend to terminate
On 4/29/21 6:56 AM, Dave Cramer wrote: For testing unusual situations I'd like to be able to cause a backend to terminate due to something like a segfault. Do we currently have this in testing ? If you can run SQL as a superuser from that backend, try: COPY (SELECT pg_backend_pid()) TO PROGRAM 'xargs kill -SIGSEGV'; HTH, Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
Re: Replication slot stats misgivings
On Thu, Apr 29, 2021 at 9:44 PM vignesh C wrote:
>
>
>
> On Thu, Apr 29, 2021 at 3:06 PM Amit Kapila wrote:
> >
> > On Wed, Apr 28, 2021 at 5:01 PM Amit Kapila wrote:
> > >
> > > On Wed, Apr 28, 2021 at 4:51 PM Masahiko Sawada
> > > wrote:
> > > >
> > > > On Wed, Apr 28, 2021 at 6:39 PM Amit Kapila
> > > > wrote:
> > >
> > > @@ -1369,7 +1369,7 @@ ReorderBufferIterTXNNext(ReorderBuffer *rb,
> > > ReorderBufferIterTXNState *state)
> > > * Update the total bytes processed before releasing the current set
> > > * of changes and restoring the new set of changes.
> > > */
> > > - rb->totalBytes += rb->size;
> > > + rb->totalBytes += entry->txn->total_size;
> > > if (ReorderBufferRestoreChanges(rb, entry->txn, &entry->file,
> > > &state->entries[off].segno))
> > >
> > > I have not tested this but won't in the above change you need to check
> > > txn->toptxn for subtxns?
> > >
> >
> > Now, I am able to reproduce this issue:
> > Create table t1(c1 int);
> > select pg_create_logical_replication_slot('s', 'test_decoding');
> > Begin;
> > insert into t1 values(1);
> > savepoint s1;
> > insert into t1 select generate_series(1, 10);
> > commit;
> >
> > postgres=# select count(*) from pg_logical_slot_peek_changes('s1', NULL,
> > NULL);
> > count
> >
> > 15
> > (1 row)
> >
> > postgres=# select * from pg_stat_replication_slots;
> > slot_name | spill_txns | spill_count | spill_bytes | stream_txns |
> > stream_count | stream_bytes | total_txns | total_bytes |
> > stats_reset
> > ---++-+-+-+--+--++-+--
> > s1| 0 | 0 | 0 | 0 |
> > 0 |0 | 2 |13200672 | 2021-04-29
> > 14:33:55.156566+05:30
> > (1 row)
> >
> > select * from pg_stat_reset_replication_slot('s1');
> >
> > Now reduce the logical decoding work mem to allow spilling.
> > postgres=# set logical_decoding_work_mem='64kB';
> > SET
> > postgres=# select count(*) from pg_logical_slot_peek_changes('s1', NULL,
> > NULL);
> > count
> >
> > 15
> > (1 row)
> >
> > postgres=# select * from pg_stat_replication_slots;
> > slot_name | spill_txns | spill_count | spill_bytes | stream_txns |
> > stream_count | stream_bytes | total_txns | total_bytes |
> > stats_reset
> > ---++-+-+-+--+--++-+--
> > s1| 1 | 202 |1320 | 0 |
> > 0 |0 | 2 | 672 | 2021-04-29
> > 14:35:21.836613+05:30
> > (1 row)
> >
> > You can notice that after we have allowed spilling the 'total_bytes'
> > stats is showing a different value. The attached patch fixes the issue
> > for me. Let me know what do you think about this?
>
> I found one issue with the following scenario when testing with
> logical_decoding_work_mem as 64kB:
>
> BEGIN;
> INSERT INTO t1 values(generate_series(1,1));
> SAVEPOINT s1;
> INSERT INTO t1 values(generate_series(1,1));
> COMMIT;
> SELECT count(*) FROM pg_logical_slot_get_changes('regression_slot1', NULL,
> NULL, 'include-xids', '0', 'skip-empty-xacts', '1');
> select * from pg_stat_replication_slots;
> slot_name | spill_txns | spill_count | spill_bytes | stream_txns |
> stream_count | stream_bytes | total_txns | total_bytes | stats_reset
> --++-+-+-+--+--++-+--
> regression_slot1 | 6 | 154 | 9130176 | 0 |
> 0 |0 | 1 | 4262016 | 2021-04-29
> 17:50:00.080663+05:30
> (1 row)
>
> Same thing works fine with logical_decoding_work_mem as 64MB:
> select * from pg_stat_replication_slots;
>slot_name | spill_txns | spill_count | spill_bytes | stream_txns |
> stream_count | stream_bytes | total_txns | total_bytes | stats_reset
> --++-+-+-+--+--++-+--
> regression_slot1 | 6 | 154 | 9130176 | 0 |
> 0 |0 | 1 | 264 | 2021-04-29
> 17:50:00.080663+05:30
> (1 row)
>
> The patch required one change:
> - rb->totalBytes += rb->size;
> + if (entry->txn->toptxn)
> + rb->totalBytes += entry->txn->toptxn->total_size;
> + else
> + rb->totalBytes += entry->txn->total_size;
>
> The above should be changed to:
> - rb->totalBytes += rb->size;
> + if (entry->txn->toptxn)
> + rb->totalBytes += entry->txn->toptxn->total_size;
> + else
> + rb->totalBytes += entry->txn->size;
>
> Attached patch fixes the issue.
> Thoughts?
After more thought, it seems to me that we sho
Re: Remove redundant variable from transformCreateStmt
On Thu, Apr 29, 2021 at 02:39:42PM -0400, Alvaro Herrera wrote: > On 2021-Apr-29, Tom Lane wrote: > > Alvaro Herrera writes: > > > I'd do it like this. Note I removed an if/else block in addition to > > > your changes. > > > > > I couldn't convince myself that this is worth pushing though; either we > > > push it to all branches (which seems unwarranted) or we create > > > back-patching hazards. > > > > Yeah ... an advantage of the if/else coding is that it'd likely be > > simple to extend to cover additional statement types, should we ever > > wish to do that. The rendering you have here is nice and compact, > > but it would not scale up well. > > That makes sense. But that part is not in Amul's patch -- he was only > on about removing the is_foreign_table Boolean. If I remove the if/else > block change, does the rest of the patch looks something we'd want to > have? I kinda agree that the redundant variable is "ugly". Is it worth > removing? My hunch is no. Getting rid of a redundant, boolean variable is good not because it's more efficient but because it's one fewer LOC to read and maintain (and an opportunity for inconsistency, I suppose). Also, this is a roundabout and too-verbose way to invert a boolean: | transformCheckConstraints(&cxt, !is_foreign_table ? true : false); -- Justin PS. It's also not pythonic ;)
Re: Add client connection check during the execution of the query
On Sat, Apr 3, 2021 at 9:27 AM Thomas Munro wrote:
> Pushed! Thanks to all who contributed.
Here's something I wanted to park here to look into for the next
cycle: it turns out that kqueue's EV_EOF flag also has the right
semantics for this. That leads to the idea of exposing the event via
the WaitEventSet API, and would the bring
client_connection_check_interval feature to 6/10 of our OSes, up from
2/10. Maybe Windows' FD_CLOSE event could get us up to 7/10, not
sure.
From 957826a4c6bfec2d88c2ea2f004e55ebf12ea473 Mon Sep 17 00:00:00 2001
From: Thomas Munro
Date: Fri, 30 Apr 2021 10:38:40 +1200
Subject: [PATCH 1/2] Add WL_SOCKET_CLOSED for socket shutdown events.
Provide a way for WaitEventSet to report that the remote peer has shut
down its socket, independently of whether there is any buffered data
remaining to be read. This works only on systems where the kernel
exposes that information, namely:
* WAIT_USE_POLL builds, on systems that have the POLLRDHUP extension
* WAIT_USE_EPOLL builds, using EPOLLRDHUP
* WAIT_USE_KQUEUE builds, using EV_EOF
---
src/backend/storage/ipc/latch.c | 64 -
src/include/storage/latch.h | 5 +--
2 files changed, 58 insertions(+), 11 deletions(-)
diff --git a/src/backend/storage/ipc/latch.c b/src/backend/storage/ipc/latch.c
index ad781131e2..6c77356019 100644
--- a/src/backend/storage/ipc/latch.c
+++ b/src/backend/storage/ipc/latch.c
@@ -841,6 +841,7 @@ FreeWaitEventSet(WaitEventSet *set)
* - WL_SOCKET_CONNECTED: Wait for socket connection to be established,
* can be combined with other WL_SOCKET_* events (on non-Windows
* platforms, this is the same as WL_SOCKET_WRITEABLE)
+ * - WL_SOCKET_CLOSED: Wait for socket to be closed by remote peer.
* - WL_EXIT_ON_PM_DEATH: Exit immediately if the postmaster dies
*
* Returns the offset in WaitEventSet->events (starting from 0), which can be
@@ -1043,12 +1044,16 @@ WaitEventAdjustEpoll(WaitEventSet *set, WaitEvent *event, int action)
else
{
Assert(event->fd != PGINVALID_SOCKET);
- Assert(event->events & (WL_SOCKET_READABLE | WL_SOCKET_WRITEABLE));
+ Assert(event->events & (WL_SOCKET_READABLE |
+WL_SOCKET_WRITEABLE |
+WL_SOCKET_CLOSED));
if (event->events & WL_SOCKET_READABLE)
epoll_ev.events |= EPOLLIN;
if (event->events & WL_SOCKET_WRITEABLE)
epoll_ev.events |= EPOLLOUT;
+ if (event->events & WL_SOCKET_CLOSED)
+ epoll_ev.events |= EPOLLRDHUP;
}
/*
@@ -1087,12 +1092,18 @@ WaitEventAdjustPoll(WaitEventSet *set, WaitEvent *event)
}
else
{
- Assert(event->events & (WL_SOCKET_READABLE | WL_SOCKET_WRITEABLE));
+ Assert(event->events & (WL_SOCKET_READABLE |
+WL_SOCKET_WRITEABLE |
+WL_SOCKET_CLOSED));
pollfd->events = 0;
if (event->events & WL_SOCKET_READABLE)
pollfd->events |= POLLIN;
if (event->events & WL_SOCKET_WRITEABLE)
pollfd->events |= POLLOUT;
+#ifdef POLLRDHUP
+ if (event->events & WL_SOCKET_CLOSED)
+ pollfd->events |= POLLRDHUP;
+#endif
}
Assert(event->fd != PGINVALID_SOCKET);
@@ -1165,7 +1176,9 @@ WaitEventAdjustKqueue(WaitEventSet *set, WaitEvent *event, int old_events)
Assert(event->events != WL_LATCH_SET || set->latch != NULL);
Assert(event->events == WL_LATCH_SET ||
event->events == WL_POSTMASTER_DEATH ||
- (event->events & (WL_SOCKET_READABLE | WL_SOCKET_WRITEABLE)));
+ (event->events & (WL_SOCKET_READABLE |
+ WL_SOCKET_WRITEABLE |
+ WL_SOCKET_CLOSED)));
if (event->events == WL_POSTMASTER_DEATH)
{
@@ -1188,9 +1201,9 @@ WaitEventAdjustKqueue(WaitEventSet *set, WaitEvent *event, int old_events)
* old event mask to the new event mask, since kevent treats readable
* and writable as separate events.
*/
- if (old_events & WL_SOCKET_READABLE)
+ if (old_events & (WL_SOCKET_READABLE | WL_SOCKET_CLOSED))
old_filt_read = true;
- if (event->events & WL_SOCKET_READABLE)
+ if (event->events & (WL_SOCKET_READABLE | WL_SOCKET_CLOSED))
new_filt_read = true;
if (old_events & WL_SOCKET_WRITEABLE)
old_filt_write = true;
@@ -1210,7 +1223,10 @@ WaitEventAdjustKqueue(WaitEventSet *set, WaitEvent *event, int old_events)
event);
}
- Assert(count > 0);
+ /* For WL_SOCKET_READ -> WL_SOCKET_CLOSED, no change needed. */
+ if (count == 0)
+ return;
+
Assert(count <= 2);
rc = kevent(set->kqueue_fd, &k_ev[0], count, NULL, 0, NULL);
@@ -1525,7 +1541,9 @@ WaitEventSetWaitBlock(WaitEventSet *set, int cur_timeout,
returned_events++;
}
}
- else if (cur_event->events & (WL_SOCKET_READABLE | WL_SOCKET_WRITEABLE))
+ else if (cur_event->events & (WL_SOCKET_READABLE |
+ WL_SOCKET_WRITEABLE |
+ WL_SOCKET_CLOSED))
{
Assert(cur_event->fd != PGINVALID_SOCKET);
@@ -1543,6 +1561,13 @@ WaitEventSetWaitBlock(WaitEventSet *set, int cur_timeout,
occurred_events->events |= WL_SOCKET_WRITEABLE;
}
+ if ((cur_event->events & WL_SOCKET_CLOSED) &&
+(cur_epoll_event->events &
Re: Enhanced error message to include hint messages for redundant options error
On Thu, Apr 29, 2021 at 10:44 PM Alvaro Herrera wrote: > > On 2021-Apr-29, vignesh C wrote: > > > Thanks for the comments, please find the attached v3 patch which has > > the change for the first part. > > Looks good to me. I would only add parser_errposition() to the few > error sites missing that. Yes, we need to add parser_errposition as agreed in [1]. I think we will have to make changes in compute_common_attribute as well because the error in the duplicate_error goto statement is actually for the duplicate option specified more than once, we can do something like the attached. If it seems okay, it can be merged with the main patch. [1] - https://www.postgresql.org/message-id/flat/CALj2ACUa%3DZM8QtOLPCHc7%3DWgFrx9P6-AgKQs8cmKLvNCvu7arQ%40mail.gmail.com With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com v1-0001-compute_common_attribute.patch Description: Binary data
Re: Result Cache node shows per-worker info even for workers not launched
On Wed, 28 Apr 2021 at 23:05, Amit Khandekar wrote: > > On Wed, 28 Apr 2021 at 13:54, David Rowley wrote: > > I've attached a patch to do this. The explain.c part is pretty similar > > to your patch, I just took my original code and comment. > > Sounds good. And thanks for the cleanup patch, and the brief history. > Patch looks ok to me. Thanks for the review. I pushed the patch with a small additional change to further tidy up show_resultcache_info(). David
Re: Asymmetric partition-wise JOIN
On 11/30/20 7:43 PM, Anastasia Lubennikova wrote: This entry was inactive during this CF, so I've marked it as returned with feedback. Feel free to resubmit an updated version to a future commitfest. I return the patch to commitfest. My current reason differs from reason of origin author. This patch can open a door for more complex optimizations in the partitionwise join push-down technique. I mean, we can push-down join not only of two partitioned tables with the same partition schema, but a partitioned (sharded) table with an arbitrary subplan that is provable independent of local resources. Example: CREATE TABLE p(a int) PARTITION BY HASH (a); CREATE TABLE p1 PARTITION OF p FOR VALUES WITH (MODULUS 3, REMAINDER 0); CREATE TABLE p2 PARTITION OF p FOR VALUES WITH (MODULUS 3, REMAINDER 1); CREATE TABLE p3 PARTITION OF p FOR VALUES WITH (MODULUS 3, REMAINDER 2); SELECT * FROM p, (SELECT * FROM generate_series(1,2) AS a) AS s WHERE p.a=s.a; Hash Join Hash Cond: (p.a = a.a) -> Append -> Seq Scan on p1 p_1 -> Seq Scan on p2 p_2 -> Seq Scan on p3 p_3 -> Hash -> Function Scan on generate_series a But with asymmetric join feature we have the plan: Append -> Hash Join Hash Cond: (p_1.a = a.a) -> Seq Scan on p1 p_1 -> Hash -> Function Scan on generate_series a -> Hash Join Hash Cond: (p_2.a = a.a) -> Seq Scan on p2 p_2 -> Hash -> Function Scan on generate_series a -> Hash Join Hash Cond: (p_3.a = a.a) -> Seq Scan on p3 p_3 -> Hash -> Function Scan on generate_series a In the case of FDW-sharding it means that if we can prove that the inner relation is independent from the execution server, we can push-down these joins and execute it in parallel. -- regards, Andrey Lepikhov Postgres Professional
Re: Replication slot stats misgivings
On Thu, Apr 29, 2021 at 12:07 PM Amit Kapila wrote: > > On Thu, Apr 29, 2021 at 11:14 AM Masahiko Sawada > wrote: > > > > > > > > How about doing both of the above suggestions? Alternatively, we can > > > wait for both 'drop' and 'create' message to be delivered but that > > > might be overkill. > > > > Agreed. Attached the patch doing both things. > > > > Thanks, the patch LGTM. I'll wait for a day before committing to see > if anyone has better ideas. > Pushed. -- With Regards, Amit Kapila.
Re: [PATCH] Identify LWLocks in tracepoints
On Thu, 29 Apr 2021 at 15:31, Peter Eisentraut wrote: > > So if you could produce a separate patch that adds the > > _ENABLED guards targeting PG14 (and PG13), that would be helpful. > > Here is a proposed patch for this. LGTM. Applies and builds fine on master and (with default fuzz) on REL_13_STABLE. Works as expected. This does increase the size of LWLockAcquire() etc slightly but since it skips these function calls, and the semaphores are easily predicted, I don't have any doubt it's a net win. +1 for merge.
Re: [PATCH] Identify LWLocks in tracepoints
On Wed, 14 Apr 2021, 22:29 Robert Haas, wrote: > On Tue, Apr 13, 2021 at 10:42 PM Craig Ringer > wrote: > > I'd really love it if a committer could add an explanatory comment or > > two in the area though. I'm happy to draft a comment patch if anyone > > agrees my suggestion is sensible. The key things I needed to know when > > studying the code were: > > [...] > > I'm willing to review a comment patch along those lines. > Cool. I'll draft soon. I since noticed that some of the info is present, but it's in lwlock.h whereas in Pg comment detail is more often than not in the .c file. I prefer it in headers myself anyway, since it's more available to tools like doxygen. I'll add a few "see lwlock.h" hints, a short para about appropriate lwlock use in the .c into comment etc and post on a separate thread soon. > > I'm actually inclined to revise the patch I sent in order to *remove* > > the LWLock name from the tracepoint argument. > Reducing the overheads is good, but I have no opinion on what's > important for people doing tracing, because I am not one of those > people. > Truthfully I'm not convinced anyone is "those people" right now. I don't think anyone is likely to be making serious use of them due to their limitations. Certainly that'll be the case for the txn ones which are almost totally useless. They only track the localxid lifecycle, they don't track real txid allocation, WAL writing, commit (wal or shmem), etc.
Re: Use simplehash.h instead of dynahash in SMgr
I've attached an updated patch. I forgot to call SH_ENTRY_CLEANUP, when it's defined during SH_RESET. I also tided up a couple of comments and change the code to use pg_rotate_right32(.., 31) instead of adding a new function for pg_rotate_left32 and calling that to shift left 1 bit. David v3-0001-Use-simplehash.h-hashtables-in-SMgr.patch Description: Binary data
Re: [BUG] "FailedAssertion" reported when streaming in logical replication
On Thu, Apr 29, 2021 at 5:24 PM Amit Kapila wrote: > > On Tue, Apr 27, 2021 at 5:18 PM Dilip Kumar wrote: > > > > I have modified the patch based on the above comments. > > > > The patch looks good to me. I have slightly modified the comments and > commit message. See, what you think of the attached? I think we can > leave the test for this as there doesn't seem to be an easy way to > automate it. Your changes look good to me. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
Re: Enhanced error message to include hint messages for redundant options error
On Fri, Apr 30, 2021 at 8:16 AM Bharath Rupireddy
wrote:
>
> On Thu, Apr 29, 2021 at 10:44 PM Alvaro Herrera
> wrote:
> >
> > On 2021-Apr-29, vignesh C wrote:
> >
> > > Thanks for the comments, please find the attached v3 patch which has
> > > the change for the first part.
> >
> > Looks good to me. I would only add parser_errposition() to the few
> > error sites missing that.
>
> Yes, we need to add parser_errposition as agreed in [1].
>
> I think we will have to make changes in compute_common_attribute as
> well because the error in the duplicate_error goto statement is
> actually for the duplicate option specified more than once, we can do
> something like the attached. If it seems okay, it can be merged with
> the main patch.
+ DefElem *duplicate_item = NULL;
+
if (strcmp(defel->defname, "volatility") == 0)
{
if (is_procedure)
goto procedure_error;
if (*volatility_item)
- goto duplicate_error;
+ duplicate_item = defel;
In this function, we already have the "defel" variable then I do not
understand why you are using one extra variable and assigning defel to
that?
If the goal is to just improve the error message then you can simply
use defel->defname?
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
Re: Enhanced error message to include hint messages for redundant options error
On Fri, Apr 30, 2021 at 10:17 AM Dilip Kumar wrote: > In this function, we already have the "defel" variable then I do not > understand why you are using one extra variable and assigning defel to > that? > If the goal is to just improve the error message then you can simply > use defel->defname? Yeah. I can do that. Thanks for the comment. While on this, I also removed the duplicate_error and procedure_error goto statements, because IMHO, using goto statements is not an elegant way. I used boolean flags to do the job instead. See the attached and let me know what you think. Just for completion, I also attached Vignesh's latest patch v3 as-is, in case anybody wants to review it. With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com v3-0001-Enhance-error-message.patch Description: Binary data v2-0001-compute_common_attribute.patch Description: Binary data
Re: Remove redundant variable from transformCreateStmt
On Fri, Apr 30, 2021 at 7:07 AM Justin Pryzby wrote: > > On Thu, Apr 29, 2021 at 02:39:42PM -0400, Alvaro Herrera wrote: > > On 2021-Apr-29, Tom Lane wrote: > > > Alvaro Herrera writes: > > > > I'd do it like this. Note I removed an if/else block in addition to > > > > your changes. > > > > > > > I couldn't convince myself that this is worth pushing though; either we > > > > push it to all branches (which seems unwarranted) or we create > > > > back-patching hazards. > > > > > > Yeah ... an advantage of the if/else coding is that it'd likely be > > > simple to extend to cover additional statement types, should we ever > > > wish to do that. The rendering you have here is nice and compact, > > > but it would not scale up well. > > > > That makes sense. But that part is not in Amul's patch -- he was only > > on about removing the is_foreign_table Boolean. If I remove the if/else > > block change, does the rest of the patch looks something we'd want to > > have? I kinda agree that the redundant variable is "ugly". Is it worth > > removing? My hunch is no. > > Getting rid of a redundant, boolean variable is good not because it's more > efficient but because it's one fewer LOC to read and maintain (and an > opportunity for inconsistency, I suppose). Yes. > Also, this is a roundabout and too-verbose way to invert a boolean: > | transformCheckConstraints(&cxt, !is_foreign_table ? true : false); I agree to remove only the redundant variable, is_foreign_table but not the if else block as Tom said: it's not scalable. We don't need to back patch this change. With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com
Re: Enhanced error message to include hint messages for redundant options error
On Fri, Apr 30, 2021 at 10:43 AM Bharath Rupireddy wrote: > > On Fri, Apr 30, 2021 at 10:17 AM Dilip Kumar wrote: > > In this function, we already have the "defel" variable then I do not > > understand why you are using one extra variable and assigning defel to > > that? > > If the goal is to just improve the error message then you can simply > > use defel->defname? > > Yeah. I can do that. Thanks for the comment. > > While on this, I also removed the duplicate_error and procedure_error > goto statements, because IMHO, using goto statements is not an elegant > way. I used boolean flags to do the job instead. See the attached and > let me know what you think. Okay, but I see one side effect of this, basically earlier on procedure_error and duplicate_error we were not assigning anything to output parameters, e.g. volatility_item, but now those values will be assigned with defel even if there is an error. So I think we should better avoid such change. But even if you want to do then better check for any impacts on the caller. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
Re: Enhanced error message to include hint messages for redundant options error
On Fri, Apr 30, 2021 at 10:51 AM Dilip Kumar wrote:
>
> On Fri, Apr 30, 2021 at 10:43 AM Bharath Rupireddy
> wrote:
> >
> > On Fri, Apr 30, 2021 at 10:17 AM Dilip Kumar wrote:
> > > In this function, we already have the "defel" variable then I do not
> > > understand why you are using one extra variable and assigning defel to
> > > that?
> > > If the goal is to just improve the error message then you can simply
> > > use defel->defname?
> >
> > Yeah. I can do that. Thanks for the comment.
> >
> > While on this, I also removed the duplicate_error and procedure_error
> > goto statements, because IMHO, using goto statements is not an elegant
> > way. I used boolean flags to do the job instead. See the attached and
> > let me know what you think.
>
> Okay, but I see one side effect of this, basically earlier on
> procedure_error and duplicate_error we were not assigning anything to
> output parameters, e.g. volatility_item, but now those values will be
> assigned with defel even if there is an error.
Yes, but on ereport(ERROR, we don't come back right? The txn gets
aborted and the control is not returned to the caller instead it will
go to sigjmp_buf of the backend.
> So I think we should
> better avoid such change. But even if you want to do then better
> check for any impacts on the caller.
AFAICS, there will not be any impact on the caller, as the control
doesn't return to the caller on error.
And another good reason to remove the goto statements is that they
have return false; statements just to suppress the compiler and having
them after ereport(ERROR, doesn't make any sense to me.
duplicate_error:
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("conflicting or redundant options"),
parser_errposition(pstate, defel->location)));
return false;/* keep compiler quiet */
procedure_error:
ereport(ERROR,
(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
errmsg("invalid attribute in procedure definition"),
parser_errposition(pstate, defel->location)));
return false;
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
Re: Enhanced error message to include hint messages for redundant options error
On Fri, Apr 30, 2021 at 11:09 AM Bharath Rupireddy
wrote:
>
> On Fri, Apr 30, 2021 at 10:51 AM Dilip Kumar wrote:
> >
> > On Fri, Apr 30, 2021 at 10:43 AM Bharath Rupireddy
> > wrote:
> > >
> > > On Fri, Apr 30, 2021 at 10:17 AM Dilip Kumar
> > > wrote:
> > > > In this function, we already have the "defel" variable then I do not
> > > > understand why you are using one extra variable and assigning defel to
> > > > that?
> > > > If the goal is to just improve the error message then you can simply
> > > > use defel->defname?
> > >
> > > Yeah. I can do that. Thanks for the comment.
> > >
> > > While on this, I also removed the duplicate_error and procedure_error
> > > goto statements, because IMHO, using goto statements is not an elegant
> > > way. I used boolean flags to do the job instead. See the attached and
> > > let me know what you think.
> >
> > Okay, but I see one side effect of this, basically earlier on
> > procedure_error and duplicate_error we were not assigning anything to
> > output parameters, e.g. volatility_item, but now those values will be
> > assigned with defel even if there is an error.
>
> Yes, but on ereport(ERROR, we don't come back right? The txn gets
> aborted and the control is not returned to the caller instead it will
> go to sigjmp_buf of the backend.
>
> > So I think we should
> > better avoid such change. But even if you want to do then better
> > check for any impacts on the caller.
>
> AFAICS, there will not be any impact on the caller, as the control
> doesn't return to the caller on error.
I see.
other comments
if (strcmp(defel->defname, "volatility") == 0)
{
if (is_procedure)
- goto procedure_error;
+ is_procedure_error = true;
if (*volatility_item)
- goto duplicate_error;
+ is_duplicate_error = true;
Another side effect I see is, in the above check earlier if
is_procedure was true it was directly goto procedure_error, but now it
will also check the if (*volatility_item) and it may set
is_duplicate_error also true, which seems wrong to me. I think you
can change it to
if (is_procedure)
is_procedure_error = true;
else if (*volatility_item)
is_duplicate_error = true;
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
Re: Replication slot stats misgivings
On Fri, Apr 30, 2021 at 5:55 AM Masahiko Sawada wrote:
>
> On Thu, Apr 29, 2021 at 9:44 PM vignesh C wrote:
> >
> >
> >
> > On Thu, Apr 29, 2021 at 3:06 PM Amit Kapila wrote:
> > >
> > > On Wed, Apr 28, 2021 at 5:01 PM Amit Kapila
> > > wrote:
> > > >
> > > > On Wed, Apr 28, 2021 at 4:51 PM Masahiko Sawada
> > > > wrote:
> > > > >
> > > > > On Wed, Apr 28, 2021 at 6:39 PM Amit Kapila
> > > > > wrote:
> > > >
> > > > @@ -1369,7 +1369,7 @@ ReorderBufferIterTXNNext(ReorderBuffer *rb,
> > > > ReorderBufferIterTXNState *state)
> > > > * Update the total bytes processed before releasing the current set
> > > > * of changes and restoring the new set of changes.
> > > > */
> > > > - rb->totalBytes += rb->size;
> > > > + rb->totalBytes += entry->txn->total_size;
> > > > if (ReorderBufferRestoreChanges(rb, entry->txn, &entry->file,
> > > > &state->entries[off].segno))
> > > >
> > > > I have not tested this but won't in the above change you need to check
> > > > txn->toptxn for subtxns?
> > > >
> > >
> > > Now, I am able to reproduce this issue:
> > > Create table t1(c1 int);
> > > select pg_create_logical_replication_slot('s', 'test_decoding');
> > > Begin;
> > > insert into t1 values(1);
> > > savepoint s1;
> > > insert into t1 select generate_series(1, 10);
> > > commit;
> > >
> > > postgres=# select count(*) from pg_logical_slot_peek_changes('s1', NULL,
> > > NULL);
> > > count
> > >
> > > 15
> > > (1 row)
> > >
> > > postgres=# select * from pg_stat_replication_slots;
> > > slot_name | spill_txns | spill_count | spill_bytes | stream_txns |
> > > stream_count | stream_bytes | total_txns | total_bytes |
> > > stats_reset
> > > ---++-+-+-+--+--++-+--
> > > s1| 0 | 0 | 0 | 0 |
> > > 0 |0 | 2 |13200672 | 2021-04-29
> > > 14:33:55.156566+05:30
> > > (1 row)
> > >
> > > select * from pg_stat_reset_replication_slot('s1');
> > >
> > > Now reduce the logical decoding work mem to allow spilling.
> > > postgres=# set logical_decoding_work_mem='64kB';
> > > SET
> > > postgres=# select count(*) from pg_logical_slot_peek_changes('s1', NULL,
> > > NULL);
> > > count
> > >
> > > 15
> > > (1 row)
> > >
> > > postgres=# select * from pg_stat_replication_slots;
> > > slot_name | spill_txns | spill_count | spill_bytes | stream_txns |
> > > stream_count | stream_bytes | total_txns | total_bytes |
> > > stats_reset
> > > ---++-+-+-+--+--++-+--
> > > s1| 1 | 202 |1320 | 0 |
> > > 0 |0 | 2 | 672 | 2021-04-29
> > > 14:35:21.836613+05:30
> > > (1 row)
> > >
> > > You can notice that after we have allowed spilling the 'total_bytes'
> > > stats is showing a different value. The attached patch fixes the issue
> > > for me. Let me know what do you think about this?
> >
> > I found one issue with the following scenario when testing with
> > logical_decoding_work_mem as 64kB:
> >
> > BEGIN;
> > INSERT INTO t1 values(generate_series(1,1));
> > SAVEPOINT s1;
> > INSERT INTO t1 values(generate_series(1,1));
> > COMMIT;
> > SELECT count(*) FROM pg_logical_slot_get_changes('regression_slot1', NULL,
> > NULL, 'include-xids', '0', 'skip-empty-xacts', '1');
> > select * from pg_stat_replication_slots;
> > slot_name | spill_txns | spill_count | spill_bytes | stream_txns |
> > stream_count | stream_bytes | total_txns | total_bytes |
> > stats_reset
> > --++-+-+-+--+--++-+--
> > regression_slot1 | 6 | 154 | 9130176 | 0 |
> > 0 |0 | 1 | 4262016 | 2021-04-29
> > 17:50:00.080663+05:30
> > (1 row)
> >
> > Same thing works fine with logical_decoding_work_mem as 64MB:
> > select * from pg_stat_replication_slots;
> >slot_name | spill_txns | spill_count | spill_bytes | stream_txns |
> > stream_count | stream_bytes | total_txns | total_bytes |
> > stats_reset
> > --++-+-+-+--+--++-+--
> > regression_slot1 | 6 | 154 | 9130176 | 0 |
> > 0 |0 | 1 | 264 | 2021-04-29
> > 17:50:00.080663+05:30
> > (1 row)
> >
> > The patch required one change:
> > - rb->totalBytes += rb->size;
> > + if (entry->txn->toptxn)
> > + rb->totalBytes += entry->txn->toptxn->total_size;
> > + else
> > + rb->totalBytes += entry->txn->t
Re: Included xid in restoring reorder buffer changes from disk log message
On Thu, Apr 29, 2021 at 9:45 PM vignesh C wrote: > > Hi, > > While debugging one of the logical decoding issues, I found that xid was not > included in restoring reorder buffer changes from disk log messages. > Attached a patch for it. I felt including the XID will be helpful in > debugging. Thoughts? > It makes sense to include xid in the debug message, earlier I thought that will it be a good idea to also print the toplevel_xid. But I think it is for debug purposes and only we have the xid we can fetch the other parameters so maybe adding xid is good enough. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
