Re: Race condition in recovery?

[Dilip Kumar]

On Sat, May 15, 2021 at 3:58 AM Robert Haas  wrote:
>
> I did notice, but keep in mind that this was more than 8 years ago.
> Even if Heikki is reading this thread, he may not remember why he
> changed 1 line of code one way rather than another in 2013. I mean if
> he does that's great, but it's asking a lot.

I agree with your point.  But I think that one line is related to the
purpose of this commit and I have explained (in 3rd paragraph below)
why do I think so.

 As I understand it, the general issue here was that if
> XLogFileReadAnyTLI() was called before expectedTLEs got set, then
> prior to this commit it would have to fail, because the foreach() loop
> in that function would be iterating over an empty list. Heikki tried
> to make it not fail in that case, by setting tles =
> readTimeLineHistory(recoveryTargetTLI), so that the foreach loop
> *wouldn't* get an empty list.

I might be missing something but I don't agree with this logic.  If
you see prior to this commit the code flow was like below[1].  So my
point is if we are calling XLogFileReadAnyTLI() somewhere while
reading the checkpoint record then note that expectedTLEs were
initialized unconditionally before even try to read that checkpoint
record.  So how expectedTLEs could be uninitialized in
LogFileReadAnyTLI?

[1]
StartupXLOG(void)
{


recoveryTargetTLI = ControlFile->checkPointCopy.ThisTimeLineID;
...
readRecoveryCommandFile();
...
expectedTLEs = readTimeLineHistory(recoveryTargetTLI);
...
..
record = ReadCheckpointRecord(checkPointLoc, 0);


Another point which I am not sure about but still I think that one
line (expectedTLEs = readTimeLineHistory(receiveTLI);), somewhere
related to the purpose of this commit.  Let me explain why do I think
so.  Basically, before this commit, we were initializing
"expectedTLEs" based on the history file of "recoveryTargetTLI", right
after calling "readRecoveryCommandFile()" (this function will
initialize recoveryTargetTLI based on the recovery target, and it
ensures it read the respective history file).  Now, right after this
point, there was a check as shown below[2], which is checking whether
the checkpoint TLI exists in the  "expectedTLEs" which is initialized
based on recoveryTargetTLI.  And it appeared that this check was
failing in some cases which this commit tried to fix and all other
code is there to support that.  Because now before going for reading
the checkpoint we are not initializing "expectedTLEs" so now after
moving this line from here it was possible that "expectedTLEs" is not
initialized in XLogFileReadAnyTLI() and the remaining code in
XLogFileReadAnyTLI() is to handle that part.

Now, coming to my point that why this one line is related,  In this
one line (expectedTLEs = readTimeLineHistory(receiveTLI);), we
completely avoiding recoveryTargetTLI and initializing "expectedTLEs"
based on the history file of the TL from which we read the checkpoint,
so now, there is no scope of below[2] check to fail because note that
we are not initializing "expectedTLEs" based on the
"recoveryTargetTLI" but we are initializing from the history from
where we read checkpoint.

So I feel if we directly fix this one line to initialize
"expectedTLEs" from "recoveryTargetTLI" then it will expose to the
same problem this commit tried to fix.

[2]
if (tliOfPointInHistory(ControlFile->checkPoint, expectedTLEs) !=
ControlFile->checkPointCopy.ThisTimeLineID)
{
error()
}


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

Re: Added missing tab completion for alter subscription set option

[vignesh C]

On Fri, May 14, 2021 at 7:10 PM Bharath Rupireddy <
bharath.rupireddyforpostg...@gmail.com> wrote:
>
> On Fri, May 14, 2021 at 6:51 PM vignesh C  wrote:
> >
> > On Fri, May 14, 2021 at 12:25 PM Bharath Rupireddy
> >  wrote:
> > >
> > > On Fri, May 14, 2021 at 12:00 PM vignesh C 
wrote:
> > > >
> > > > Hi,
> > > >
> > > > While I was reviewing one of the logical decoding features, I found
> > > > Streaming and binary options were missing in tab completion for the
> > > > alter subscription set option, the attached patch has the changes
for
> > > > the same.
> > > > Thoughts?
> > >
> > > +1.
> > >
> > > Without patch:
> > > postgres=# alter subscription testsub set (S
> > > SLOT_NAME   SYNCHRONOUS_COMMIT
> > >
> > > With patch:
> > > postgres=# alter subscription testsub set (
> > > BINARY  SLOT_NAME   STREAMING
SYNCHRONOUS_COMMIT
> > >
> > > How about ordering the options alphabetically as the tab complete
> > > output anyways shows that way? I'm not sure if that's the practice,
> > > but just a thought.
> >
> > I did not see any rule for this, but also did not see any harm in
> > keeping it in alphabetical order, so changed it in the attached patch.
>
> Thanks. Just a few nitpicks:
> 1) How about patch name: "Add tab completion for ALTER SUBSCRIPTION
> SET options streaming and binary"?

Modified.

> 2) How about a detailed message: "Tab completion for the options
> streaming and binary were missing in case of ALTER SUBSCRIPTION SET
> command. This patch adds them."?
>

Modified.

> You may want to add this in commitfest so that we don't lose track of it.

I have added a commitfest entry at [1].
Thanks for the comments, the attached patch has the changes for the same.

[1] - https://commitfest.postgresql.org/33/3116/

Regards,
Vignesh
From 9baa6c750c99a8e3bbd5a3581980b10f10e9e45e Mon Sep 17 00:00:00 2001
From: vignesh 
Date: Fri, 14 May 2021 11:37:05 +0530
Subject: [PATCH v3] Add tab completion for ALTER SUBSCRIPTION SET options
 streaming and binary.

Tab completion for the options streaming and binary were missing in case of
ALTER SUBSCRIPTION SET command. This patch adds them.
---
 src/bin/psql/tab-complete.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 6598c5369a..1b9661fb24 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -1665,7 +1665,7 @@ psql_completion(const char *text, int start, int end)
 		COMPLETE_WITH("(", "PUBLICATION");
 	/* ALTER SUBSCRIPTION  SET ( */
 	else if (HeadMatches("ALTER", "SUBSCRIPTION", MatchAny) && TailMatches("SET", "("))
-		COMPLETE_WITH("slot_name", "synchronous_commit");
+		COMPLETE_WITH("binary", "slot_name", "streaming", "synchronous_commit");
 	/* ALTER SUBSCRIPTION  SET PUBLICATION */
 	else if (HeadMatches("ALTER", "SUBSCRIPTION", MatchAny) && TailMatches("SET", "PUBLICATION"))
 	{
-- 
2.25.1

Re:Re: Parallel scan with SubTransGetTopmostTransaction assert coredump

[刘鹏程]

Hi Greg,
  It is really weird. Could you make sure is the SnapShot overflow in you ENV? 
It is very impoint. 
  Abount SnapShot overflow and Subtrans, you can refer this 
https://www.cybertec-postgresql.com/en/subtransactions-and-performance-in-postgresql/.
 
 
  In the script sub_120.sql, for one transaction, we use 120 transcations. So 
this pgxact->overflowed will be set to true. 
  Then snapshot must be overflow. When MVCC, it will call 
SubTransGetTopmostTransaction. 
  So the snapshot overflow is requirement.
  
  Even though there is no coredump in you ENV, from the codes, we can find some 
clue.
 
  First, in main process , ActiveSnapshot xmin is very likely preceds 
TransactionSnapShot xmin.
  Second, in parallel work process, it sets TransactionXmin with 
TransactionSnapShot from main process. But table Scan with ative Snapshot from 
main process.
  So in parallel work process SubTransGetTopmostTransaction, the Assert 
TransactionIdFollowsOrEquals(xid, TransactionXmin) is not correct. 
  At least this assert is unsuitable for parallel work process. 
  For my analyze, if there is any incorrect, please corret me.
  
  BTW, I test it in a high performance server. It is verly easily be 
reproduced. My colleague and me use different environment both can reproduce 
it. 
  
  
  Thanks
  Pengcheng

Rewriting the test of pg_upgrade as a TAP test - take three - remastered set

[Michael Paquier]

Hi,
(Adding Andrew in CC for the buildfarm and PostgresNode parts.)

$subject has been around for a couple of years now, with the following
threads:
https://www.postgresql.org/message-id/20180126080026.gi17...@paquier.xyz
https://www.postgresql.org/message-id/cab7npqrdan1a1ynjxnl9t1juewct8ttqq29dnv8w_o37+e8...@mail.gmail.com

An advantage of moving to TAP is that we can then remove the support
for upgrades within the MSVC scripts, and also remove pg_upgrade's
test.sh that has accumulated tweaks that are solved by the TAP tests,
resulting in cleanup:
 8 files changed, 230 insertions(+), 403 deletions(-)

Based on the past discussions, there were two obstacles preventing to
do this switch:
- Support for tests with older versions, something where the gap as
been closed thanks to Andrew's work in 4c4eaf3d.
- Buildfarm support, and I am not sure how things need to be extended
there.

Another thing to note is that HEAD uses oldbindir, bindir and libdir
to track the location of the old and new libraries and binaries.  With
the infrastructure in place, once can define only an install path for
a PostgresNode, so this version uses only two variables:
- oldinstall, for the installation path of the version to upgrade
from.
- oldsrc, to point to the source of the old version.

It is not difficult to switch to one approach or the other, but
reducing the logic to a minimum number of variables is a good deal to
take IMO.

I have been testing this patch a bit with older versions, down to 12,
and that was logically working, and PostgresNode may need more to be
able to work with ~11, as documented in get_new_node().  And I have
not tested that with MSVC yet.  Anyway, attached is a new patch to
make the discussion move on.  Even if there is still work to be done
here, would people here still support this switch?

Thanks,
--
Michael
From 557e1bd4cea5e1081ea77e364c3e7f37ad3c5268 Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Sat, 15 May 2021 10:44:18 +0900
Subject: [PATCH] Switch tests of pg_upgrade to use TAP

---
 src/bin/pg_upgrade/Makefile|  17 +-
 src/bin/pg_upgrade/TESTING |   9 +-
 src/bin/pg_upgrade/t/001_basic.pl  |   9 +
 src/bin/pg_upgrade/t/002_pg_upgrade.pl | 188 +
 src/bin/pg_upgrade/test.sh | 272 -
 src/test/perl/PostgresNode.pm  |  25 +++
 doc/src/sgml/install-windows.sgml  |   1 -
 src/tools/msvc/vcregress.pl| 112 +-
 8 files changed, 230 insertions(+), 403 deletions(-)
 create mode 100644 src/bin/pg_upgrade/t/001_basic.pl
 create mode 100644 src/bin/pg_upgrade/t/002_pg_upgrade.pl
 delete mode 100644 src/bin/pg_upgrade/test.sh

diff --git a/src/bin/pg_upgrade/Makefile b/src/bin/pg_upgrade/Makefile
index 44d06be5a6..fa8dee0a9c 100644
--- a/src/bin/pg_upgrade/Makefile
+++ b/src/bin/pg_upgrade/Makefile
@@ -49,17 +49,8 @@ clean distclean maintainer-clean:
 	   pg_upgrade_dump_globals.sql \
 	   pg_upgrade_dump_*.custom pg_upgrade_*.log
 
-# When $(MAKE) is present, make automatically infers that this is a
-# recursive make. which is not actually what we want here, as that
-# e.g. prevents output synchronization from working (as make thinks
-# that the subsidiary make knows how to deal with that itself, but
-# we're invoking a shell script that doesn't know). Referencing
-# $(MAKE) indirectly avoids that behaviour.
-# See https://www.gnu.org/software/make/manual/html_node/MAKE-Variable.html#MAKE-Variable
-NOTSUBMAKEMAKE=$(MAKE)
+check:
+	$(prove_check)
 
-check: test.sh all temp-install
-	MAKE=$(NOTSUBMAKEMAKE) $(with_temp_install) bindir=$(abs_top_builddir)/tmp_install/$(bindir) EXTRA_REGRESS_OPTS="$(EXTRA_REGRESS_OPTS)" $(SHELL) $<
-
-# installcheck is not supported because there's no meaningful way to test
-# pg_upgrade against a single already-running server
+installcheck:
+	$(prove_installcheck)
diff --git a/src/bin/pg_upgrade/TESTING b/src/bin/pg_upgrade/TESTING
index e69874b42d..b589bcaf6d 100644
--- a/src/bin/pg_upgrade/TESTING
+++ b/src/bin/pg_upgrade/TESTING
@@ -9,14 +9,11 @@ an upgrade from the version in this source tree to a new instance of
 the same version.
 
 To test an upgrade from a different version, you must have a built
-source tree for the old version as well as this version, and you
-must have done "make install" for both versions.  Then do:
+source tree for the old version as well as this version.  Then do:
 
 export oldsrc=...somewhere/postgresql	(old version's source tree)
-export oldbindir=...otherversion/bin	(old version's installed bin dir)
-export bindir=...thisversion/bin	(this version's installed bin dir)
-export libdir=...thisversion/lib	(this version's installed lib dir)
-sh test.sh
+export oldinstall=...otherversion/bin	(old version's install base path)
+make check
 
 In this case, you will have to manually eyeball the resulting dump
 diff for version-specific differences, as explained below.
diff --git a/src/bin/pg_upgrade/t/001_basic.pl 

Re: compute_query_id and pg_stat_statements

[Justin Pryzby]

On Fri, May 14, 2021 at 07:50:13PM -0400, Alvaro Herrera wrote:
> +++ b/doc/src/sgml/config.sgml
> @@ -7643,7 +7643,12 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' 
> WITH csv;
>  identifier to be computed.  Note that an external module can
>  alternatively be used if the in-core query identifier computation
>  method is not acceptable.  In this case, in-core computation
> -must be disabled.  The default is off.
> +must be always disabled.
> +Valid values are off (always disabled),
> +on (always enabled) and auto,
> +which let modules such as 
> +automatically enable it.
> +The default is auto.

which lets

> +/* True when a module requests query IDs and they're set auto */
> +bool query_id_enabled = false;

Does "they're" mean the GUC compute_query_id ?

> +/*
> + * This should only be called if IsQueryIdEnabled()
> + * return true.
> + */
>  JumbleState *
>  JumbleQuery(Query *query, const char *querytext)

Should it Assert() that ?

Maybe you should update this too ?
doc/src/sgml/release-14.sgml

Re: compute_query_id and pg_stat_statements

[Alvaro Herrera]

On 2021-May-14, Julien Rouhaud wrote:

> On Fri, May 14, 2021 at 12:20:00PM +0900, Fujii Masao wrote:

> > +void
> > +EnableQueryId(void)
> > +{
> > +   if (compute_query_id == COMPUTE_QUERY_ID_AUTO)
> > +   auto_query_id_enabled = true;
> > 
> > Shouldn't EnableQueryId() enable auto_query_id_enabled whatever 
> > compute_query_id is?
> > Otherwise, for example, the following scenario can happen and it's a bit 
> > strange.
> > 
> > 1. The server starts up with shared_preload_libraries=pg_stat_statements 
> > and compute_query_id=on
> > 2. compute_query_id is set to auto and the configuration file is reloaded
> > Then, even though compute_query_id is auto and pg_stat_statements is loaded,
> > query ids are not computed and no queries are tracked by pg_stat_statements.
> 
> +1.

That makes sense.  Done in this version.

I took out the new WARNING in pg_stat_statements.  It's not clear to me
that that's terribly useful (it stops working as soon as you have one
query in the pg_stat_statements stash and later disable everything).
Maybe there is an useful warning to add, but I think it's independent of
changing the GUC behabior.

I also made IsQueryIdEnabled() a static inline in queryjumble.h, to
avoid a function call at every site where we need that.  Also did some
light doc editing.

I think I should aim at pushing this tomorrow morning.

> Note that if you switch from compute_query_id = off + custom
> query_id + pg_stat_statements to compute_query_id = auto then thing will
> immediately break (as we instruct third-party plugins authors to error out in
> that case), which is way better than breaking at the next random service
> restart.

Hmm, ok.  I tested pg_queryid and that behavior of throwing an error
seems quite unhelpful -- it basically makes every single query fail if
you set things wrong.  I think that instruction is bogus and should be
reconsidered.  Maybe pg_queryid could use a custom Boolean GUC that
tells it to overwrite the core query_id if that is enabled, or to just
silently do nothing in that case.



While thinking about this patch it occurred to that an useful gadget
might be to let pg_stat_statements be loaded always, but with
compute_query_id=false; so it's never active; except if you
  ALTER {DATABASE, USER} foo SET (compute_query_id=on);
so that it's possible to enable it selectively.  I think this doesn't
currently achieve anything because pgss_store is always called
regardless of query ID being active (so you'd always have at least one
function call as performance penalty, only that the work would be for
naught), but that seems a simple change to make.  I didn't look closely
to see what other things would need patched.

-- 
Álvaro Herrera39°49'30"S 73°17'W
Maybe there's lots of data loss but the records of data loss are also lost.
(Lincoln Yeoh)
>From acd29794dbba5e9828d78fd348621374a73c6d7a Mon Sep 17 00:00:00 2001
From: Alvaro Herrera 
Date: Fri, 14 May 2021 16:55:18 -0400
Subject: [PATCH v4] Allow compute_query_id set to 'auto'

---
 .../pg_stat_statements/pg_stat_statements.c   |  6 +++
 .../pg_stat_statements.conf   |  1 -
 doc/src/sgml/config.sgml  |  9 -
 doc/src/sgml/pgstatstatements.sgml| 14 +++
 src/backend/commands/explain.c|  2 +-
 src/backend/parser/analyze.c  |  4 +-
 src/backend/postmaster/postmaster.c   |  3 ++
 src/backend/tcop/postgres.c   |  2 +-
 src/backend/utils/misc/guc.c  | 38 ++-
 src/backend/utils/misc/postgresql.conf.sample |  2 +-
 src/backend/utils/misc/queryjumble.c  | 23 +++
 src/include/utils/guc.h   |  1 -
 src/include/utils/queryjumble.h   | 33 +++-
 13 files changed, 108 insertions(+), 30 deletions(-)

diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index a85f962801..09433c8c96 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -369,6 +369,12 @@ _PG_init(void)
 	if (!process_shared_preload_libraries_in_progress)
 		return;
 
+	/*
+	 * Inform the postmaster that we want to enable query_id calculation if
+	 * compute_query_id is set to auto.
+	 */
+	EnableQueryId();
+
 	/*
 	 * Define (or redefine) custom GUC variables.
 	 */
diff --git a/contrib/pg_stat_statements/pg_stat_statements.conf b/contrib/pg_stat_statements/pg_stat_statements.conf
index e47b26040f..13346e2807 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.conf
+++ b/contrib/pg_stat_statements/pg_stat_statements.conf
@@ -1,2 +1 @@
 shared_preload_libraries = 'pg_stat_statements'
-compute_query_id = on
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 45bd1f1b7e..60d8b24f5e 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -7627,7 +7627,7 @@ COPY postgres_log FROM 

Re: Possible memory corruption (src/timezone/zic.c b/src/timezone/zic.c)

[Tom Lane]

I wrote:
> So the question for us is whether it's worth trying to make pgreadlink
> conform to the letter of the POSIX spec in this detail.  TBH, I can't
> get excited about that, at least not so far as zic's usage is concerned.

Hmmm ... on closer inspection, though, it might not be that hard.
pgreadlink is already using a fixed-length buffer (with only enough
room for MAX_PATH WCHARs) for the input of WideCharToMultiByte.  So
it could use a fixed-length buffer of say 4 * MAX_PATH bytes for the
output, and then transfer just the appropriate amount of data to the
caller's buffer.

regards, tom lane

Re: Possible memory corruption (src/timezone/zic.c b/src/timezone/zic.c)

[Tom Lane]

Ranier Vilela  writes:
> Per Coverity.
> CID 1412632 (#1 of 1): Out-of-bounds access (OVERRUN)1.
> overrun-buffer-val: Overrunning buffer pointed to by  of 1 bytes by
> passing it to a function which accesses it at byte offset 4.

> For some people, Coverity opinions count zero.

This particular complaint seems to match a pattern that Coverity has
been generating a lot lately.  I've yet to see one that wasn't a
false positive, so it looks like a Coverity bug to me.

> It doesn't matter if WideCharToMultiByte, it will fail or not, the danger
> exists.
> If WideCharToMultiByte returns 4, memmove will possibly destroy 4 bytes.

This analysis seems to me to be nonsense.

(1) sizeof(char) is one, per the C standard.  Therefore, the existing
coding in itssymlink accurately describes the size of the buffer it's
providing.  The alternative you propose also accurately describes
the size of the buffer it's providing.  It's nonsense to suppose that
one is safer than the other --- if readlink is willing to write past
the specified buffer size, they're both equally dangerous.  So this
fix fixes nothing.

(2) As an independent matter, we should worry about whether our
pgreadlink() implementation is capable of writing past the specified
buffer size.  I don't think that WideCharToMultiByte will do so;
Microsoft's documentation clearly says that "cbMultiByte" is the
size *in bytes* of the buffer indicated by "lpMultiByteStr".
However it's fair to question whether that bit of code for deleting
"\??\" is safe.  I think it is though.  Per the Microsoft docs,
the return value of WideCharToMultiByte is:

  If successful, returns the number of bytes written to the buffer pointed
  to by lpMultiByteStr. If the function succeeds and cbMultiByte is 0, the
  return value is the required size, in bytes, for the buffer indicated by
  lpMultiByteStr.  [ but we aren't passing zero for cbMultiByte ]

  The function returns 0 if it does not succeed.
  [ and one of the failure cases is: ]
  ERROR_INSUFFICIENT_BUFFER. A supplied buffer size was not large enough,
  or it was incorrectly set to NULL.

So I don't believe that it will return r > 4 when the supplied buffer size
is only 1.  What's going to happen instead is a failure return, because
the string doesn't fit.

Hence, we do have a problem here, which is that pgreadlink is pretty
much always going to fail when used in the way zic.c is using it, and
thus zic is going to fail to recognize symlinks when run on Windows.

The IANA crew are unlikely to care: they're going to say that they're
using readlink() per the POSIX specification for it, and they'll be
right.

So the question for us is whether it's worth trying to make pgreadlink
conform to the letter of the POSIX spec in this detail.  TBH, I can't
get excited about that, at least not so far as zic's usage is concerned.
What Windows user is going to be using our version of zic to install
timezone files into a subdirectory that has pre-existing symlinks?

By the same token, I'm pretty unexcited about working around pgreadlink's
deficiency by modifying the IANA code in the way you suggest.  It's
painful enough to keep our copy of their code in sync with their updates;
we don't need hacks like that added.

In short, I don't see much of a case for doing anything; but if somebody
were really excited about this they could try to make pgreadlink() fill
the supplied buffer without failing when it's too short.

regards, tom lane

Re: Race condition in recovery?

[Robert Haas]

On Fri, May 14, 2021 at 12:59 AM Dilip Kumar  wrote:
> I am not sure that have you noticed the commit id which changed the
> definition of expectedTLEs, Heikki has committed that change so adding
> him in the list to know his opinion.

I did notice, but keep in mind that this was more than 8 years ago.
Even if Heikki is reading this thread, he may not remember why he
changed 1 line of code one way rather than another in 2013. I mean if
he does that's great, but it's asking a lot.

> =
> ee994272ca50f70b53074f0febaec97e28f83c4e
> Author: Heikki Linnakangas   2013-01-03 14:11:58
> Committer: Heikki Linnakangas   2013-01-03 14:11:58
>
> Delay reading timeline history file until it's fetched from master.
> .
>  Without the timeline history file, recovering that file
> will fail as the older timeline ID is not recognized to be an ancestor of
> the target timeline. If you try to recover from such a backup, using only
> streaming replication to fetch the WAL, this patch is required for that to
> work.
> =
>
> Part of this commit message says that it will not identify the
> recoveryTargetTLI as the ancestor of the checkpoint timeline (without
> history file).  I did not understand what it is trying to say.  Does
> it is trying to say that even though the recoveryTargetTLI is the
> ancestor of the checkpoint timeline but we can not track that because
> we don't have a history file?  So to handle this problem change the
> definition of expectedTLEs to directly point to the checkpoint
> timeline?
>
> Because before this commit, we were directly initializing expectedTLEs
> with the history file of recoveryTargetTLI, we were not even waiting
> for reading the checkpoint,  but under this commit, it is changed.

Well, I think that is talking about what the commit did in general,
not specifically the one line of code that we think may be incorrect.
As I understand it, the general issue here was that if
XLogFileReadAnyTLI() was called before expectedTLEs got set, then
prior to this commit it would have to fail, because the foreach() loop
in that function would be iterating over an empty list. Heikki tried
to make it not fail in that case, by setting tles =
readTimeLineHistory(recoveryTargetTLI), so that the foreach loop
*wouldn't* get an empty list.

Thinking about this a bit more, I think the idea behind the logic this
commit added to XLogFileReadAnyTLI() is that
XLogFileReadAnyTLI(recoveryTargetTLI) may or may not produce the
correct answer. If the timeline history file exists, it will contain
all the information that we need and will return a complete list of
TLEs. But if the file does not exist yet, then it will return a
1-entry list saying that the TLI in question has no parents. If
readTimeLineHistory() actually reads the file, then it's safe to save
the return value in expectedTLEs, but if it doesn't, then it may or
may not be safe. If XLogFileReadAnyTLI calls XLogFileRead and it
works, then the WAL segment we need exists on our target timeline and
we don't actually need the timeline history for anything because we
can just directly begin replay from the target timeline. But if
XLogFileRead fails with the 1-entry dummy list, then we need the
timeline history and don't have it yet, so we have to retry later,
when the history file will hopefully be present, and then at that
point readTimeLineHistory will return a different and better answer
and hopefully it will all work.

I think this is what the commit message is talking about when it says
that "Without the timeline history file, recovering that file will
fail as the older timeline ID is not recognized to be an ancestor of
the target timeline." Without the timeline history file, we can't know
whether some other timeline is an ancestor or not. But the specific
way that manifests is that XLogFileReadAnyTLI() returns a 1-entry
dummy list instead of the real contents of the timeline history file.
This commit doesn't prevent that from happening, but it does prevent
the 1-entry dummy list from getting stored in the global variable
expectedTLEs, except in the case where no timeline switch is occurring
and the lack of history therefore doesn't matter. Without this commit,
if the call to readTimeLineHistory(recoveryTargetTLI) happens at a
time when the timeline history file is not yet available, the 1-entry
dummy list ends up in the global variable and there's no way for it to
ever be replaced with a real history even if the timeline history file
shows up in the archive later.

As I see it, the question on the table here is whether there's any
justification for the fact that when the second switch in
WaitForWALToBecomeAvailable takes the
XLOG_FROM_ARCHIVE/XLOG_FROM_PG_WAL branch, it calls XLogFileReadAnyTLI
which tries to read the history of recoveryTargetTLI, while when that
same switch takes the XLOG_FROM_STREAM branch, it tries to read the
history of receiveTLI. I tend to think it doesn't make sense. On
general principle, archiving 

Possible memory corruption (src/timezone/zic.c b/src/timezone/zic.c)

[Ranier Vilela]

Hi,

Per Coverity.
CID 1412632 (#1 of 1): Out-of-bounds access (OVERRUN)1.
overrun-buffer-val: Overrunning buffer pointed to by  of 1 bytes by
passing it to a function which accesses it at byte offset 4.

For some people, Coverity opinions count zero.
Who knows for others, it helps.

It doesn't matter if WideCharToMultiByte, it will fail or not, the danger
exists.
If WideCharToMultiByte returns 4, memmove will possibly destroy 4 bytes.

The fix, use of the traditional and bogus C style, without tricks.

diff --git a/src/timezone/zic.c b/src/timezone/zic.c
index 0ea6ead2db..a5f7e7f1cd 100644
--- a/src/timezone/zic.c
+++ b/src/timezone/zic.c
@@ -1129,9 +1129,9 @@ static bool
 itssymlink(char const *name)
 {
 #ifdef HAVE_SYMLINK
- char c;
+ char linkpath[MAXPGPATH];

- return 0 <= readlink(name, , 1);
+ return 0 <= readlink(name, linkpath, sizeof(linkpath));
 #else
  return false;
 #endif

regards,
Ranier Vilela


fix_possible_memory_corruption_zic.patch
Description: Binary data

Re: Some other CLOBBER_CACHE_ALWAYS culprits

[Tom Lane]

Alvaro Herrera  writes:
> On 2021-May-14, Tom Lane wrote:
>> An idea I'd been toying with was to make invals probabilistic, that is
>> there would be X% chance of an inval being forced at any particular
>> opportunity.  Then you could dial X up or down to make a tradeoff
>> between speed and the extent of coverage you get from a single run.
>> (Over time, you could expect pretty complete coverage even with X
>> not very close to 1, I think.)

> Maybe we could say that debug_invalidate_system_caches_always=2 means to
> use the current behavior, and debug_invalidate_system_caches_always=1
> uses some probabilistic rule?

What I had in mind was to replace the boolean with an actual fraction.
Probability zero is the non-debug behavior, and probability one gives
you the same result as CLOBBER_CACHE_ALWAYS, and values in between
give you tradeoffs.  But I'm not sure exactly how to extend that to
the recursive cases.

regards, tom lane

Re: Some other CLOBBER_CACHE_ALWAYS culprits

[Tom Lane]

Andres Freund  writes:
> On 2021-05-14 16:53:16 -0400, Tom Lane wrote:
>> An idea I'd been toying with was to make invals probabilistic, that is
>> there would be X% chance of an inval being forced at any particular
>> opportunity.  Then you could dial X up or down to make a tradeoff
>> between speed and the extent of coverage you get from a single run.
>> (Over time, you could expect pretty complete coverage even with X
>> not very close to 1, I think.)

> That'd make sense, I've been wondering about something similar. But I'm
> a bit worried about that making it harder to reproduce problems
> reliably?

Once you know or suspect a problem, you dial X up to 1 and wait.

regards, tom lane

Re: Some other CLOBBER_CACHE_ALWAYS culprits

[Alvaro Herrera]

On 2021-May-14, Tom Lane wrote:

> An idea I'd been toying with was to make invals probabilistic, that is
> there would be X% chance of an inval being forced at any particular
> opportunity.  Then you could dial X up or down to make a tradeoff
> between speed and the extent of coverage you get from a single run.
> (Over time, you could expect pretty complete coverage even with X
> not very close to 1, I think.)

Maybe we could say that debug_invalidate_system_caches_always=2 means to
use the current behavior, and debug_invalidate_system_caches_always=1
uses some probabilistic rule?

-- 
Álvaro Herrera   Valdivia, Chile

Re: Some other CLOBBER_CACHE_ALWAYS culprits

[Andres Freund]

Hi,

On 2021-05-14 16:53:16 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > In essence, debug_invalidate_system_caches_always=1 in some important 
> > aspects
> > behaves like debug_invalidate_system_caches_always=3, due to the syscache
> > involvement.
> 
> Yeah.  I think it's important to test those recursive invalidation
> scenarios, but it could likely be done more selectively.

Agreed. I wonder if the logic could be something like indicating that we
don't invalidate due to pg_class/attribute/am/... (a set of super common
system catalogs) being opened, iff that open is at the "top level". So
we'd e.g. not trigger invalidation for a syscache miss scanning
pg_class, unless the miss happens during a relcache build. But we would
continue to trigger invalidations without further checks if
e.g. pg_subscription is opened.


> > What about having a mode where each "nesting" level of SearchCatCacheMiss
> > allows only one interior InvalidateSystemCaches()?
> 
> An idea I'd been toying with was to make invals probabilistic, that is
> there would be X% chance of an inval being forced at any particular
> opportunity.  Then you could dial X up or down to make a tradeoff
> between speed and the extent of coverage you get from a single run.
> (Over time, you could expect pretty complete coverage even with X
> not very close to 1, I think.)
> 
> This could be extended to what you're thinking about by reducing X
> (according to some rule or other) for each level of cache-flush
> recursion.  The argument to justify that is that recursive cache
> flushes are VERY repetitive, so that even a small probability will
> add up to full coverage of those code paths fairly quickly.

That'd make sense, I've been wondering about something similar. But I'm
a bit worried about that making it harder to reproduce problems
reliably?


> I've not worked out the math to justify any specific proposal
> along this line, though.

FWIW, I've prototyped the idea of only invalidating once for each
syscache level, and it does reduce runtime of

CREATE TABLE blarg_{0,1,2,3}(id serial primary key);
SET debug_invalidate_system_caches_always = 1;
SELECT * FROM blarg_0 join blarg_1 USING (id) join blarg_2 using (id) JOIN 
blarg_3 USING(id);
RESET ALL;

from 7.5s to 4.7s. The benefits are smaller when fewer tables are
accessed, and larger if more (surprising, right :)).

Greetings,

Andres Freund

Re: Some other CLOBBER_CACHE_ALWAYS culprits

[Tom Lane]

Andres Freund  writes:
> In essence, debug_invalidate_system_caches_always=1 in some important aspects
> behaves like debug_invalidate_system_caches_always=3, due to the syscache
> involvement.

Yeah.  I think it's important to test those recursive invalidation
scenarios, but it could likely be done more selectively.

> What about having a mode where each "nesting" level of SearchCatCacheMiss
> allows only one interior InvalidateSystemCaches()?

An idea I'd been toying with was to make invals probabilistic, that is
there would be X% chance of an inval being forced at any particular
opportunity.  Then you could dial X up or down to make a tradeoff
between speed and the extent of coverage you get from a single run.
(Over time, you could expect pretty complete coverage even with X
not very close to 1, I think.)

This could be extended to what you're thinking about by reducing X
(according to some rule or other) for each level of cache-flush
recursion.  The argument to justify that is that recursive cache
flushes are VERY repetitive, so that even a small probability will
add up to full coverage of those code paths fairly quickly.

I've not worked out the math to justify any specific proposal
along this line, though.

regards, tom lane

Re: OOM in spgist insert

[Tom Lane]

Alvaro Herrera  writes:
> This comment made me remember a patch I've had for a while, which splits
> the CHECK_FOR_INTERRUPTS() definition in two -- one of them is
> INTERRUPTS_PENDING_CONDITION() which let us test the condition
> separately; that allows the lock we hold to be released prior to
> actually processing the interrupts.

I've now pushed that macro change ...

> The btree code modified was found to be an actual problem in production
> when a btree is corrupted in such a way that vacuum would get an
> infinite loop.  I don't remember the exact details but I think we saw
> vacuum running for a couple of weeks, and had to restart the server in
> order to terminate it (since it wouldn't respond to signals).

... but I think this bit still needs work, if we still want it at all.
The problem is that it seems to believe that ProcessInterrupts is
guaranteed not to return, which is far from the truth.  Maybe it was
true once, but we've grown a lot of accretions on it that will just
clear InterruptPending and return.  I see that the "return false"
leads directly to an "Assert(false)", which seems unhelpful.

regards, tom lane

Re: PG 14 release notes, first draft

[Bruce Momjian]

On Fri, May 14, 2021 at 03:39:39PM -0400, Tom Lane wrote:
> Bruce Momjian  writes:
> > On Thu, May 13, 2021 at 09:01:41PM -0500, Justin Pryzby wrote:
> >> New SQL-accessible functionality should be included:
> >>> 8c15a29745 Allow ALTER TYPE to update an existing type's typsubscript 
> >>> value.
> 
> > OK, text is:
> 
> > 
> > 
>   
> > 
> > Allow ALTER TYPE to specify or remove a SUBSCRIPT handler
> > (Tom Lane)
> > 
> > 
> 
> I don't understand why we'd bother to list that as a separate bullet item.
> It's an expected part of the custom-subscript capability, I'd think.

I am not sure either.

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

  If only the physical world exists, free will is an illusion.

Re: PG 14 release notes, first draft

[Tom Lane]

Bruce Momjian  writes:
> On Thu, May 13, 2021 at 09:01:41PM -0500, Justin Pryzby wrote:
>> New SQL-accessible functionality should be included:
>>> 8c15a29745 Allow ALTER TYPE to update an existing type's typsubscript value.

> OK, text is:

>   
>   

>   
>   Allow ALTER TYPE to specify or remove a SUBSCRIPT handler
>   (Tom Lane)
>   
>   

I don't understand why we'd bother to list that as a separate bullet item.
It's an expected part of the custom-subscript capability, I'd think.

regards, tom lane

Re: allow specifying direct role membership in pg_hba.conf

[Tom Lane]

Stephen Frost  writes:
> * Chapman Flack (c...@anastigmatix.net) wrote:
>> If pg_hba syntax changes are being entertained, I would love to be able
>> to set ssl_min_protocol_version locally in a hostssl rule.
>> Some clients at $work are stuck with ancient SSL libraries, but I would
>> much rather be able to weaken ssl_min_protocol_version just for them
>> than do it globally.

> This (unlike what was actually proposed) does seem like it'd be a useful
> improvement.  Not sure exaclty how it would work but I'm generally on
> board with the idea.

Seems like putting GUCs directly into pg_hba would be a mess.  Would
it be enough to tell people to use ALTER ROLE/DATABASE SET for this,
and then fix things so that we recheck the protocol version (and
possibly bail out) after absorbing those settings?

I can think of objections to this:

* If you actually want to tie the restriction to source IP addresses,
rather than users or databases, this doesn't get the job done.

* The authentication cycle would be completed (or at least mostly
so) before we bail out; so if the concern is about packet-sniffing
or MITM attacks, maybe this would expose too much.

But it does have the advantage of being something it seems like
we could get done easily.

regards, tom lane

Re: allow specifying direct role membership in pg_hba.conf

[Stephen Frost]

Greetings,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Andrew Dunstan  writes:
> > On 5/13/21 7:38 PM, Bossart, Nathan wrote:
> >> I've attached a small patch that allows specifying only direct members
> >> of a group in pg_hba.conf.
> 
> > Do we really want to be creating two classes of role membership?
> 
> Yeah, this seems to be going against the clear meaning of the
> SQL spec.  I realize you can argue that pg_hba.conf doesn't have
> to follow the spec, but it doesn't seem like a terribly good idea
> to interpret role membership differently in different places.

Agreed.

The lack of any particular justifcation for wanting this isn't a useful
way to propose a patch either.

Thanks,

Stephen


signature.asc
Description: PGP signature

Re: allow specifying direct role membership in pg_hba.conf

[Stephen Frost]

Greetings,

* Chapman Flack (c...@anastigmatix.net) wrote:
> If pg_hba syntax changes are being entertained, I would love to be able
> to set ssl_min_protocol_version locally in a hostssl rule.
> 
> Some clients at $work are stuck with ancient SSL libraries, but I would
> much rather be able to weaken ssl_min_protocol_version just for them
> than do it globally.

This (unlike what was actually proposed) does seem like it'd be a useful
improvement.  Not sure exaclty how it would work but I'm generally on
board with the idea.

Thanks,

Stephen


signature.asc
Description: PGP signature

Re: compute_query_id and pg_stat_statements

[Pavel Stehule]

pá 14. 5. 2021 v 19:28 odesílatel Bruce Momjian  napsal:

> On Fri, May 14, 2021 at 12:21:23PM -0400, Bruce Momjian wrote:
> > On Fri, May 14, 2021 at 12:04:05PM -0400, Andrew Dunstan wrote:
> > > > On May 14, 2021, at 8:35 AM, Bruce Momjian  wrote:
> > > >
> > > > On Thu, May 13, 2021 at 09:41:42PM -0400, Bruce Momjian wrote:
> > > > I think keeping the output as 'auto', and documenting that this query
> > > > must be run to determine if the query id is being computed:
> > > >
> > > >SELECT query_id
> > > >FROM pg_stat_activity
> > > >WHERE pid = pg_backend_pid();
> > > >
> > > > is the right approach.
> > >
> > > I’d rather we added a specific function. This is not really obvious.
> >
> > Well, we can document this query, add a function, or add a read-only
> > GUC.  I am not sure how we decide which one to use.
>
> I wonder if we should go with an SQL query now (no new API needed) and
> then add a GUC once we decide on how extensions can register that they
> are generating the query id, so the GUC can report the generating
> source, not just a boolean.  The core server can also register as the
> source.
>

I have no problem with it. This is an internal feature and can be enhanced
(fixed) in time without problems.

Pavel



> --
>   Bruce Momjian  https://momjian.us
>   EDB  https://enterprisedb.com
>
>   If only the physical world exists, free will is an illusion.
>
>

Re: alter subscription drop publication fixes

[vignesh C]

On Fri, May 14, 2021 at 8:56 PM Tom Lane  wrote:
>
> Bharath Rupireddy  writes:
> > On Fri, May 14, 2021 at 7:58 PM vignesh C  wrote:
> >> I have changed it to:
> >> ADD adds additional publications,
> >> -  DROP removes publications from the list of
> >> +  DROP removes publications to/from the list of
>
> > How about "Publications are added to or dropped from the existing list
> > of publications by ADD  or DROP
> > respectively." ?
>
> We generally prefer to use the active voice, so I don't think
> restructuring the sentence that way is an improvement.  The quoted
> bit would be better left alone entirely.  Or maybe split it into
> two sentences, but keep the active voice.

I felt changing it to the below was better:
SET replaces the entire list of publications with a new list, ADD adds
additional publications to the list of publications and DROP removes
the publications from the list of publications.

Attached patch has the change for the same.
Thoughts?

Regards,
Vignesh
From 4f887b0b3f338f55d186fa059cfdc255b9783263 Mon Sep 17 00:00:00 2001
From: vignesh 
Date: Fri, 14 May 2021 19:30:40 +0530
Subject: [PATCH v4] Fixes in alter subscription drop publication.

Fixes in alter subscription drop publication.
---
 doc/src/sgml/ref/alter_subscription.sgml   | 19 ++-
 src/backend/commands/subscriptioncmds.c|  6 +++---
 src/bin/psql/tab-complete.c|  9 ++---
 src/test/regress/expected/subscription.out |  2 +-
 4 files changed, 20 insertions(+), 16 deletions(-)

diff --git a/doc/src/sgml/ref/alter_subscription.sgml b/doc/src/sgml/ref/alter_subscription.sgml
index 367ac814f4..6f5a4268e5 100644
--- a/doc/src/sgml/ref/alter_subscription.sgml
+++ b/doc/src/sgml/ref/alter_subscription.sgml
@@ -22,9 +22,9 @@ PostgreSQL documentation
  
 
 ALTER SUBSCRIPTION name CONNECTION 'conninfo'
-ALTER SUBSCRIPTION name SET PUBLICATION publication_name [, ...] [ WITH ( set_publication_option [= value] [, ... ] ) ]
-ALTER SUBSCRIPTION name ADD PUBLICATION publication_name [, ...] [ WITH ( set_publication_option [= value] [, ... ] ) ]
-ALTER SUBSCRIPTION name DROP PUBLICATION publication_name [, ...] [ WITH ( set_publication_option [= value] [, ... ] ) ]
+ALTER SUBSCRIPTION name SET PUBLICATION publication_name [, ...] [ WITH ( publication_option [= value] [, ... ] ) ]
+ALTER SUBSCRIPTION name ADD PUBLICATION publication_name [, ...] [ WITH ( publication_option [= value] [, ... ] ) ]
+ALTER SUBSCRIPTION name DROP PUBLICATION publication_name [, ...] [ WITH ( publication_option [= value] [, ... ] ) ]
 ALTER SUBSCRIPTION name REFRESH PUBLICATION [ WITH ( refresh_option [= value] [, ... ] ) ]
 ALTER SUBSCRIPTION name ENABLE
 ALTER SUBSCRIPTION name DISABLE
@@ -102,17 +102,17 @@ ALTER SUBSCRIPTION name RENAME TO <
  
   Changes the list of subscribed publications.  SET
   replaces the entire list of publications with a new list,
-  ADD adds additional publications,
-  DROP removes publications from the list of
-  publications.  See  for more
-  information.  By default, this command will also act like
+  ADD adds additional publications to the list of
+  publications and DROP removes the publications from
+  the list of publications.  See 
+  for more information.  By default, this command will also act like
   REFRESH PUBLICATION, except that in case of
   ADD or DROP, only the added or
   dropped publications are refreshed.
  
 
  
-  set_publication_option specifies additional
+  publication_option specifies additional
   options for this operation.  The supported options are:
 
   
@@ -129,7 +129,8 @@ ALTER SUBSCRIPTION name RENAME TO <
   
 
   Additionally, refresh options as described
-  under REFRESH PUBLICATION may be specified.
+  under REFRESH PUBLICATION may be specified, except for
+  DROP PUBLICATION.
  
 

diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 8aa6de1785..e93fee6b99 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -951,8 +951,6 @@ AlterSubscription(AlterSubscriptionStmt *stmt, bool isTopLevel)
 bool		refresh;
 List	   *publist;
 
-publist = merge_publications(sub->publications, stmt->publication, isadd, stmt->subname);
-
 parse_subscription_options(stmt->options,
 		   NULL,	/* no "connect" */
 		   NULL, NULL,	/* no "enabled" */
@@ -965,6 +963,8 @@ AlterSubscription(AlterSubscriptionStmt *stmt, bool isTopLevel)
 		   NULL, NULL,	/* no "binary" */
 		   NULL, NULL); /* no "streaming" */
 
+publist = merge_publications(sub->publications, stmt->publication, isadd, stmt->subname);
+
 values[Anum_pg_subscription_subpublications - 1] =
 	publicationListToArray(publist);
 replaces[Anum_pg_subscription_subpublications - 1] = true;
@@ -1671,7 +1671,7 @@ merge_publications(List *oldpublist, List 

Re: compute_query_id and pg_stat_statements

[Bruce Momjian]

On Fri, May 14, 2021 at 12:21:23PM -0400, Bruce Momjian wrote:
> On Fri, May 14, 2021 at 12:04:05PM -0400, Andrew Dunstan wrote:
> > > On May 14, 2021, at 8:35 AM, Bruce Momjian  wrote:
> > > 
> > > On Thu, May 13, 2021 at 09:41:42PM -0400, Bruce Momjian wrote:
> > > I think keeping the output as 'auto', and documenting that this query
> > > must be run to determine if the query id is being computed:
> > > 
> > >SELECT query_id
> > >FROM pg_stat_activity
> > >WHERE pid = pg_backend_pid();
> > > 
> > > is the right approach.
> > 
> > I’d rather we added a specific function. This is not really obvious.
> 
> Well, we can document this query, add a function, or add a read-only
> GUC.  I am not sure how we decide which one to use.

I wonder if we should go with an SQL query now (no new API needed) and
then add a GUC once we decide on how extensions can register that they
are generating the query id, so the GUC can report the generating
source, not just a boolean.  The core server can also register as the
source.

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

  If only the physical world exists, free will is an illusion.

Re: pgsql: autovacuum: handle analyze for partitioned tables

[Alvaro Herrera]

Thanks for these corrections -- I applied them and a few minor changes
from myself, and pushed.  Another set of eyes over the result would be
most welcome.

I hope we can close this now :-)

-- 
Álvaro Herrera39°49'30"S 73°17'W
"Those who use electric razors are infidels destined to burn in hell while
we drink from rivers of beer, download free vids and mingle with naked
well shaved babes." (http://slashdot.org/comments.pl?sid=44793=4647152)

Re: PG 14 release notes, first draft

[Bruce Momjian]

On Thu, May 13, 2021 at 09:01:41PM -0500, Justin Pryzby wrote:
> On Mon, May 10, 2021 at 09:40:45AM -0500, Justin Pryzby wrote:
> > Should any of these be included?
> 
> New SQL-accessible functionality should be included:
> > 8c15a29745 Allow ALTER TYPE to update an existing type's typsubscript value.

OK, text is:





Allow ALTER TYPE to specify or remove a SUBSCRIPT handler
(Tom Lane)



> I'm undecided on this one:
> > 7db0cd2145 Set PD_ALL_VISIBLE and visibility map bits in COPY FREEZE
> 
> People who didn't used to use FREEZE (because it didn't work or otherwise)
> might want to start using it.

Yes, that seems very important:





Have COPY FREEZE appropriately update page visibility bits
(Anastasia Lubennikova, Pavan Deolasee, Jeff Janes)



This one one of those, "I must be confused, since we certainly already
did this before."

> I'm withdrawing these, as modifications to existing log messages don't need to
> be included:
> 
> > 10a5b35a00 Report resource usage at the end of recovery
> > 7e453634bb Add additional information in the vacuum error context.
> > 1ea396362b Improve logging of bad parameter values in BIND messages.

OK.

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

  If only the physical world exists, free will is an illusion.

Re: Parallel scan with SubTransGetTopmostTransaction assert coredump

[Greg Nancarrow]

On Fri, May 14, 2021 at 8:36 PM Pengchengliu  wrote:
>   Did you use pgbench with the script sub_120.sql which I provide in 
> attachment?

yes

>
>   Did you increase the number PGPROC_MAX_CACHED_SUBXIDS? Please don't change 
> any codes, now we just use the origin codes in PG13.2.
>

No, I have made no source code changes at all.
That was my suggestion, for you to try - because if the problem is
avoided by increasing PGPROC_MAX_CACHED_SUBXIDS (to say 128) then it
probably indicates the overflow condition is affecting the xmin.xmax
of the two snapshots such that it invalidates the condition that is
asserted.


I think one problem is that in your settings, you haven't set
"max_worker_processes", yet have set "max_parallel_workers = 128".
I'm finding no more than 8 parallel workers are actually active at any one time.
On top of this, you've got pgbench running with 200 concurrent clients.
So many queries are actually executing parallel plans without using
parallel workers, as the workers can't actually be launched (and this
is probably why I'm finding it hard to reproduce the issue, if the
problem involves snapshot suboverflow and parallel workers).
I find that the following settings improve the parallelism per query
and the whole test runs very much faster:

max_connections = 2000
parallel_setup_cost=0
parallel_tuple_cost=0
min_parallel_table_scan_size=0
max_parallel_workers_per_gather=4
max_parallel_workers = 100
max_worker_processes = 128

and adjust the pgbench command-line:   pgbench  -d postgres -p 33550
-n -r -f sub_120.sql   -c 25 -j 25 -T 1800

Problem is, I still get no coredump when using this.
Can you try these settings and let me know if the crash still happens
if you use these settings?

I also tried:

max_connections = 2000
parallel_setup_cost=0
parallel_tuple_cost=0
min_parallel_table_scan_size=0
max_parallel_workers_per_gather=2
max_parallel_workers = 280
max_worker_processes = 300

and the pgbench command-line:   pgbench  -d postgres -p 33550  -n -r
-f sub_120.sql   -c 140 -j 140 -T 1800

- but I still get no coredump.

Regards,
Greg Nancarrow
Fujitsu Australia

Re: compute_query_id and pg_stat_statements

[Pavel Stehule]

pá 14. 5. 2021 v 18:21 odesílatel Bruce Momjian  napsal:

> On Fri, May 14, 2021 at 12:04:05PM -0400, Andrew Dunstan wrote:
> > > On May 14, 2021, at 8:35 AM, Bruce Momjian  wrote:
> > >
> > > On Thu, May 13, 2021 at 09:41:42PM -0400, Bruce Momjian wrote:
> > > I think keeping the output as 'auto', and documenting that this query
> > > must be run to determine if the query id is being computed:
> > >
> > >SELECT query_id
> > >FROM pg_stat_activity
> > >WHERE pid = pg_backend_pid();
> > >
> > > is the right approach.
> >
> > I’d rather we added a specific function. This is not really obvious.
>
> Well, we can document this query, add a function, or add a read-only
> GUC.  I am not sure how we decide which one to use.
>

I though and I prefer read only GUC

It is easy to write "show all"

Pavel



> --
>   Bruce Momjian  https://momjian.us
>   EDB  https://enterprisedb.com
>
>   If only the physical world exists, free will is an illusion.
>
>

Re: compute_query_id and pg_stat_statements

[Bruce Momjian]

On Fri, May 14, 2021 at 12:04:05PM -0400, Andrew Dunstan wrote:
> > On May 14, 2021, at 8:35 AM, Bruce Momjian  wrote:
> > 
> > On Thu, May 13, 2021 at 09:41:42PM -0400, Bruce Momjian wrote:
> > I think keeping the output as 'auto', and documenting that this query
> > must be run to determine if the query id is being computed:
> > 
> >SELECT query_id
> >FROM pg_stat_activity
> >WHERE pid = pg_backend_pid();
> > 
> > is the right approach.
> 
> I’d rather we added a specific function. This is not really obvious.

Well, we can document this query, add a function, or add a read-only
GUC.  I am not sure how we decide which one to use.

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

  If only the physical world exists, free will is an illusion.

Re: compute_query_id and pg_stat_statements

[Julien Rouhaud]

On Fri, May 14, 2021 at 12:04:05PM -0400, Andrew Dunstan wrote:
> 
> > On May 14, 2021, at 8:35 AM, Bruce Momjian  wrote:
> > 
> > On Thu, May 13, 2021 at 09:41:42PM -0400, Bruce Momjian wrote:
> >>> On Fri, May 14, 2021 at 09:40:15AM +0800, Julien Rouhaud wrote:
> >>> On Fri, May 14, 2021 at 8:13 AM Bruce Momjian  wrote:
>  
>  On Thu, May 13, 2021 at 08:04:37PM -0400, Álvaro Herrera wrote:
> > Here's a first attempt at what was suggested.  If you say "auto" it
> > remains auto in SHOW, but it gets enabled if a module asks for it.
> > 
> > Not final yet, but I thought I'd throw it out for early commentary ...
>  
>  I certainly like this idea better than having the extension change the
>  output of the GUC.
> >>> 
> >>> Oh, I didn't understand that it was the major blocker.  I'm fine with it 
> >>> too.
> >> 
> >> I think if we keep the output as 'auto', and document that you check
> >> pg_stat_activity for a hash to see if it is enabled, that gets us pretty
> >> far.
> > 
> > I think keeping the output as 'auto', and documenting that this query
> > must be run to determine if the query id is being computed:
> > 
> >SELECT query_id
> >FROM pg_stat_activity
> >WHERE pid = pg_backend_pid();
> > 
> > is the right approach.
> 
> I’d rather we added a specific function. This is not really obvious.

We could, but I don't know how much this will be used in practice.  The only
way someone would try to know if "auto" means that query_id are computed is if
she has an extension like pg_stat_statements, and she will probably just check
that anyway, and will get a warning if query_id are *not* computed.

That being said no objection to an SQL wrapper around a query like it.

Re: compute_query_id and pg_stat_statements

[Andrew Dunstan]

Sent from my iPhone

> On May 14, 2021, at 8:35 AM, Bruce Momjian  wrote:
> 
> On Thu, May 13, 2021 at 09:41:42PM -0400, Bruce Momjian wrote:
>>> On Fri, May 14, 2021 at 09:40:15AM +0800, Julien Rouhaud wrote:
>>> On Fri, May 14, 2021 at 8:13 AM Bruce Momjian  wrote:
 
 On Thu, May 13, 2021 at 08:04:37PM -0400, Álvaro Herrera wrote:
> Here's a first attempt at what was suggested.  If you say "auto" it
> remains auto in SHOW, but it gets enabled if a module asks for it.
> 
> Not final yet, but I thought I'd throw it out for early commentary ...
 
 I certainly like this idea better than having the extension change the
 output of the GUC.
>>> 
>>> Oh, I didn't understand that it was the major blocker.  I'm fine with it 
>>> too.
>> 
>> I think if we keep the output as 'auto', and document that you check
>> pg_stat_activity for a hash to see if it is enabled, that gets us pretty
>> far.
> 
> I think keeping the output as 'auto', and documenting that this query
> must be run to determine if the query id is being computed:
> 
>SELECT query_id
>FROM pg_stat_activity
>WHERE pid = pg_backend_pid();
> 
> is the right approach.

I’d rather we added a specific function. This is not really obvious.

Cheers

Andrew

Re: allow specifying direct role membership in pg_hba.conf

[Chapman Flack]

On 05/13/21 19:38, Bossart, Nathan wrote:
> I chose "&" as a new group name prefix for this purpose.  This choice

If pg_hba syntax changes are being entertained, I would love to be able
to set ssl_min_protocol_version locally in a hostssl rule.

Some clients at $work are stuck with ancient SSL libraries, but I would
much rather be able to weaken ssl_min_protocol_version just for them
than do it globally.

Regards,
-Chap

Re: alter subscription drop publication fixes

[Tom Lane]

Bharath Rupireddy  writes:
> On Fri, May 14, 2021 at 7:58 PM vignesh C  wrote:
>> I have changed it to:
>> ADD adds additional publications,
>> -  DROP removes publications from the list of
>> +  DROP removes publications to/from the list of

> How about "Publications are added to or dropped from the existing list
> of publications by ADD  or DROP
> respectively." ?

We generally prefer to use the active voice, so I don't think
restructuring the sentence that way is an improvement.  The quoted
bit would be better left alone entirely.  Or maybe split it into
two sentences, but keep the active voice.

regards, tom lane

Re: allow specifying direct role membership in pg_hba.conf

[Tom Lane]

Andrew Dunstan  writes:
> On 5/13/21 7:38 PM, Bossart, Nathan wrote:
>> I've attached a small patch that allows specifying only direct members
>> of a group in pg_hba.conf.

> Do we really want to be creating two classes of role membership?

Yeah, this seems to be going against the clear meaning of the
SQL spec.  I realize you can argue that pg_hba.conf doesn't have
to follow the spec, but it doesn't seem like a terribly good idea
to interpret role membership differently in different places.

regards, tom lane

Re: alter subscription drop publication fixes

[Bharath Rupireddy]

On Fri, May 14, 2021 at 7:58 PM vignesh C  wrote:
> I have changed it to:
>ADD adds additional publications,
> -  DROP removes publications from the list of
> +  DROP removes publications to/from the list of

How about "Publications are added to or dropped from the existing list
of publications by ADD  or DROP
respectively." ?

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

Re: PG 14 release notes, first draft

[Bruce Momjian]

On Thu, May 13, 2021 at 09:08:37PM -0500, Justin Pryzby wrote:
> On Thu, May 13, 2021 at 09:01:41PM -0500, Justin Pryzby wrote:
> > These should be merged:
> > > 756ab29124 Allow pageinspect to inspect GiST indexes (Andrey Borodin, 
> > > Heikki Linnakangas)
> > > 9e596b65f4 Add "LP_DEAD item?" column to GiST pageinspect functions
> 
> Sorry, this was my error while reconciling my list with yours.
> Your notes only have one item for these, which is correct.
> 
> Possibly you'd want to include the 9e59 commit in the comment (but it wouldn't
> have avoided my own confusion, tough).

OK, I added that commit:





Allow pageinspect to inspect GiST indexes (Andrey Borodin,
Heikki Linnakangas)



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

  If only the physical world exists, free will is an illusion.

Re: alter subscription drop publication fixes

[vignesh C]

On Fri, May 14, 2021 at 7:41 AM Japin Li  wrote:
>
>
> On Thu, 13 May 2021 at 22:13, vignesh C  wrote:
> > On Wed, May 12, 2021 at 10:15 PM Bharath Rupireddy
> >  wrote:
> >>
> >> On Wed, May 12, 2021 at 9:55 PM vignesh C  wrote:
> >> > While I was reviewing one of the logical decoding features, I found a
> >> > few issues in alter subscription drop publication.
> >>
> >> Thanks!
> >>
> >> > Alter subscription drop publication does not support copy_data
option,
> >> > that needs to be removed from tab completion.
> >>
> >> +1. You may want to also change set_publication_option(to something
> >> like drop_pulication_option with only refresh option) for the drop in
> >> the docs? Because "Additionally, refresh options as described under
> >> REFRESH PUBLICATION may be specified." doesn't make sense.
> >>
> >> > Dropping all the publications present in the subscription using alter
> >> > subscription drop publication would throw "subscription must contain
> >> > at least one publication". This message was slightly confusing to me.
> >> > As even though some publication was present on the subscription I was
> >> > not able to drop. Instead I feel we could throw an error message
> >> > something like "dropping specified publication will result in
> >> > subscription without any publication, this is not supported".
> >>
> >> -1 for that long message. The intention of that error was to throw an
> >> error if all the publications of a subscription are dropped. If that's
> >> so confusing, then you could just let the error message be
> >> "subscription must contain at least one publication", add an error
> >> detail "Subscription without any publication is not allowed to
> >> exist/is not supported." or "Removing/Dropping all the publications
> >> from a subscription is not allowed/supported." or some other better
> >> wording.
> >>
> >
> > Modified the error message to "errmsg("cannot drop all the
> > publications of the subscriber \"%s\"", subname)".
> > I have separated the Drop publication documentation contents. There
> > are some duplicate contents but the readability is slightly better.
> > Thoughts?
> >
> >> > merge_publications can be called after validation of the options
> >> > specified, I think we should check if the options specified are
> >> > correct or not before checking the actual publications.
> >>
> >> +1. That was a miss in the original feature.
> >
> > Attached patch has the changes for the same.
> >
>
> Thanks for updating the patch. I have a little comments for the new patch.
>
> -  ADD adds additional publications,
> -  DROP removes publications from the list of
> +  ADD adds additional publications from the list
of
>
> I think, we should change the word 'from' to 'to'.

I have changed it to:
   ADD adds additional publications,
-  DROP removes publications from the list of
+  DROP removes publications to/from the list of

The changes for the same are shared in v3 patch at [1].
[1] -
https://www.postgresql.org/message-id/CALDaNm3svMg%2BhMA9GsJsUQ75HXtpjpAh2gk%3D8yZfgAnA9BMsnA%40mail.gmail.com

Regards,
Vignesh

Re: compute_query_id and pg_stat_statements

[Julien Rouhaud]

On Fri, May 14, 2021 at 08:57:41AM -0400, Bruce Momjian wrote:
> On Fri, May 14, 2021 at 08:35:14AM -0400, Bruce Momjian wrote:
> > On Thu, May 13, 2021 at 09:41:42PM -0400, Bruce Momjian wrote:
> > > I think if we keep the output as 'auto', and document that you check
> > > pg_stat_activity for a hash to see if it is enabled, that gets us pretty
> > > far.
> > 
> > I think keeping the output as 'auto', and documenting that this query
> > must be run to determine if the query id is being computed:
> > 
> > SELECT query_id
> > FROM pg_stat_activity
> > WHERE pid = pg_backend_pid();
> > 
> > is the right approach.
> 
> Actually, we talked about huge_pages = try needing to use OS commands to
> see if huge pages are being used, so requiring an SQL query to see if
> query id is being computed seems reasonable.

I totally agree.

Re: alter subscription drop publication fixes

[vignesh C]

On Thu, May 13, 2021 at 8:13 PM Bharath Rupireddy
 wrote:
>
> On Thu, May 13, 2021 at 7:43 PM vignesh C  wrote:
> > I have separated the Drop publication documentation contents. There
> > are some duplicate contents but the readability is slightly better.
> > Thoughts?
>
> -ALTER SUBSCRIPTION name
> DROP PUBLICATION  class="parameter">publication_name [, ...] [ WITH (
> set_publication_option [=
> value] [, ... ] ) ]
> +ALTER SUBSCRIPTION name
> DROP PUBLICATION  class="parameter">publication_name [, ...] [ WITH (
> refresh [= value] ) ]
>
> IMO, let's not list the "refresh" option directly here. If we don't
> want to add a new list of operations "drop_publication_opition", you
> could just mention a note "Except for DROP PUBLICATION, the refresh
> options as described under REFRESH PUBLICATION may be specified." or
> "Additionally, refresh options as described under REFRESH PUBLICATION
> may be specified, except for DROP PUBLICATION."

Thanks for the comment, the attached v3 patch has the changes for the
same. I also made another change to change set_publication_option to
publication_option as it is common for SET/ADD & DROP.

Regards,
Vignesh
From 3b477122f909d5e1df35c32a1775665bc92ec86b Mon Sep 17 00:00:00 2001
From: vignesh 
Date: Fri, 14 May 2021 19:30:40 +0530
Subject: [PATCH v3] Fixes in alter subscription drop publication.

Fixes in alter subscription drop publication.
---
 doc/src/sgml/ref/alter_subscription.sgml   | 13 +++--
 src/backend/commands/subscriptioncmds.c|  6 +++---
 src/bin/psql/tab-complete.c|  9 ++---
 src/test/regress/expected/subscription.out |  2 +-
 4 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/doc/src/sgml/ref/alter_subscription.sgml b/doc/src/sgml/ref/alter_subscription.sgml
index 367ac814f4..faf785760c 100644
--- a/doc/src/sgml/ref/alter_subscription.sgml
+++ b/doc/src/sgml/ref/alter_subscription.sgml
@@ -22,9 +22,9 @@ PostgreSQL documentation
  
 
 ALTER SUBSCRIPTION name CONNECTION 'conninfo'
-ALTER SUBSCRIPTION name SET PUBLICATION publication_name [, ...] [ WITH ( set_publication_option [= value] [, ... ] ) ]
-ALTER SUBSCRIPTION name ADD PUBLICATION publication_name [, ...] [ WITH ( set_publication_option [= value] [, ... ] ) ]
-ALTER SUBSCRIPTION name DROP PUBLICATION publication_name [, ...] [ WITH ( set_publication_option [= value] [, ... ] ) ]
+ALTER SUBSCRIPTION name SET PUBLICATION publication_name [, ...] [ WITH ( publication_option [= value] [, ... ] ) ]
+ALTER SUBSCRIPTION name ADD PUBLICATION publication_name [, ...] [ WITH ( publication_option [= value] [, ... ] ) ]
+ALTER SUBSCRIPTION name DROP PUBLICATION publication_name [, ...] [ WITH ( publication_option [= value] [, ... ] ) ]
 ALTER SUBSCRIPTION name REFRESH PUBLICATION [ WITH ( refresh_option [= value] [, ... ] ) ]
 ALTER SUBSCRIPTION name ENABLE
 ALTER SUBSCRIPTION name DISABLE
@@ -103,7 +103,7 @@ ALTER SUBSCRIPTION name RENAME TO <
   Changes the list of subscribed publications.  SET
   replaces the entire list of publications with a new list,
   ADD adds additional publications,
-  DROP removes publications from the list of
+  DROP removes publications to/from the list of
   publications.  See  for more
   information.  By default, this command will also act like
   REFRESH PUBLICATION, except that in case of
@@ -112,7 +112,7 @@ ALTER SUBSCRIPTION name RENAME TO <
  
 
  
-  set_publication_option specifies additional
+  publication_option specifies additional
   options for this operation.  The supported options are:
 
   
@@ -129,7 +129,8 @@ ALTER SUBSCRIPTION name RENAME TO <
   
 
   Additionally, refresh options as described
-  under REFRESH PUBLICATION may be specified.
+  under REFRESH PUBLICATION may be specified, except for
+  DROP PUBLICATION.
  
 

diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 8aa6de1785..e93fee6b99 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -951,8 +951,6 @@ AlterSubscription(AlterSubscriptionStmt *stmt, bool isTopLevel)
 bool		refresh;
 List	   *publist;
 
-publist = merge_publications(sub->publications, stmt->publication, isadd, stmt->subname);
-
 parse_subscription_options(stmt->options,
 		   NULL,	/* no "connect" */
 		   NULL, NULL,	/* no "enabled" */
@@ -965,6 +963,8 @@ AlterSubscription(AlterSubscriptionStmt *stmt, bool isTopLevel)
 		   NULL, NULL,	/* no "binary" */
 		   NULL, NULL); /* no "streaming" */
 
+publist = merge_publications(sub->publications, stmt->publication, isadd, stmt->subname);
+
 values[Anum_pg_subscription_subpublications - 1] =
 	publicationListToArray(publist);
 replaces[Anum_pg_subscription_subpublications - 1] = true;
@@ -1671,7 +1671,7 @@ merge_publications(List *oldpublist, List *newpublist, bool addpub, const char *
 	if 

Re: Bracket, brace, parenthesis

[Tom Lane]

Kyotaro Horiguchi  writes:
> I'm not sure how much we (or people) are strcit on the distinction
> between the $SUBJECT, isn't '{' a brace generally?

+1.  I tend to write "square bracket" or "curly brace" when I want to
be extra clear, but I think the bare terms are widely understood to
have those meanings.

regards, tom lane

Re: OOM in spgist insert

[Tom Lane]

Robert Haas  writes:
> On Thu, May 13, 2021 at 6:20 PM Tom Lane  wrote:
>> OTOH, the 10-cycles-to-show-progress rule could be
>> argued to be an API break.

> Not being familiar with this code, I don't really understand why 10
> cycles to show progress wouldn't, like 640kB, be enough for anyone.

Yeah, after further thought I'm thinking that that ought to be plenty.
In released branches, that code will never execute at all unless the
datum-to-be-indexed is larger than ~8kB.  If you are starting with,
say, a 30kB string, you'd better be removing a heck of a lot more than
one byte per tree level, or your search performance is going to be
abysmal.  Maybe algorithmic oddities would sometimes result in
seemingly making no progress for one cycle, but I doubt there's need
for more slop than that.  In this light, a 10-cycle grace period seems
if anything excessive.

> But as far as back-patching the code goals, I think the question is
> not so much whether this restriction could hypothetically break
> anything but whether it will actually break anything, which leads to
> the question of how many spgist opclasses we think exist outside of
> core.

That is also an interesting viewpoint.  codesearch.debian.net knows
of no external SPGiST opclasses other than PostGIS'.  They don't
seem to have indexed the two you found on github, though.  None of
those four set longValuesOK to true, which means that the whole
discussion is moot for them.  So it's entirely plausible that
spgtextproc.c is the only affected code anywhere.

Of course, that conclusion could lead to the position that there's
no point in back-patching anyway, since there's no reason to think
that spgtextproc.c is buggy.

regards, tom lane

Re: View invoker privileges

[Joe Conway]

On 5/14/21 4:11 AM, Noah Misch wrote:

On Wed, Apr 14, 2021 at 10:25:08AM +0300, Ivan Ivanov wrote:

In Postgres we can create view with view owner privileges only. What’s the
reason that there is no option to create view with invoker privileges? Is
there any technical or security subtleties related to absence of this
feature?


The SQL standard calls for the owner privileges behavior, and nobody has
implemented an invoker privileges option.  I know of no particular subtlety.
An SQL-language function can behave like an invoker-privileges view, but a
view would allow more optimizer freedom.  It would be a good option to have.


+1

Joe

--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

Re: OOM in spgist insert

[Tom Lane]

Pavel Borisov  writes:
> Now when checking for shortening of  leaf tuple is added LongValuesOK
> become slightly redundant. I'd propose to replace it with more legible name
> as LongValuesOK doesn't mean they are warranted to be ok just that we can
> try to shorten them? What about tryShortening, trySuffixing or
> can(Try)ShortenTuple?

That field name is part of the opclass API.  I fear it's several years
too late to rename it for no compelling reason.

regards, tom lane

Re: Added missing tab completion for alter subscription set option

[Bharath Rupireddy]

On Fri, May 14, 2021 at 6:51 PM vignesh C  wrote:
>
> On Fri, May 14, 2021 at 12:25 PM Bharath Rupireddy
>  wrote:
> >
> > On Fri, May 14, 2021 at 12:00 PM vignesh C  wrote:
> > >
> > > Hi,
> > >
> > > While I was reviewing one of the logical decoding features, I found
> > > Streaming and binary options were missing in tab completion for the
> > > alter subscription set option, the attached patch has the changes for
> > > the same.
> > > Thoughts?
> >
> > +1.
> >
> > Without patch:
> > postgres=# alter subscription testsub set (S
> > SLOT_NAME   SYNCHRONOUS_COMMIT
> >
> > With patch:
> > postgres=# alter subscription testsub set (
> > BINARY  SLOT_NAME   STREAMING   
> > SYNCHRONOUS_COMMIT
> >
> > How about ordering the options alphabetically as the tab complete
> > output anyways shows that way? I'm not sure if that's the practice,
> > but just a thought.
>
> I did not see any rule for this, but also did not see any harm in
> keeping it in alphabetical order, so changed it in the attached patch.

Thanks. Just a few nitpicks:
1) How about patch name: "Add tab completion for ALTER SUBSCRIPTION
SET options streaming and binary"?
2) How about a detailed message: "Tab completion for the options
streaming and binary were missing in case of ALTER SUBSCRIPTION SET
command. This patch adds them."?

You may want to add this in commitfest so that we don't lose track of it.

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

Re: Added missing tab completion for alter subscription set option

[vignesh C]

On Fri, May 14, 2021 at 12:25 PM Bharath Rupireddy
 wrote:
>
> On Fri, May 14, 2021 at 12:00 PM vignesh C  wrote:
> >
> > Hi,
> >
> > While I was reviewing one of the logical decoding features, I found
> > Streaming and binary options were missing in tab completion for the
> > alter subscription set option, the attached patch has the changes for
> > the same.
> > Thoughts?
>
> +1.
>
> Without patch:
> postgres=# alter subscription testsub set (S
> SLOT_NAME   SYNCHRONOUS_COMMIT
>
> With patch:
> postgres=# alter subscription testsub set (
> BINARY  SLOT_NAME   STREAMING   SYNCHRONOUS_COMMIT
>
> How about ordering the options alphabetically as the tab complete
> output anyways shows that way? I'm not sure if that's the practice,
> but just a thought.

I did not see any rule for this, but also did not see any harm in
keeping it in alphabetical order, so changed it in the attached patch.

Regards,
Vignesh
From 0d209ae307841827aec5478a4ca3f8e3d945eef3 Mon Sep 17 00:00:00 2001
From: vignesh 
Date: Fri, 14 May 2021 11:37:05 +0530
Subject: [PATCH v2] Added missing tab completion for alter subscription set
 option.

Streaming and binary options were missing in tab completion for alter
subscription set option, included it.
---
 src/bin/psql/tab-complete.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 6598c5369a..1b9661fb24 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -1665,7 +1665,7 @@ psql_completion(const char *text, int start, int end)
 		COMPLETE_WITH("(", "PUBLICATION");
 	/* ALTER SUBSCRIPTION  SET ( */
 	else if (HeadMatches("ALTER", "SUBSCRIPTION", MatchAny) && TailMatches("SET", "("))
-		COMPLETE_WITH("slot_name", "synchronous_commit");
+		COMPLETE_WITH("binary", "slot_name", "streaming", "synchronous_commit");
 	/* ALTER SUBSCRIPTION  SET PUBLICATION */
 	else if (HeadMatches("ALTER", "SUBSCRIPTION", MatchAny) && TailMatches("SET", "PUBLICATION"))
 	{
-- 
2.25.1

Re: compute_query_id and pg_stat_statements

[Bruce Momjian]

On Fri, May 14, 2021 at 08:35:14AM -0400, Bruce Momjian wrote:
> On Thu, May 13, 2021 at 09:41:42PM -0400, Bruce Momjian wrote:
> > I think if we keep the output as 'auto', and document that you check
> > pg_stat_activity for a hash to see if it is enabled, that gets us pretty
> > far.
> 
> I think keeping the output as 'auto', and documenting that this query
> must be run to determine if the query id is being computed:
> 
>   SELECT query_id
>   FROM pg_stat_activity
>   WHERE pid = pg_backend_pid();
> 
> is the right approach.

Actually, we talked about huge_pages = try needing to use OS commands to
see if huge pages are being used, so requiring an SQL query to see if
query id is being computed seems reasonable.

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

  If only the physical world exists, free will is an illusion.

Re: compute_query_id and pg_stat_statements

[Bruce Momjian]

On Thu, May 13, 2021 at 09:41:42PM -0400, Bruce Momjian wrote:
> On Fri, May 14, 2021 at 09:40:15AM +0800, Julien Rouhaud wrote:
> > On Fri, May 14, 2021 at 8:13 AM Bruce Momjian  wrote:
> > >
> > > On Thu, May 13, 2021 at 08:04:37PM -0400, Álvaro Herrera wrote:
> > > > Here's a first attempt at what was suggested.  If you say "auto" it
> > > > remains auto in SHOW, but it gets enabled if a module asks for it.
> > > >
> > > > Not final yet, but I thought I'd throw it out for early commentary ...
> > >
> > > I certainly like this idea better than having the extension change the
> > > output of the GUC.
> > 
> > Oh, I didn't understand that it was the major blocker.  I'm fine with it 
> > too.
> 
> I think if we keep the output as 'auto', and document that you check
> pg_stat_activity for a hash to see if it is enabled, that gets us pretty
> far.

I think keeping the output as 'auto', and documenting that this query
must be run to determine if the query id is being computed:

SELECT query_id
FROM pg_stat_activity
WHERE pid = pg_backend_pid();

is the right approach.

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

  If only the physical world exists, free will is an illusion.

Re: parallel vacuum - few questions on docs, comments and code

[Bharath Rupireddy]

On Fri, May 14, 2021 at 10:43 AM Dilip Kumar  wrote:
>
> On Thu, May 13, 2021 at 9:00 PM Bharath Rupireddy
>  wrote:
> >
> > Done that way.
> >
> > PSA patch.
>
> Your changes look good.  About changing the "non-negative integer" to
> "greater than or equal to zero", there is another thread [1], I am not
> sure that have we concluded anything there yet.
>
> - pg_log_error("parallel vacuum degree must be a non-negative integer");
> + pg_log_error("parallel workers for vacuum must be greater than or
> equal to zero");
>   exit(1);
>
> [1] 
> https://www.postgresql.org/message-id/os0pr01mb5716415335a06b489f1b3a8194...@os0pr01mb5716.jpnprd01.prod.outlook.com

Yeah. Tom proposed if (foo <= 0) { error:"foo must be greater than
zero" } at [1]. In the subsequent messages both Michael and I agreed
with that. But we also have cases like  if (foo < 0) for which I think
{ error:"foo must be greater than or equal to zero" } would be better,
similar to what's proposed. Please feel free to provide your thoughts
there in that thread.

[1] - https://www.postgresql.org/message-id/621822.1620655780%40sss.pgh.pa.us

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

Re: Can a child process detect postmaster death when in pg_usleep?

[Aleksander Alekseev]

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:   tested, passed
Spec compliant:   tested, passed
Documentation:tested, passed

This patch looks fine. Tested on MacOS Catalina; master 09ae3299

The new status of this patch is: Ready for Committer

Re: OOM in spgist insert

[Robert Haas]

On Thu, May 13, 2021 at 6:20 PM Tom Lane  wrote:
> OTOH, the 10-cycles-to-show-progress rule could be
> argued to be an API break.

Not being familiar with this code, I don't really understand why 10
cycles to show progress wouldn't, like 640kB, be enough for anyone.
But as far as back-patching the code goals, I think the question is
not so much whether this restriction could hypothetically break
anything but whether it will actually break anything, which leads to
the question of how many spgist opclasses we think exist outside of
core. I did a Google search and found some evidence that PostGIS might
have such things, and also this:

https://github.com/fake-name/pg-spgist_hamming

There might be other things, but I did not find them.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: allow specifying direct role membership in pg_hba.conf

[Andrew Dunstan]

On 5/13/21 7:38 PM, Bossart, Nathan wrote:
> Hi hackers,
>
> I've attached a small patch that allows specifying only direct members
> of a group in pg_hba.conf.  The "+" prefix offered today matches both
> direct and indirect role members, which may complicate some role
> setups.  For example, if you have one set of roles that are members of
> the "pam" role and another set that are members of the "scram-sha-256"
> role, granting membership in a PAM role to a SCRAM role might
> inadvertently modify the desired authentication method for the
> grantee.  If only direct membership is considered, no such inadvertent
> authentication method change would occur.
>
> I chose "&" as a new group name prefix for this purpose.  This choice
> seemed as good as any, but I'm open to changing it if anyone has
> suggestions.  For determining direct role membership, I added a new
> function in acl.c that matches other related functions.  I added a new
> role cache type since it seemed to fit in reasonably well, but it seems
> unlikely that there is any real performance benefit versus simply
> open-coding the syscache lookup.
>
> I didn't see any existing authentication tests for groups at first
> glance.  If folks are interested in this functionality, I can work on
> adding some tests for this stuff.
>

Do we really want to be creating two classes of role membership?


cheers


andrew

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

Re: compute_query_id and pg_stat_statements

[Bruce Momjian]

On Fri, May 14, 2021 at 12:26:23AM -0400, Tom Lane wrote:
> I then tried a really dumb xor'ing implementation, and
> that gives me a slowdown of 2.2%.  This could undoubtedly
> be improved on further, say by unrolling the loop or
> processing multiple bytes at once.  One problem with it
> is that I suspect it will tend to concentrate the entropy
> into the third/fourth and seventh/eighth bytes of the
> accumulator, since so many of the fields being jumbled
> are 4-byte or 8-byte fields with most of the entropy in
> their low-order bits.  Probably that could be improved
> with a bit more thought -- say, an extra bump of the
> nextbyte pointer after each field.
> 
> Anyway, I think that what we have here is quite an inferior
> implementation, and we can do better with some more thought.

Considering what even a simple query has to do, I am still baffled why
such a computation takes ~2%, though it obviously does since you have
confirmed that figure.

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

  If only the physical world exists, free will is an illusion.

Re: naming of async_mode parameter

[Zhihong Yu]

On Fri, May 14, 2021 at 1:05 AM Etsuro Fujita 
wrote:

> Hi,
>
> On Thu, May 13, 2021 at 2:23 AM Zhihong Yu  wrote:
> > I was looking at
> >   Fix EXPLAIN ANALYZE for async-capable nodes.
>
> Thanks for that!
>
> > which adds the following parameter / field:
> >
> > +   boolasync_mode; /* true if node is in async mode */
> >
> > async_mode implies an enum: {sync, async}
> > Since there are only two values, the data type is bool. I think it
> should be named is_async.
>
> By async_mode, I mean "is in async mode?", as commented above.  I
> thought the naming is_in_async_mode would be a bit long, so I
> shortened it to async_mode.  IIUC, I think another example in our
> codebase would be the hash_spill_mode parameter in the AggState
> struct.  So I think async_mode would be acceptable IMO.
>
> Best regards,
> Etsuro Fujita
>

Hi,
Searching postgres codebase reveals the following (partial) examples:

bool is_varlena
bool is_leaf

I think these are more intuitive.

If you think is_in_async_mode is too long, how about naming the parameter
is_async ?

If you agree, I can send out a patch.

Cheers

Re: Support for VACUUMing Foreign Tables

[Laurenz Albe]

On Thu, 2021-05-13 at 09:44 +0530, Bharath Rupireddy wrote:
> I think it will be useful to allow foreign tables to be VACUUMed if
> the underlying FDW supports, currently VACUUM doesn't support foreign
> tables, see [1]. In case of postgres_fdw, if foreign tables are
> specified in the local VACUUM command, a per-server remote VACUUM
> command can be prepared with the foreign tables that belong to the
> same server and sent to the foreign server. This design is similar to
> TRUNCATE on foreign tables committed as part of 8ff1c946. Although,
> this may not be much useful for FDWs that connect to remote non-MVCC
> databases where the concept of VACUUM may not apply, but for
> postgres_fdw and others it might help.
> 
> I would like to hear opinions from the hackers. If it's not
> possible/feasible to do this, please let me know the reasons. Thanks.

I see no value in this.

First, it wouldn't make sense for anything except postgres_fdw, so
I think it should not be part of the FDW API.  If anything, it should
mean that knowledge about postgres_fdw gets hardwired into VACUUM.

But I don't think that is a smart idea either.  Each database cluster
is in charge of vacuuming its own tables, so the better approach would
be to tune autovacuum on the remote side so that it does the right thing.

Yours,
Laurenz Albe

RE: Parallel scan with SubTransGetTopmostTransaction assert coredump

[Pengchengliu]

Hi Greg,

  

  When you get the different xmin between active snapshot and transaction 
snapshot, maybe there is no coredump. 

  As maybe there is not tupe(xmin) between ActiveSnapshot->xmin and 
TransactionSnapshot->xmin which needs to be scaned in parallel process.

  

  There is no doubt, it is very likely that ActiveSnapshot->xmin precedes 
TransactionSnapshot->xmin.

  

  For this coredump, we must make sure parallel and snapshot overflow. If 
snapshot is not overflow, you cannot get the coredump. 

  As coredump is from parallel scan in MVCC when snapshot is overflow.

  

  Did you use pgbench with the script sub_120.sql which I provide in 
attachment? 

  As the default PGPROC_MAX_CACHED_SUBXIDS is 64. In script sub_120.sql, for 
one transaction, it will use 120 subtransactions which is much larger than 64.

  While getting the snapshot, it must be overflow. I really don't know why your 
snapshot is not overflow.

  Did you increase the number PGPROC_MAX_CACHED_SUBXIDS? Please don't change 
any codes, now we just use the origin codes in PG13.2.

  I have checked the codes in master branch, there is no change about this 
mechanism. This issue should still exist. 

  

  

  Thanks

  Pengcheng

 

-Original Message-
From: Greg Nancarrow  
Sent: 2021年5月14日 16:47
To: Pengchengliu 
Cc: Andres Freund ; PostgreSQL-development 

Subject: Re: Parallel scan with SubTransGetTopmostTransaction assert coredump

 

On Fri, May 14, 2021 at 12:25 PM Pengchengliu < 
 pengcheng...@tju.edu.cn> wrote:

> 

> Hi Greg,

> 

>   Thanks for you replay and test.

> 

> 

> 

>   When main process gets the transaction snapshot in 
> InitializeParallelDSM->GetTransactionSnapshot,  the transaction snapshot xmin 
> is very likely follows active snapshot xmin.

> 

> 

> 

>   Use gdb it is easy to verify it.

> 

>   Create env as blow:

> 

> 

> 

>   1, Use PG13.2(3fb4c75e857adee3da4386e947ba58a75f3e74b7), init a cluster 
> database.

> 

>   2, Append the postgres.conf as below:

> 

> 

> 

>   max_connections = 2000

> 

>   parallel_setup_cost=0

> 

>   parallel_tuple_cost=0

> 

>   min_parallel_table_scan_size=0

> 

>   max_parallel_workers_per_gather=8

> 

>   max_parallel_workers = 128

> 

> 

> 

>   3, Start the cluster database. Use the init_test.sql script in attachment 
> to create some test tables.

> 

>   4, Use the sub_120.sql script in attachment with pgbench to test it.

> 

> 

> 

>   pgbench  -d postgres -p 33550  -n -r -f sub_120.sql   -c 200 -j 200 -T 1800

> 

> 

> 

> 

> 

> 

> 

>   Then you can login the database, and use GDB to verify it.

> 

>   1, First use explain, make sure force Parallel is OK.

> 

> 

> 

>   postgres=# explain (verbose,analyze) select count(*) from contend1;

> 

>  

> QUERY PLAN

> 

> 

> 

> --

> ---

> 

> 

> 

> Finalize Aggregate  (cost=12006.11..12006.12 rows=1 width=8) (actual 

> time=1075.214..1075.449 rows=1 loops=1)

> 

>Output: count(*)

> 

>->  Gather  (cost=12006.08..12006.09 rows=8 width=8) (actual 

> time=1075.198..1075.433 rows=1 loops=1)

> 

>  Output: (PARTIAL count(*))

> 

>  Workers Planned: 8

> 

>  Workers Launched: 0

> 

>  ->  Partial Aggregate  (cost=12006.08..12006.09 rows=1 

> width=8) (actual time=1074.674..1074.676 rows=1 loops=1)

> 

>Output: PARTIAL count(*)

> 

>->  Parallel Seq Scan on public.contend1  

> (cost=0.00..11690.06 rows=126406 width=0) (actual time=0.008..587.454 

> rows=1

> 

> 010200 loops=1)

> 

>  Output: id, val, c2, c3, c4, c5, c6, c7, c8, c9, 

> c10, crt_time

> 

> Planning Time: 0.123 ms

> 

> Execution Time: 1075.588 ms

> 

>  postgres=# select pg_backend_pid();

> 

> pg_backend_pid

> 

> 

> 

> 2182678

> 

> 

> 

> 

> 

>   2, use gdb to debug our backend process. Add the breakpoint in 
> parallel.c:219 and continue.

> 

> 

> 

>   gdb  -q -p 2182678

> 

>   ...

> 

>   (gdb) b parallel.c:219

> 

> Breakpoint 1 at 0x55d085: file 
> /home/liupc/build/build_postgres2/../../devel/postgres2/src/backend/access/transam/parallel.c,
>  line 219.

> 

>   (gdb) c

> 

> Continuing.

> 

> 

> 

>   3, In the psql clinet, we can execute the explain command in step1 again.

> 

>  After we get the breakpoint in gdb, we wait a moment. Then we execute 
> next.

> 

>  Use gdb check active_snapshot and transaction_snapshot, 
> active_snapshot->xmin is 158987 and transaction_snapshot->xmin is 162160.

> 

> When I use gdb test it, sometimes active_snapshot is the same as 
> transaction_snapshot. Then you can try it multiple times, and before execute 
> next, try wait longer time.

> 

> 

> 

>Breakpoint 1, 

Re: dump cannot be restored if schema permissions revoked

[Noah Misch]

On Wed, Apr 07, 2021 at 10:13:30AM -0700, Richard Yen wrote:
> I noticed that in some situations involving the use of REVOKE ON SCHEMA,
> pg_dump
> can produce a dump that cannot be restored.  This prevents successful
> pg_restore (and by corollary, pg_upgrade).
> 
> An example shell script to recreate this problem is attached.  The error
> output appears at the end like this:
> 
> 
> + pg_restore -d postgres /tmp/foo.dmp
> pg_restore: [archiver (db)] Error while PROCESSING TOC:
> pg_restore: [archiver (db)] Error from TOC entry 2748; 0 0 ACL TABLE
> mytable owneruser
> pg_restore: [archiver (db)] could not execute query: ERROR:  permission
> denied for schema private
> Command was: GRANT SELECT ON TABLE private.mytable TO privileged WITH
> GRANT OPTION;
> SET SESSION AUTHORIZATION privileged;
> GRANT SELECT ON TABLE private.mytable TO enduser WITH GRANT OPTION;
> RESET SESSION AUTHORIZATION;
> WARNING: errors ignored on restore: 1
> -bash-4.2$
> 
> 
> Note that `privileged` user needs to grant permissions to `enduser`, but
> can no longer do so because `privileged` no longer has access to the
> `private` schema (it was revoked).
> 
> How might we fix up pg_dump to handle these sorts of situations?

I would approach this by allowing GRANT to take a grantor role name.  Then,
we'd remove the SET SESSION AUTHORIZATION, and the user running the restore
would set the grantor.  "GRANT SELECT ON TABLE foo TO bob GRANTED BY alice;"
looks reasonable to me, though one would need to check if SQL requires that to
have some different behavior.

> It seems
> like pg_dump might need extra logic to GRANT the schema permissions to the
> `privileged` user and then REVOKE them later on?

That could work, but I would avoid it for a couple of reasons.  In some
"pg_restore --use-list" partial restores, the schema privilege may already
exist, and this design may surprise the DBA by removing the existing
privilege.  When running a restore as a non-superuser, the additional
GRANT/REVOKE could be a source of permission denied failures.

RE: Forget close an open relation in ReorderBufferProcessTXN()

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

On Thursday, May 13, 2021 7:21 PM Amit Kapila  wrote:
> On Thu, May 13, 2021 at 11:15 AM osumi.takami...@fujitsu.com
>  wrote:
> >
> > I tried the following scenarios for trying to reproduce this.
> > Scenario2:
> > (1) set up 1 publisher and 1 subscriber
> > (2) create table with user_catalog_table = true on the pub
> > (3) insert some data to this table
> > (4) create publication for the table on the pub
> > (5) create table with user_catalog_table = true on the sub
> > (6) create subscription on the sub
> > (7) add synchronous_standby_names to publisher's configuration and
> > restart the pub
> > (8) have a session to truncate the user_catalog_table on the pub
> >
> > Scenario 2 was successful.
> 
> Yeah, because pgoutput or for that matter even test_decoding doesn't
> acquire a lock on user catalog tables.
> 
> > Are these the scenario you have in mind, if not please let me know for
> > the missing steps.
> > I would like to reproduce the scenario and write a patch to fix this.
> 
> I don't think we can reproduce it with core plugins as they don't lock user
> catalog tables. 
OK. My current understanding about how the deadlock happens is below.

1. TRUNCATE command is performed on user_catalog_table.
2. TRUNCATE command locks the table and index with ACCESS EXCLUSIVE LOCK.
3. TRUNCATE waits for the subscriber's synchronization
when synchronous_standby_names is set.
4. Here, the walsender stops, *if* it tries to acquire a lock on the 
user_catalog_table
because the table where it wants to see is locked by the TRUNCATE 
already. 

If this is right, we need to go back to a little bit higher level discussion,
since whether we should hack any plugin to simulate the deadlock caused by 
user_catalog_table reference
with locking depends on the assumption if the plugin takes a lock on the 
user_catalog_table or not.
In other words, if the plugin does read only access to that type of table with 
no lock
(by RelationIdGetRelation for example ?), the deadlock concern disappears and 
we don't
need to add anything to plugin sides, IIUC.

Here, we haven't gotten any response about whether output plugin takes (should 
take)
the lock on the user_catalog_table. But, I would like to make a consensus
about this point before the implementation.

By the way, Amit-san already mentioned the main reason of this
is that we allow decoding of TRUNCATE operation for user_catalog_table in 
synchronous mode.
The choices are provided by Amit-san already in the past email in [1].
(1) disallow decoding of TRUNCATE operation for user_catalog_tables
(2) disallow decoding of any operation for user_catalog_tables like system 
catalog tables

Yet, I'm not sure if either option solves the deadlock concern completely.
If application takes an ACCESS EXCLUSIVE lock by LOCK command (not by TRUNCATE 
!)
on the user_catalog_table in a transaction, and if the plugin tries to take a 
lock on it,
I think the deadlock happens. Of course, having a consensus that the plugin 
takes no lock at all
would remove this concern, though. 

Like this, I'd like to discuss those two items in question together at first.
* the plugin should take a lock on user_catalog_table or not
* the range of decoding related to user_catalog_table

To me, taking no lock on the user_catalog_table from plugin is fine because
we have historical snapshot mechanism, which doesn't produce deadlock in any 
combination
even when application executes a LOCK command for ACCESS EXCLUSIVE.
In addition, I agree with the idea that we don't decode any operation on 
user_catalog_table
and have better alignment with usual system catalogs.

Thoughts ?

[1] - 
https://www.postgresql.org/message-id/CAA4eK1LP8xTysPEMEBYAZ%3D6KoMWfjyf0gzF-9Bp%3DSgVFvYQDVw%40mail.gmail.com



Best Regards,
Takamichi Osumi

Re: Parallel scan with SubTransGetTopmostTransaction assert coredump

[Greg Nancarrow]

On Fri, May 14, 2021 at 12:25 PM Pengchengliu  wrote:
>
> Hi Greg,
>
>   Thanks for you replay and test.
>
>
>
>   When main process gets the transaction snapshot in 
> InitializeParallelDSM->GetTransactionSnapshot,  the transaction snapshot xmin 
> is very likely follows active snapshot xmin.
>
>
>
>   Use gdb it is easy to verify it.
>
>   Create env as blow:
>
>
>
>   1, Use PG13.2(3fb4c75e857adee3da4386e947ba58a75f3e74b7), init a cluster 
> database.
>
>   2, Append the postgres.conf as below:
>
>
>
>   max_connections = 2000
>
>   parallel_setup_cost=0
>
>   parallel_tuple_cost=0
>
>   min_parallel_table_scan_size=0
>
>   max_parallel_workers_per_gather=8
>
>   max_parallel_workers = 128
>
>
>
>   3, Start the cluster database. Use the init_test.sql script in attachment 
> to create some test tables.
>
>   4, Use the sub_120.sql script in attachment with pgbench to test it.
>
>
>
>   pgbench  -d postgres -p 33550  -n -r -f sub_120.sql   -c 200 -j 200 -T 1800
>
>
>
>
>
>
>
>   Then you can login the database, and use GDB to verify it.
>
>   1, First use explain, make sure force Parallel is OK.
>
>
>
>   postgres=# explain (verbose,analyze) select count(*) from contend1;
>
>  QUERY 
> PLAN
>
>
>
> -
>
> 
>
> Finalize Aggregate  (cost=12006.11..12006.12 rows=1 width=8) (actual 
> time=1075.214..1075.449 rows=1 loops=1)
>
>Output: count(*)
>
>->  Gather  (cost=12006.08..12006.09 rows=8 width=8) (actual 
> time=1075.198..1075.433 rows=1 loops=1)
>
>  Output: (PARTIAL count(*))
>
>  Workers Planned: 8
>
>  Workers Launched: 0
>
>  ->  Partial Aggregate  (cost=12006.08..12006.09 rows=1 width=8) 
> (actual time=1074.674..1074.676 rows=1 loops=1)
>
>Output: PARTIAL count(*)
>
>->  Parallel Seq Scan on public.contend1  (cost=0.00..11690.06 
> rows=126406 width=0) (actual time=0.008..587.454 rows=1
>
> 010200 loops=1)
>
>  Output: id, val, c2, c3, c4, c5, c6, c7, c8, c9, c10, 
> crt_time
>
> Planning Time: 0.123 ms
>
> Execution Time: 1075.588 ms
>
>  postgres=# select pg_backend_pid();
>
> pg_backend_pid
>
> 
>
> 2182678
>
>
>
>
>
>   2, use gdb to debug our backend process. Add the breakpoint in 
> parallel.c:219 and continue.
>
>
>
>   gdb  -q -p 2182678
>
>   ...
>
>   (gdb) b parallel.c:219
>
> Breakpoint 1 at 0x55d085: file 
> /home/liupc/build/build_postgres2/../../devel/postgres2/src/backend/access/transam/parallel.c,
>  line 219.
>
>   (gdb) c
>
> Continuing.
>
>
>
>   3, In the psql clinet, we can execute the explain command in step1 again.
>
>  After we get the breakpoint in gdb, we wait a moment. Then we execute 
> next.
>
>  Use gdb check active_snapshot and transaction_snapshot, 
> active_snapshot->xmin is 158987 and transaction_snapshot->xmin is 162160.
>
> When I use gdb test it, sometimes active_snapshot is the same as 
> transaction_snapshot. Then you can try it multiple times, and before execute 
> next, try wait longer time.
>
>
>
>Breakpoint 1, InitializeParallelDSM (pcxt=0x2d53670)
>
> at 
> /home/liupc/build/build_postgres2/../../devel/postgres2/src/backend/access/transam/parallel.c:219
>
> 219 Snapshottransaction_snapshot = 
> GetTransactionSnapshot();
>
> (gdb) n
>
> 220 Snapshotactive_snapshot = GetActiveSnapshot();
>
> (gdb)
>
> 223 oldcontext = MemoryContextSwitchTo(TopTransactionContext);
>
> (gdb) p *transaction_snapshot
>
> $1 = {snapshot_type = SNAPSHOT_MVCC, xmin = 162160, xmax = 183011, xip = 
> 0x2d50d10, xcnt = 179, subxip = 0x148a9cddf010,
>
>   subxcnt = 0, suboverflowed = true, takenDuringRecovery = false, copied = 
> false, curcid = 0, speculativeToken = 0,
>
>   active_count = 0, regd_count = 0, ph_node = {first_child = 0x0, 
> next_sibling = 0x0, prev_or_parent = 0x0}, whenTaken = 0, lsn = 0}
>
> (gdb) p *active_snapshot
>
> $2 = {snapshot_type = SNAPSHOT_MVCC, xmin = 158987, xmax = 173138, xip = 
> 0x2d53288, xcnt = 178, subxip = 0x0, subxcnt = 0,
>
>   suboverflowed = true, takenDuringRecovery = false, copied = true, curcid = 
> 0, speculativeToken = 0, active_count = 1,
>
>   regd_count = 2, ph_node = {first_child = 0x0, next_sibling = 0x0, 
> prev_or_parent = 0x2d52e48}, whenTaken = 0, lsn = 0}
>
> (gdb)
>
>

Hi Pengcheng,

I followed all your steps.
However, I perhaps get different behavior in my environment.
99% of the time, the xmin and xmax of the active_snapshot and
transaction_snapshot are the same (regardless of how long I wait at
different points after the breakpoint is hit). I've had one or two
instances where the xmax values differ. I managed to catch just one
case where there were different xmin and xmax values in the snapshots,
but this occurred just prior to 

Re: OOM in spgist insert

[Pavel Borisov]

>
>
> Now when checking for shortening of  leaf tuple is added LongValuesOK
> become slightly redundant. I'd propose to replace it with more legible name
> as LongValuesOK doesn't mean they are warranted to be ok just that we can
> try to shorten them? What about tryShortening, trySuffixing or
> can(Try)ShortenTuple?
>
Or maybe it's even more logical now to invert it and make
dontTrySuffixing for use in the opclasses for kd-tree, quadtree etc which
are warranted to have the same key data length at any tree level ?

--
Best regards,
Pavel Borisov

Postgres Professional: http://postgrespro.com 

RE: Parallel INSERT SELECT take 2

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

> > > > > So, users need to check count(*) for this to determine
> > > > > parallel-safety? How about if we provide a wrapper function on
> > > > > top of this function or a separate function that returns char to
> > > > > indicate whether it is safe, unsafe, or restricted to perform a
> > > > > DML operation on the table?
> > > >
> > > > Such wrapper function make sense.
> > >
> > > Thanks for the suggestion, and I agree.
> > > I will add another wrapper function and post new version patches soon.
> >
> > Attaching new version patches with the following changes:
> >
> > 0001
> > Add a new function pg_get_max_parallel_hazard('table_name') returns
> > char('s', 'u', 'r') which indicate whether it is safe, unsafe, or 
> > restricted to
> perform a DML.
> 
> Thanks for the patches. I think we should have the table name as regclass type
> for pg_get_max_parallel_hazard? See, pg_relation_size, pg_table_size,
> pg_filenode_relation and so on.

Thanks for the comment.
I have changed the type to regclass in the latest patchset.

Best regards,
houzj

Re: OOM in spgist insert

[Pavel Borisov]

Now when checking for shortening of  leaf tuple is added LongValuesOK
become slightly redundant. I'd propose to replace it with more legible name
as LongValuesOK doesn't mean they are warranted to be ok just that we can
try to shorten them? What about tryShortening, trySuffixing or
can(Try)ShortenTuple?


-- 
Best regards,
Pavel Borisov

Postgres Professional: http://postgrespro.com 

Re: View invoker privileges

[Noah Misch]

On Wed, Apr 14, 2021 at 10:25:08AM +0300, Ivan Ivanov wrote:
> In Postgres we can create view with view owner privileges only. What’s the
> reason that there is no option to create view with invoker privileges? Is
> there any technical or security subtleties related to absence of this
> feature?

The SQL standard calls for the owner privileges behavior, and nobody has
implemented an invoker privileges option.  I know of no particular subtlety.
An SQL-language function can behave like an invoker-privileges view, but a
view would allow more optimizer freedom.  It would be a good option to have.

Re: naming of async_mode parameter

[Etsuro Fujita]

Hi,

On Thu, May 13, 2021 at 2:23 AM Zhihong Yu  wrote:
> I was looking at
>   Fix EXPLAIN ANALYZE for async-capable nodes.

Thanks for that!

> which adds the following parameter / field:
>
> +   boolasync_mode; /* true if node is in async mode */
>
> async_mode implies an enum: {sync, async}
> Since there are only two values, the data type is bool. I think it should be 
> named is_async.

By async_mode, I mean "is in async mode?", as commented above.  I
thought the naming is_in_async_mode would be a bit long, so I
shortened it to async_mode.  IIUC, I think another example in our
codebase would be the hash_spill_mode parameter in the AggState
struct.  So I think async_mode would be acceptable IMO.

Best regards,
Etsuro Fujita

Re: Forget close an open relation in ReorderBufferProcessTXN()

[Amit Langote]

Takamichi-san,

On Fri, May 14, 2021 at 11:19 AM osumi.takami...@fujitsu.com
 wrote:
> On Thursday, May 13, 2021 7:43 PM Amit Kapila  wrote:
> > On Tue, Apr 20, 2021 at 8:36 AM Amit Langote 
> > wrote:
> > > Back in:
> > https://www.postgresql.org/message-id/CA%2BHiwqEeU19iQgjN6HF1HTP
> > U0L5%2
> > > BJxyS5CmxaOVGNXBAfUY06Q%40mail.gmail.com
> > >
> > > I had proposed to move the map creation from maybe_send_schema() to
> > > get_rel_sync_entry(), mainly because the latter is where I realized it
> > > belongs, though a bit too late.
> >
> > It seems in get_rel_sync_entry, it will only build the map again when there 
> > is
> > any invalidation in publication_rel. Don't we need to build it after any 
> > DDL on
> > the relation itself? I haven't tried this with a test so I might be missing
> > something.
> Yeah, the patch not only tries to address the memory leak
> but also changes the timing (condition) to call convert_tuples_by_name.
> This is because the patch placed the function within a condition of 
> !entry->replicate_valid in get_rel_sync_entry.
> OTOH, OSS HEAD calls it based on RelationSyncEntry's schema_sent in 
> maybe_send_schema.
>
> The two flags (replicate_valid and schema_sent) are reset at different timing 
> somehow.
> InvalidateSystemCaches resets both flags but schema_send is also reset by 
> LocalExecuteInvalidationMessage
> while replicate_valid is reset by CallSyscacheCallbacks.
>
> IIUC, InvalidateSystemCaches, which applies to both flags, is called
> when a transaction starts, via AtStart_Cache and when a table lock is taken 
> via LockRelationOid, etc.
> Accordingly, I think we can notice changes after any DDL on the relation.
>
> But, as for the different timing, we need to know the impact of the change 
> accurately.
> LocalExecuteInvalidationMessage is called from functions in reorderbuffer
> (e.g. ReorderBufferImmediateInvalidation, ReorderBufferExecuteInvalidations).
> This seems to me that changing the condition by the patch
> reduces the chance of the reorderbuffer's proactive reset of
> the flag which leads to rebuild the map in the end.
>
> Langote-san, could you please explain this perspective ?

Please check the reply I just sent.  In summary, moving map-creation
into get_rel_sync_entry() was not correct.

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

Re: Forget close an open relation in ReorderBufferProcessTXN()

[Amit Langote]

On Thu, May 13, 2021 at 7:43 PM Amit Kapila  wrote:
> On Tue, Apr 20, 2021 at 8:36 AM Amit Langote  wrote:
> > On Sat, Apr 17, 2021 at 1:30 PM Amit Kapila  wrote:
> > > On Fri, Apr 16, 2021 at 11:24 PM Andres Freund  
> > > wrote:> > This made me take a brief look at pgoutput.c - maybe I am 
> > > missing
> > > > something, but how is the following not a memory leak?
> > > >
> > > > static void
> > > > maybe_send_schema(LogicalDecodingContext *ctx,
> > > >   ReorderBufferTXN *txn, ReorderBufferChange *change,
> > > >   Relation relation, RelationSyncEntry *relentry)
> > > > {
> > > > ...
> > > > /* Map must live as long as the session does. */
> > > > oldctx = MemoryContextSwitchTo(CacheMemoryContext);
> > > > relentry->map = 
> > > > convert_tuples_by_name(CreateTupleDescCopy(indesc),
> > > >
> > > > CreateTupleDescCopy(outdesc));
> > > > MemoryContextSwitchTo(oldctx);
> > > > send_relation_and_attrs(ancestor, xid, ctx);
> > > > RelationClose(ancestor);
> > > >
> > > > If - and that's common - convert_tuples_by_name() won't have to do
> > > > anything, the copied tuple descs will be permanently leaked.
> > > >
> > >
> > > I also think this is a permanent leak. I think we need to free all the
> > > memory associated with this map on the invalidation of this particular
> > > relsync entry (basically in rel_sync_cache_relation_cb).
> >
> > I agree there's a problem here.
> >
> > Back in:
> >
> > https://www.postgresql.org/message-id/CA%2BHiwqEeU19iQgjN6HF1HTPU0L5%2BJxyS5CmxaOVGNXBAfUY06Q%40mail.gmail.com
> >
> > I had proposed to move the map creation from maybe_send_schema() to
> > get_rel_sync_entry(), mainly because the latter is where I realized it
> > belongs, though a bit too late.
> >
>
> It seems in get_rel_sync_entry, it will only build the map again when
> there is any invalidation in publication_rel. Don't we need to build
> it after any DDL on the relation itself? I haven't tried this with a
> test so I might be missing something.

That's a good point, I didn't really think that through.  So,
rel_sync_cache_relation_cb(), that gets called when the published
table's relcache is invalidated, only resets schema_sent but not
replicate_valid.  The latter, as you said, is reset by
rel_sync_cache_publication_cb() when a pg_publication syscache
invalidation occurs.  So with the patch, it's possible for the map to
not be recreated, even when it should, if for example DDL changes the
table's TupleDesc.

I have put the map-creation code back into maybe_send_schema() in the
attached updated patch, updated some comments related to the map, and
added a test case that would fail with the previous patch (due to
moving map-creation code into get_rel_sync_entry() that is) but
succeeds with the updated patch.

> Also, don't we need to free the
> entire map as suggested by me?

Yes, I had missed that too.  Addressed in the updated patch.

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


pgoutput-tupeconv-map-leak_v2.patch
Description: Binary data

Re: Added missing tab completion for alter subscription set option

[Bharath Rupireddy]

On Fri, May 14, 2021 at 12:00 PM vignesh C  wrote:
>
> Hi,
>
> While I was reviewing one of the logical decoding features, I found
> Streaming and binary options were missing in tab completion for the
> alter subscription set option, the attached patch has the changes for
> the same.
> Thoughts?

+1.

Without patch:
postgres=# alter subscription testsub set (S
SLOT_NAME   SYNCHRONOUS_COMMIT

With patch:
postgres=# alter subscription testsub set (
BINARY  SLOT_NAME   STREAMING   SYNCHRONOUS_COMMIT

How about ordering the options alphabetically as the tab complete
output anyways shows that way? I'm not sure if that's the practice,
but just a thought.
Change:
+COMPLETE_WITH("binary", "slot_name", "synchronous_commit",
"streaming");
To:
+COMPLETE_WITH("binary", "slot_name", "streaming",
"synchronous_commit");

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

Bracket, brace, parenthesis

[Kyotaro Horiguchi]

I found the following code in multirangetypes.c

>   if (*ptr == '{')
>   ptr++;
>   else
>   ereport(ERROR,
>   (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
>errmsg("malformed multirange literal: \"%s\"",
>   input_str),
>errdetail("Missing left bracket.")));

I'm not sure how much we (or people) are strcit on the distinction
between the $SUBJECT, isn't '{' a brace generally?

postgres=# select '[1,3]'::int4multirange;
ERROR:  malformed multirange literal: "[1,3]"
LINE 1: select '[1,3]'::int4multirange;
   ^
DETAIL:  Missing left bracket.

The distinction is significant there.  It should at least be "Missing
left curly bracket." or "Missing left brace." (or left curly brace..?)

'{' is mentioned as "curly brackets" in comments of the soruce file.
It is mentioned as "brace" in regexp doc [1].  And.. uh.. I found the
world "curly braces" in the doc for conding conventions..

[1]: https://www.postgresql.org/docs/devel/functions-matching.html

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/utils/adt/multirangetypes.c b/src/backend/utils/adt/multirangetypes.c
index 0b81649779..fbcc27d072 100644
--- a/src/backend/utils/adt/multirangetypes.c
+++ b/src/backend/utils/adt/multirangetypes.c
@@ -146,7 +146,7 @@ multirange_in(PG_FUNCTION_ARGS)
 (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
  errmsg("malformed multirange literal: \"%s\"",
 		input_str),
- errdetail("Missing left bracket.")));
+ errdetail("Missing left brace.")));
 
 	/* consume ranges */
 	parse_state = MULTIRANGE_BEFORE_RANGE;
@@ -282,7 +282,7 @@ multirange_in(PG_FUNCTION_ARGS)
 (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
  errmsg("malformed multirange literal: \"%s\"",
 		input_str),
- errdetail("Junk after right bracket.")));
+ errdetail("Junk after right brace.")));
 
 	ret = make_multirange(mltrngtypoid, rangetyp, range_count, ranges);
 	PG_RETURN_MULTIRANGE_P(ret);

Re: Support for VACUUMing Foreign Tables

[Bharath Rupireddy]

On Fri, May 14, 2021 at 11:48 AM Dilip Kumar  wrote:
>
> On Fri, May 14, 2021 at 6:35 AM tsunakawa.ta...@fujitsu.com
>  wrote:
> >
> > From: Bharath Rupireddy 
> > > I think it will be useful to allow foreign tables to be VACUUMed if
> > > the underlying FDW supports, currently VACUUM doesn't support foreign
> > > tables, see [1].
> >
> > Could you let us imagine more concretely how useful it will be?  While 
> > TRUNCATE can be part of an application's data processing as alternative to 
> > DELETE, I think VACUUM is purely the data storage maintenance that's 
> > performed by the DBA and can be done naturally locally on the server where 
> > the table resides.  (The existing ANALYZE on FDW is an exception; it's 
> > useful to also have data statistics locally.)
>
> I agree that TRUNCATE is a user-visible command so it is good to send
> such a command to a remote server.  But, sending ANALYZE and VACUUM to
> FDW can have a similar use case.  I mean based on the current data
> changes/operation the DBA can decide whether it needs to generate the
> statistic or whether it needs to do garbage collection.  I think
> ideally both these operations can be performed locally on the remote
> server but if we are sending ANALYZE to the remote server then IMHO
> there is some merit to sending VACUUM as well.  Having said that, I
> think the purpose of ANALYZE can be generic across the databases that
> we want to update the statistic but VACUUM is different, it too much
> depends upon how the data is stored (what MVCC mechanism they are
> using) on the remote server and what is the vacuuming need for that
> database.  So maybe garbage collection should be controlled locally by
> the DBA on that server.

Agree. Different MVCC databases can have different commands to clean
up the bloat, their implementation of the vacuum's FdwRoutine can be
implemented accordingly. postgres_fdw can prepare the "VACUUM rel;"
command. Having said that, I don't think all the remote databases will
have the same ANALYZE rel; or TRUNCATE rel; commands either. It's
better left to the implementation of the FdwRoutine for a particular
remote database.

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

Added missing tab completion for alter subscription set option

[vignesh C]

Hi,

While I was reviewing one of the logical decoding features, I found
Streaming and binary options were missing in tab completion for the
alter subscription set option, the attached patch has the changes for
the same.
Thoughts?

Regards,
Vignesh
From 84a0c07760c913576ef38d74ab37ffd9ee3ad686 Mon Sep 17 00:00:00 2001
From: vignesh 
Date: Fri, 14 May 2021 11:37:05 +0530
Subject: [PATCH v1] Added missing tab completion for alter subscription set
 option.

Streaming and binary options were missing in tab completion for alter
subscription set option, included it.
---
 src/bin/psql/tab-complete.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 6598c5369a..2c15d8064b 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -1665,7 +1665,7 @@ psql_completion(const char *text, int start, int end)
 		COMPLETE_WITH("(", "PUBLICATION");
 	/* ALTER SUBSCRIPTION  SET ( */
 	else if (HeadMatches("ALTER", "SUBSCRIPTION", MatchAny) && TailMatches("SET", "("))
-		COMPLETE_WITH("slot_name", "synchronous_commit");
+		COMPLETE_WITH("binary", "slot_name", "synchronous_commit", "streaming");
 	/* ALTER SUBSCRIPTION  SET PUBLICATION */
 	else if (HeadMatches("ALTER", "SUBSCRIPTION", MatchAny) && TailMatches("SET", "PUBLICATION"))
 	{
-- 
2.25.1

Re: Support for VACUUMing Foreign Tables

[Bharath Rupireddy]

On Fri, May 14, 2021 at 6:35 AM tsunakawa.ta...@fujitsu.com
 wrote:
>
> From: Bharath Rupireddy 
> > I think it will be useful to allow foreign tables to be VACUUMed if
> > the underlying FDW supports, currently VACUUM doesn't support foreign
> > tables, see [1].
>
> Could you let us imagine more concretely how useful it will be?  While 
> TRUNCATE can be part of an application's data processing as alternative to 
> DELETE, I think VACUUM is purely the data storage maintenance that's 
> performed by the DBA and can be done naturally locally on the server where 
> the table resides.  (The existing ANALYZE on FDW is an exception; it's useful 
> to also have data statistics locally.)

This can be useful in situations like where there are many remote
postgres servers that are connected to a single coordinator on which
foreign tables are defined for each of the remote tables. In this
case, the DBA (or whoever is responsible to do that job) doesn't have
to figure out which remote server should be logged onto to perform the
VACUUM. They can issue VACUUM command on the foreign table from the
coordinator server.

> > this may not be much useful for FDWs that connect to remote non-MVCC
> > databases where the concept of VACUUM may not apply, but for
> > postgres_fdw and others it might help.
>
> Can you show some examples of "others"?  I believe we should be careful not 
> to make the FDW interface a swamp for functions that are only convenient for 
> PostgreSQL.

There are other databases that have MVCC implemented for which the
bloat clean up might be necessary at some point. They may not have the
same terminology that postgres has for cleaning up the bloat. For
instance, MySQL (instead of VACUUM they have OPTIMIZE TABLE command)
which can be connected to postgres_fdw using supported fdw

And see [1] for the databases that have MVCC support. I'm not sure if
all of them have a FDW to connect to postgres.

[1] 
https://dbdb.io/browse?concurrency-control=multi-version-concurrency-control-mvcc

> How about adding a routine to the FDW interface that allows to execute an 
> arbitrary command like the following?  VACUUM will be able to use this.
>
> PGresult *DoCommandPathThrough(ForeignTable *table, const char *command);
>
> Or, maybe it's more flexible to use ForeignServer instead of ForeignTable.

I agree with Michael Paquier's response to this point that it can be
an issue from a security standpoint. But we could have had such kind
of a generic API for commands like TRUNCATE, ANALYZE, VACUUM etc. void
ExecRemoteCommand(ForeignServer *server, const char *command, void
*input_params, void *output_params); with the API knowing all the
supported commands and erroring out on unsupported commands. Now, we
have a separate API for each of the supported commands which looks
actually cleaner.

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

Re: What is lurking in the shadows?

[Michael Paquier]

On Fri, May 14, 2021 at 01:16:37PM +1200, David Rowley wrote:
> I'm inclined to think that since a bug has already been found due to a
> local variable shadowing a global one that it would be good to review
> these and then consider if it's worth doing any renaming. I think the
> process of looking at each warning individually will allow us to
> determine if; a) there are any bugs, or; b) if it's worth doing any
> renaming.

70116493 is another instance of that, from a not-so-far past..

> I'd say it might be worth aspiring to reduce the warnings from
> building with these flags. If we reduced these down then it might
> allow us to more easily identify cases where there are actual bugs.
> Maybe we can get to a point where we could enable either
> -Wshadow=compatible-local or -Wshadow=local.  I doubt we could ever
> get to a stage where -Wshadow=global would work for us.  There's also
> some quite old discussion in [1] that you might want to review.

Agreed, not before the 15 branch opens for business for cosmetic
changes.  compatible-local did not sound that much interesting to me
on first sight, but the report of Peter tells the contrary: most of
the conflicts come from local problems.  I am not sure that you could
enable that safely though as PG_TRY() would complain on that, for
example in ProcessUtilitySlow().

> We also need to take into account that renaming variables here can
> increase the overhead of backpatching fixes.  The process of fixing
> those up to make the patch apply to the back branch does increase the
> chances that bugs could make their way into the back branches.
> However, it's probably more likely to end up as a bug if the patch was
> written for the back branch then there's a bigger opportunity for the
> patch author to pick the wrong variable name when converting the patch
> to work with master. In the reverse case, that does not seem as likely
> due to both variables having the same name.

That may be tricky, even if global or local variables are changed,
but I'd like to think that there is room for improvement.  Looking at
the report, the global conflicts involve:
- synchronous_commit
- ssl_key_file
- wal_segment_size
- DataDir, with the control data business. 

These seem changeable without much holes with potential back-patches.
--
Michael


signature.asc
Description: PGP signature

Re: Support for VACUUMing Foreign Tables

[Dilip Kumar]

On Fri, May 14, 2021 at 6:35 AM tsunakawa.ta...@fujitsu.com
 wrote:
>
> From: Bharath Rupireddy 
> > I think it will be useful to allow foreign tables to be VACUUMed if
> > the underlying FDW supports, currently VACUUM doesn't support foreign
> > tables, see [1].
>
> Could you let us imagine more concretely how useful it will be?  While 
> TRUNCATE can be part of an application's data processing as alternative to 
> DELETE, I think VACUUM is purely the data storage maintenance that's 
> performed by the DBA and can be done naturally locally on the server where 
> the table resides.  (The existing ANALYZE on FDW is an exception; it's useful 
> to also have data statistics locally.)

I agree that TRUNCATE is a user-visible command so it is good to send
such a command to a remote server.  But, sending ANALYZE and VACUUM to
FDW can have a similar use case.  I mean based on the current data
changes/operation the DBA can decide whether it needs to generate the
statistic or whether it needs to do garbage collection.  I think
ideally both these operations can be performed locally on the remote
server but if we are sending ANALYZE to the remote server then IMHO
there is some merit to sending VACUUM as well.  Having said that, I
think the purpose of ANALYZE can be generic across the databases that
we want to update the statistic but VACUUM is different, it too much
depends upon how the data is stored (what MVCC mechanism they are
using) on the remote server and what is the vacuuming need for that
database.  So maybe garbage collection should be controlled locally by
the DBA on that server.

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

Re: Some doubious error messages and comments

[Kyotaro Horiguchi]

At Mon, 10 May 2021 15:52:15 +0900, Michael Paquier  wrote 
in 
> On Wed, Apr 28, 2021 at 08:11:47AM -0500, Justin Pryzby wrote:
> > On Wed, Apr 28, 2021 at 05:36:33PM +0900, Kyotaro Horiguchi wrote:
> >> 0001: I found some typos in a error message and a comment.
> >> 
> >> multirangetypes.c: 1420
> >> > errmsg("range_intersect_agg must be called with a multirange")));
> >> 
> >> This "range_intersect_agg" looks like a typo of "multirange_..".
> >> 
> >> operatorcmds.c:303
> >> > * Look up a join estimator function ny name, and verify that it has the
> >> 
> >> "ny" looks like a typo of "by".
> > 
> > "ny name" shows up a 2nd time.
> > 
> > I have another "comment typos" patch - maybe someone will want to push them
> > together.
> 
> Thanks.  Two of them were already fixed, two of them are correct but
> went missing so I have applied a fix for these.  The change in

Thanks.

> multirange_intersect_agg_transfn() is incorrect as the error refers to
> the SQL function range_intersect_agg().

Uh!! It seems to be a never-shown message.  Thaks for checking that.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center