Re: Wrong assert in TransactionGroupUpdateXidStatus

[Amit Kapila]

On Wed, Dec 11, 2019 at 11:00 AM Amit Kapila  wrote:
>
> On Wed, Dec 11, 2019 at 4:02 AM Andres Freund  wrote:
> > On 2019-12-10 13:55:40 +0530, Dilip Kumar wrote:
> >
> > > /*
> > > * Overflowed transactions should not use group XID status update
> > > * mechanism.
> > > */
> > > Assert(!pgxact->overflowed);
> > >
> > > A solution could be either we remove this assert or change this assert
> > > to Assert(pgxact->nxids <= THRESHOLD_SUBTRANS_CLOG_OPT);
> >
> > Maybe I'm missing something, but isn't this a bug then? IIRC We can't
> > rely on MyProc->subxids once we overflowed, even if since then the
> > remaining number of children has become low enough.
> >
>
> AFAICS, the MyProc->subxids is maintained properly if the number of
> subtransactions is lesser than PGPROC_MAX_CACHED_SUBXIDS (64).  Can
> you explain the case where that won't be true?  Also, even if what you
> are saying is true, I think the memcmp in TransactionIdSetPageStatus
> should avoid taking us a wrong decision.
>

I am able to reproduce the issue by reducing the values of
PGPROC_MAX_CACHED_SUBXIDS and THRESHOLD_SUBTRANS_CLOG_OPT to 2.  Below
is what I did after reducing the values:
Session-1
--
postgres=# begin;
BEGIN
postgres=# insert into t1 values(1);
INSERT 0 1
postgres=# savepoint s1;
SAVEPOINT
postgres=# insert into t1 values(2);
INSERT 0 1
postgres=# savepoint s2;
SAVEPOINT
postgres=# insert into t1 values(3);
INSERT 0 1
postgres=# savepoint s3;
SAVEPOINT
postgres=# insert into t1 values(4);
INSERT 0 1
postgres=# rollback to s2;
ROLLBACK

Session-2
---
insert into t1 values(4);  -- attach debugger and stop in
TransactionIdSetPageStatus after acquiring CLogControlLock

Session-1
---
Commit;  -- This will wait to acquire CLogControlLock in a group
update path (TransactionGroupUpdateXidStatus).  Now, continue in the
session-2 debugger.  After that continue in session-1's debugger and
it will hit the Assert.

The attached patch fixes it by changing the Assert.  I have
additionally removed the redundant Assert in
TransactionIdSetPageStatus as pointed out by Andres.  I am planning to
commit and backpatch this early next week (Monday) unless someone
wants to review it further or has objections to it.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


v2-0001-Change-overly-strict-Assert-in-TransactionGroupUpdat.patch
Description: Binary data

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

[David Fetter]

On Thu, Dec 12, 2019 at 12:23:42AM -0500, Tom Lane wrote:
> David Fetter  writes:
> > I've found myself writing a lot of boilerplate pg_hba.conf entries
> > along the lines of
> > hostnosslall all 0.0.0.0/0  reject
> > hostssl  all all 0.0.0.0/0  md5
> > so I thought I'd make it easier to do that from initdb.
> > What say?
> 
> I'm pretty suspicious of loading down initdb with random configuration
> options, because I think most people nowadays use PG via vendor packages
> that script their calls to initdb.  So an option like this doesn't help
> unless you can persuade all those vendors to pass the option through.

Would the official PGDG .deb and .rpm packages suffice?

> That problem exists even before you get to the question of whether
> this specific option is useful or well-designed ... a question I'm
> not opining about here, but it would certainly require thought.

I think it was a reasonable extension. We cover lines that start with
local and host, but they can also start with hostssl and hostnossl.

Meanwhile, please find attached a fix for an oversight around IPv6.

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
>From cd32f9d27dbb0f7c24a5f8141267b0943320f546 Mon Sep 17 00:00:00 2001
From: David Fetter 
Date: Wed, 11 Dec 2019 18:55:03 -0800
Subject: [PATCH v2] Enable setting pg_hba.conf permissions from initdb
To: hackers
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="2.23.0"

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


The new optional arguments are --auth-hostssl and --auth-hostnossl

diff --git a/doc/src/sgml/ref/initdb.sgml b/doc/src/sgml/ref/initdb.sgml
index da5c8f5307..03b76732d1 100644
--- a/doc/src/sgml/ref/initdb.sgml
+++ b/doc/src/sgml/ref/initdb.sgml
@@ -154,6 +154,28 @@ PostgreSQL documentation
   
  
 
+ 
+  --auth-hostssl=authmethod
+  
+   
+This option specifies the authentication method for users via
+TLS connections used in pg_hba.conf
+(hostssl lines).
+   
+  
+ 
+
+ 
+  --auth-hostnossl=authmethod
+  
+   
+This option specifies the authentication method for users via
+non-TLS connections used in pg_hba.conf
+(hostnossl lines).
+   
+  
+ 
+
  
   --auth-local=authmethod
   
diff --git a/src/backend/libpq/pg_hba.conf.sample b/src/backend/libpq/pg_hba.conf.sample
index c853e36232..48ef465cca 100644
--- a/src/backend/libpq/pg_hba.conf.sample
+++ b/src/backend/libpq/pg_hba.conf.sample
@@ -87,3 +87,9 @@ hostall all ::1/128 @authmethodhost@
 @remove-line-for-nolocal@local   replication all @authmethodlocal@
 hostreplication all 127.0.0.1/32@authmethodhost@
 hostreplication all ::1/128 @authmethodhost@
+
+# hostnossl all   all 0.0.0.0/0   @authmethodhostnossl@
+# hostnossl all   all ::/0@authmethodhostnossl@
+
+# hostssl all all 0.0.0.0/0   @authmethodhostssl@
+# hostssl all all ::/0@authmethodhostssl@
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index 1f6d8939be..09bf49aa23 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -136,6 +136,8 @@ static char *pwfilename = NULL;
 static char *superuser_password = NULL;
 static const char *authmethodhost = NULL;
 static const char *authmethodlocal = NULL;
+static const char *authmethodhostssl = NULL;
+static const char *authmethodhostnossl = NULL;
 static bool debug = false;
 static bool noclean = false;
 static bool do_sync = true;
@@ -438,7 +440,6 @@ replace_token(char **lines, const char *token, const char *replacement)
  *
  * a sort of poor man's grep -v
  */
-#ifndef HAVE_UNIX_SOCKETS
 static char **
 filter_lines_with_token(char **lines, const char *token)
 {
@@ -461,7 +462,6 @@ filter_lines_with_token(char **lines, const char *token)
 
 	return result;
 }
-#endif
 
 /*
  * get the lines from a text file
@@ -1326,6 +1326,44 @@ setup_config(void)
 			  "@authcomment@",
 			  (strcmp(authmethodlocal, "trust") == 0 || strcmp(authmethodhost, "trust") == 0) ? AUTHTRUST_WARNING : "");
 
+	if (authmethodhostssl != NULL)
+	{
+		conflines = replace_token(conflines,
+  "# hostssl all all 0.0.0.0/0   @authmethodhostssl@",
+  "hostssl all all 0.0.0.0/0   @authmethodhostssl@");
+#ifdef HAVE_IPV6
+		conflines = replace_token(conflines,
+  "# hostssl all all ::/0   

Re: logical decoding : exceeded maxAllocatedDescs for .spill files

[Amit Khandekar]

On Thu, 12 Dec 2019 at 11:34, Amit Khandekar  wrote:

> So max_changes_in_memory is the one
> that allows us to reduce the number of transactions required, so we
> can cut down on the outer loop iterations and make the test finish
> much earlier.

>
> But also note that, we can't use the test suite in
> contrib/test_decoding, because max_changes_in_memory needs server
> restart. So we need to shift this test to src/test/recovery. And
> there, I guess it is not that critical for the testcase to be very
> quick because the tests in general are much slower than the ones in
> contrib/test_decoding, although it would be nice to make it fast. What
> I propose is to modify  max_changes_in_memory, do a server restart
> (which takes hardly a sec), run the testcase (3.5 sec) and then
> restart after resetting the guc. So totally it will be around 4-5
> seconds.

Sorry I meant max_files_per_process. We need to reduce
max_files_per_process, so that it causes max_safe_fds to be reduced,
and so only a few transactions are sufficient to reproduce the
problem, because the reserveAllocatedDesc() will return false much
sooner due to low max_safe_fds.

-- 
Thanks,
-Amit Khandekar
EnterpriseDB Corporation
The Postgres Database Company

Re: Collation versioning

[Thomas Munro]

On Thu, Dec 12, 2019 at 6:32 PM Tom Lane  wrote:
> Thomas Munro  writes:
> > Erm, but I shouldn't have to reindex my hash indexes (at least not
> > until someone invents collation-based equality and therefore
> > necessarily also collation-based hashing).  How can we exclude that?
>
> Um, we already invented that with nondeterministic collations, no?

Urghlgh, right, thanks, somehow I missed/forgot that that stuff
already works for hashing (neat).  So we do need to track collation
version dependencies for hash indexes, but only for non-deterministic
collations.  I wonder how best to code that.

Re: logical decoding : exceeded maxAllocatedDescs for .spill files

[Amit Khandekar]

On Thu, 12 Dec 2019 at 09:49, Amit Kapila  wrote:
>
> On Wed, Dec 11, 2019 at 4:17 PM Amit Khandekar  wrote:
> >
> > On Sat, 7 Dec 2019 at 11:37, Amit Kapila  wrote:
> > >
> > > On Fri, Dec 6, 2019 at 5:00 PM Amit Khandekar  
> > > wrote:
> > > >
> > > > On Fri, 6 Dec 2019 at 15:40, Amit Kapila  
> > > > wrote:
> > > > >
> > > > > 1.
> > > > > + /* Now that the state fields are initialized, it is safe to return 
> > > > > it. */
> > > > > + *iter_state = state;
> > > > > +
> > > > >   /* allocate heap */
> > > > >   state->heap =
> > > > > binaryheap_allocate(state->nr_txns,
> > > > > ReorderBufferIterCompare,
> > > > >
> > > > > Is there a reason for not initializing iter_state after
> > > > > binaryheap_allocate?  If we do so, then we don't need additional check
> > > > > you have added in ReorderBufferIterTXNFinish.
> > > >
> > > > If iter_state is initialized *after* binaryheap_allocate, then we
> > > > won't be able to close the vfds if binaryheap_allocate() ereports().
> > > >
> > >
> > > Is it possible to have vfds opened before binaryheap_allocate(), if so 
> > > how?
> > No it does not look possible for the vfds to be opened before
> > binaryheap_allocate(). But actually, the idea behind placing the
> > iter_state at the place where I put is that, we should return back the
> > iter_state at the *earliest* place in the code where it is safe to
> > return.
> >
>
> Sure, I get that point, but it seems it is equally good to do this
> after binaryheap_allocate().  It will be sligthly better because if
> there is any error in binaryheap_allocate, then we don't need to even
> call ReorderBufferIterTXNFinish().

All right. WIll do that.

>
> >
> > >> 4. I think we should also check how much time increase will happen for
> > >> test_decoding regression test after the test added by this patch?
> > Amit Khandekar  wrote:
> > > Yeah, it currently takes noticeably longer compared to the others.
> > > Let's see if setting logical_decoding_work_mem to a min value allows
> > > us to reproduce the test with much lesser number of inserts.
> >
> > The test in the patch takes around 20 seconds, as compared to the max
> > time of 2 seconds any of the other tests take in that test suite.
> >
> > But if we set the max_files_per_process to a very low value (say 26)
> > as against the default 1000, we can reproduce the issue with as low as
> > 20 sub-transactions as against the 600 that I used in spill.sql test.
> > And with this, the test runs in around 4 seconds, so this is good.
> >
>
> Do you get 4s even after setting the minimum value of
> logical_decoding_work_mem?  I think 4s is also too much for this test
> which is going to test one extreme scenario. Can you try with some
> bigger rows or something else to reduce this time?  I think if we can
> get it close to 2s or whatever maximum time taken by any other logical
> decoding tests, then good, otherwise, it doesn't seem like a good idea
> to add this test.

>
> > But
> > the problem is : max_files_per_process needs server restart. So either
> > we have to shift this test to src/test/recovery in one of the
> > logical_decoding test, or retain it in contrib/test_decoding and let
> > it run for 20 seconds. Let me know if you figure out any other
> > approach.
> >
>
> I don't think 20s for one test is acceptable.  So, we should just give
> up that path, you can try what I suggested above, if we can reduce the
> test time, then good, otherwise, I suggest to drop the test from the
> patch.  Having said that, we should try our best to reduce the test
> time as it will be good if we can have such a test.

I tried; it is actually roughly 3.4 seconds. Note that reducing
logical_decoding_work_mem does not reduce the test time. It causes the
serialization to start early, and so increases the chance of
reproducing the problem. During restore of the serialized data, we
still use max_changes_in_memory. So max_changes_in_memory is the one
that allows us to reduce the number of transactions required, so we
can cut down on the outer loop iterations and make the test finish
much earlier.

But also note that, we can't use the test suite in
contrib/test_decoding, because max_changes_in_memory needs server
restart. So we need to shift this test to src/test/recovery. And
there, I guess it is not that critical for the testcase to be very
quick because the tests in general are much slower than the ones in
contrib/test_decoding, although it would be nice to make it fast. What
I propose is to modify  max_changes_in_memory, do a server restart
(which takes hardly a sec), run the testcase (3.5 sec) and then
restart after resetting the guc. So totally it will be around 4-5
seconds.


-- 
Thanks,
-Amit Khandekar
EnterpriseDB Corporation
The Postgres Database Company

Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes

[Tom Lane]

I wrote:
> Alvaro Herrera  writes:
>> I don't quite understand why a readline library that doesn't have
>> rl_filename_completion_function is known to have a
>> filename_completion_function, ie. this bit 

>> #ifdef HAVE_RL_FILENAME_COMPLETION_FUNCTION
>> #define filename_completion_function rl_filename_completion_function
>> #else
>> /* decl missing in some header files, but function exists anyway */
>> extern char *filename_completion_function();
>> #endif

> I think the point is that before rl_filename_completion_function the
> function existed but was just called filename_completion_function.
> It's possible that that's obsolete --- I've not really checked.

I had a look through the buildfarm results, and it seems that the only
(non-Windows) animals that don't HAVE_RL_FILENAME_COMPLETION_FUNCTION
are prairiedog and locust.  prairiedog is using the libedit that
Apple supplied in its stone-age version of macOS, and I imagine the
same can be said of locust, though that's one macOS release newer.

prairiedog's version does define filename_completion_function:

$ grep completion_func /usr/include/readline/readline.h 
extern CPPFunction  *rl_attempted_completion_function;
char*filename_completion_function(const char *, int);
char*username_completion_function(const char *, int);

so the assumption embodied in our code is both correct and necessary
so far as the current universe of buildfarm critters is concerned.

Having said that, prairiedog's version of libedit is buggy as hell;
it generates bogus warnings at every psql exit, for instance.

$ psql postgres
psql (13devel)
Type "help" for help.

postgres=# \q
could not save history to file "/Users/tgl/.psql_history": operating system 
error 0
$ 

It wouldn't be an unreasonable idea to desupport this version,
if it allowed additional simplifications in psql beside this
particular #ifdef mess.  I'm not sure whether any of the
contortions in e.g. saveHistory could go away if we did so.

regards, tom lane

Re: Collation versioning

[Tom Lane]

Thomas Munro  writes:
> Erm, but I shouldn't have to reindex my hash indexes (at least not
> until someone invents collation-based equality and therefore
> necessarily also collation-based hashing).  How can we exclude that?

Um, we already invented that with nondeterministic collations, no?

regards, tom lane

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

[Tom Lane]

David Fetter  writes:
> I've found myself writing a lot of boilerplate pg_hba.conf entries
> along the lines of
> hostnosslall all 0.0.0.0/0  reject
> hostssl  all all 0.0.0.0/0  md5
> so I thought I'd make it easier to do that from initdb.
> What say?

I'm pretty suspicious of loading down initdb with random configuration
options, because I think most people nowadays use PG via vendor packages
that script their calls to initdb.  So an option like this doesn't help
unless you can persuade all those vendors to pass the option through.

That problem exists even before you get to the question of whether
this specific option is useful or well-designed ... a question I'm
not opining about here, but it would certainly require thought.

regards, tom lane

Re: Collation versioning

[Thomas Munro]

On Thu, Dec 12, 2019 at 5:00 PM Thomas Munro  wrote:
> Then, as a special case, there is the collation of the actual indexed
> value, because that will implicitly be used as input to the btree ops
> that would be collation sensitive.  [...]

Erm, but I shouldn't have to reindex my hash indexes (at least not
until someone invents collation-based equality and therefore
necessarily also collation-based hashing).  How can we exclude that?
amcanorder seems somehow right but also wrong.

Re: non-exclusive backup cleanup is mildly broken

[Kyotaro Horiguchi]

Hello.

At Wed, 11 Dec 2019 17:32:05 -0500, Robert Haas  wrote 
in 
> While reviewing the first patch in Asif Rehman's series of patches for
> parallel backup over at
> http://postgr.es/m/CADM=Jeg3ZN+kPQpiSfeWCXr=xgplrq4cbqe5zviuxygkq3v...@mail.gmail.com
> I discovered that commit 7117685461af50f50c03f43e6a622284c8d54694
> introduced a use of cancel_before_shmem_exit which falsifies the
> comment for that function. So you can cause a spurious WARNING in the
> logs by doing something like this, with max_prepared_transactions set
> to a non-zero value:
> 
> select pg_start_backup('one', false, false);
> begin;
> prepare transaction 'nothing';
> select pg_stop_backup(false);
> \q
> 
> in the server log:
> WARNING:  aborting backup due to backend exiting before pg_stop_backup
> was called
> 
> And you can cause an assertion failure like this:
> 
> select pg_start_backup('one', false, false);
> begin;
> prepare transaction 'nothing';
> select pg_stop_backup(false);
> select pg_start_backup('two');
> \q
> 
> We've discussed before the idea that it might be good to remove the
> limitation that before_shmem_exit() can only remove the
> most-recently-added callback, which would be one way to fix this
> problem, but I'd like to propose an alternative solution which I think
> will work out more nicely for the patch mentioned above: don't use
> cancel_before_shmem_exit, and just leave the callback registered for
> the lifetime of the backend. That requires some adjustment of the
> callback, since it needs to tolerate exclusive backup mode being in
> effect.
> 
> The attached patch takes that approach. Thoughts welcome on (1) the
> approach and (2) whether to back-patch. I think there's little doubt
> that this is formally a bug, but it's a pretty minor one.

The direction seems reasonable, but the patch doesn't free up the
before_shmem_exec slot nor avoid duplicate registration of the
callback. Actually before_shmem_exit_list gets bloat with multiple
do_pg_abort_backup entries through repeated rounds of non-exclusive
backups.

As the result, if one ends a session while a non-exclusive backup is
active after closing the previous non-exclusive backup,
do_pg_abort_backup aborts for assertion failure.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

RE: Conflict handling for COPY FROM

[asaba.takan...@fujitsu.com]

Hello Surafel,

I'm very interested in this patch.
Although I'm a beginner,I would like to participate in the development of 
PostgreSQL.


1. I want to suggest new output format.
In my opinion, it's kind to display description of output and add "line number" 
and "error" to output.
For example,

error lines

line number | first | second | third | error
+---++---+
  1 | 1 | 10 |   0.5 |   UNIQUE
  2 | 2 | 42 |   0.1 |CHECK
  3 | 3 |   NULL | 0 | NOT NULL
(3 rows)

Although only unique or exclusion constraint violation returned back to the 
caller currently, 
I think that column "error" will be useful when it becomes possible to handle 
other types of errors(check, not-null and so on).

If you assume that users re-execute COPY FROM with the output lines as input, 
these columns are obstacles.
Therefore I think that this output format should be displayed only when we set 
new option(for example ERROR_VERBOSE) like "COPY FROM ... ERROR_VERBOSE;".


2. I have a question about copy meta-command.
When I executed copy meta-command, output wasn't displayed.
Does it correspond to copy meta-command?

Regards

--
Asaba Takanori

Re: logical decoding : exceeded maxAllocatedDescs for .spill files

[Amit Kapila]

On Wed, Dec 11, 2019 at 4:17 PM Amit Khandekar  wrote:
>
> On Sat, 7 Dec 2019 at 11:37, Amit Kapila  wrote:
> >
> > On Fri, Dec 6, 2019 at 5:00 PM Amit Khandekar  
> > wrote:
> > >
> > > On Fri, 6 Dec 2019 at 15:40, Amit Kapila  wrote:
> > > >
> > > > 1.
> > > > + /* Now that the state fields are initialized, it is safe to return 
> > > > it. */
> > > > + *iter_state = state;
> > > > +
> > > >   /* allocate heap */
> > > >   state->heap =
> > > > binaryheap_allocate(state->nr_txns,
> > > > ReorderBufferIterCompare,
> > > >
> > > > Is there a reason for not initializing iter_state after
> > > > binaryheap_allocate?  If we do so, then we don't need additional check
> > > > you have added in ReorderBufferIterTXNFinish.
> > >
> > > If iter_state is initialized *after* binaryheap_allocate, then we
> > > won't be able to close the vfds if binaryheap_allocate() ereports().
> > >
> >
> > Is it possible to have vfds opened before binaryheap_allocate(), if so how?
> No it does not look possible for the vfds to be opened before
> binaryheap_allocate(). But actually, the idea behind placing the
> iter_state at the place where I put is that, we should return back the
> iter_state at the *earliest* place in the code where it is safe to
> return.
>

Sure, I get that point, but it seems it is equally good to do this
after binaryheap_allocate().  It will be sligthly better because if
there is any error in binaryheap_allocate, then we don't need to even
call ReorderBufferIterTXNFinish().

>
> >> 4. I think we should also check how much time increase will happen for
> >> test_decoding regression test after the test added by this patch?
> Amit Khandekar  wrote:
> > Yeah, it currently takes noticeably longer compared to the others.
> > Let's see if setting logical_decoding_work_mem to a min value allows
> > us to reproduce the test with much lesser number of inserts.
>
> The test in the patch takes around 20 seconds, as compared to the max
> time of 2 seconds any of the other tests take in that test suite.
>
> But if we set the max_files_per_process to a very low value (say 26)
> as against the default 1000, we can reproduce the issue with as low as
> 20 sub-transactions as against the 600 that I used in spill.sql test.
> And with this, the test runs in around 4 seconds, so this is good.
>

Do you get 4s even after setting the minimum value of
logical_decoding_work_mem?  I think 4s is also too much for this test
which is going to test one extreme scenario. Can you try with some
bigger rows or something else to reduce this time?  I think if we can
get it close to 2s or whatever maximum time taken by any other logical
decoding tests, then good, otherwise, it doesn't seem like a good idea
to add this test.

> But
> the problem is : max_files_per_process needs server restart. So either
> we have to shift this test to src/test/recovery in one of the
> logical_decoding test, or retain it in contrib/test_decoding and let
> it run for 20 seconds. Let me know if you figure out any other
> approach.
>

I don't think 20s for one test is acceptable.  So, we should just give
up that path, you can try what I suggested above, if we can reduce the
test time, then good, otherwise, I suggest to drop the test from the
patch.  Having said that, we should try our best to reduce the test
time as it will be good if we can have such a test.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: PATCH: logical_work_mem and logical streaming of large in-progress transactions

[Dilip Kumar]

On Wed, Dec 11, 2019 at 5:22 PM Amit Kapila  wrote:
>
> On Mon, Dec 9, 2019 at 1:27 PM Dilip Kumar  wrote:
> >
> > I have review the patch set and here are few comments/questions
> >
> > 1.
> > +static void
> > +pg_decode_stream_change(LogicalDecodingContext *ctx,
> > + ReorderBufferTXN *txn,
> > + Relation relation,
> > + ReorderBufferChange *change)
> > +{
> > + OutputPluginPrepareWrite(ctx, true);
> > + appendStringInfo(ctx->out, "streaming change for TXN %u", txn->xid);
> > + OutputPluginWrite(ctx, true);
> > +}
> >
> > Should we show the tuple in the streamed change like we do for the
> > pg_decode_change?
> >
>
> I think so.  The patch shows the message in
> pg_decode_stream_message(), so why to prohibit showing tuple here?
>
> > 2. pg_logical_slot_get_changes_guts
> > It recreate the decoding slot [ctx =
> > CreateDecodingContext(InvalidXLogRecPtr] but doesn't set the streaming
> > to false, should we pass a parameter to
> > pg_logical_slot_get_changes_guts saying whether we want streamed results or 
> > not
> >
>
> CreateDecodingContext internally calls StartupDecodingContext which
> sets the value of streaming based on if the plugin has provided
> callbacks for streaming functions. Isn't that sufficient?  Why do we
> need additional parameters here?

I don't think that if plugin provides streaming function then we
should stream.  Like pgoutput plugin provides streaming function but
we only stream if streaming is on in create subscription command.  So
I feel that should be true with any plugin.

>
> > 3.
> > + XLogRecPtr prev_lsn = InvalidXLogRecPtr;
> >   ReorderBufferChange *change;
> >   ReorderBufferChange *specinsert = NULL;
> >
> > @@ -1565,6 +1965,16 @@ ReorderBufferCommit(ReorderBuffer *rb, TransactionId 
> > xid,
> >   Relation relation = NULL;
> >   Oid reloid;
> >
> > + /*
> > + * Enforce correct ordering of changes, merged from multiple
> > + * subtransactions. The changes may have the same LSN due to
> > + * MULTI_INSERT xlog records.
> > + */
> > + if (prev_lsn != InvalidXLogRecPtr)
> > + Assert(prev_lsn <= change->lsn);
> > +
> > + prev_lsn = change->lsn;
> > I did not understand, how this change is relavent to this patch
> >
>
> This is just to ensure that changes are in LSN order.  I think as we
> are merging the changes before commit for streaming, it is good to
> have such an Assertion for ReorderBufferStreamTXN.   And, if we want
> to have it in ReorderBufferStreamTXN, then there is no harm in keeping
> it in ReorderBufferCommit() at least to keep the code consistent.  Do
> you see any problem with this?
I am fine with this.
>
> > 4.
> > + /*
> > + * TOCHECK: We have to rebuild historic snapshot to be sure it includes all
> > + * information about subtransactions, which could arrive after streaming 
> > start.
> > + */
> > + if (!txn->is_schema_sent)
> > + snapshot_now = ReorderBufferCopySnap(rb, txn->base_snapshot,
> > + txn, command_id);
> >
> > In which case, txn->is_schema_sent will be true, because at the end of
> > the stream in ReorderBufferExecuteInvalidations we are always setting
> > it false,
> > so while sending next stream it will always be false.  That means we
> > never required snapshot_now variable in ReorderBufferTXN.
> >
>
> You are probably right, but as discussed we need to change this part
> of design/code (when to send schema changes) due to the issues
> discovered.  So, I think this part will anyway change when we fix that
> problem.
Make sense.
>
> > 5.
> > @@ -2299,6 +2746,23 @@ ReorderBufferXidSetCatalogChanges(ReorderBuffer
> > *rb, TransactionId xid,
> >   txn = ReorderBufferTXNByXid(rb, xid, true, NULL, lsn, true);
> >
> >   txn->txn_flags |= RBTXN_HAS_CATALOG_CHANGES;
> > +
> > + /*
> > + * We read catalog changes from WAL, which are not yet sent, so
> > + * invalidate current schema in order output plugin can resend
> > + * schema again.
> > + */
> > + txn->is_schema_sent = false;
> >
> > Same as point 4, during decode time it will never be true.
> >
>
> Sure, my previous point's reply applies here as well.
ok
>
> > 6.
> > + /* send fields */
> > + pq_sendint64(out, commit_lsn);
> > + pq_sendint64(out, txn->end_lsn);
> > + pq_sendint64(out, txn->commit_time);
> >
> > Commit_time and end_lsn is used in standby_feedback
> >
>
> I don't understand what you mean by this.  Can you be a bit more clear?
I think I paste it here by mistake.  just ignore it.
>
> >
> > 7.
> > + /* FIXME optimize the search by bsearch on sorted data */
> > + for (i = nsubxacts; i > 0; i--)
> > + {
> > + if (subxacts[i - 1].xid == subxid)
> > + {
> > + subidx = (i - 1);
> > + found = true;
> > + break;
> > + }
> > + }
> > We can not rollback intermediate subtransaction without rollbacking
> > latest sub-transaction, so why do we need
> > to search in the array?  It will always be the the last subxact no?
> >
>
> The same thing is already mentioned in the comments above this code
> ("XXX Or perhaps we can rely on the aborts to arrive in the reverse
> order, i.e. from the inner-

Let people set host(no)ssl settings from initdb

[David Fetter]

Folks,

I've found myself writing a lot of boilerplate pg_hba.conf entries
along the lines of

hostnosslall all 0.0.0.0/0  reject
hostssl  all all 0.0.0.0/0  md5

so I thought I'd make it easier to do that from initdb.

What say?

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
>From 5750bafe6169a239f6084d8d2e12b40da422fced Mon Sep 17 00:00:00 2001
From: David Fetter 
Date: Wed, 11 Dec 2019 18:55:03 -0800
Subject: [PATCH v1] Enable setting pg_hba.conf permissions from initdb
To: hackers
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="2.23.0"

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


The new optional arguments are --auth-hostssl and --auth-hostnossl

diff --git a/doc/src/sgml/ref/initdb.sgml b/doc/src/sgml/ref/initdb.sgml
index da5c8f5307..03b76732d1 100644
--- a/doc/src/sgml/ref/initdb.sgml
+++ b/doc/src/sgml/ref/initdb.sgml
@@ -154,6 +154,28 @@ PostgreSQL documentation
   
  
 
+ 
+  --auth-hostssl=authmethod
+  
+   
+This option specifies the authentication method for users via
+TLS connections used in pg_hba.conf
+(hostssl lines).
+   
+  
+ 
+
+ 
+  --auth-hostnossl=authmethod
+  
+   
+This option specifies the authentication method for users via
+non-TLS connections used in pg_hba.conf
+(hostnossl lines).
+   
+  
+ 
+
  
   --auth-local=authmethod
   
diff --git a/src/backend/libpq/pg_hba.conf.sample b/src/backend/libpq/pg_hba.conf.sample
index c853e36232..48ef465cca 100644
--- a/src/backend/libpq/pg_hba.conf.sample
+++ b/src/backend/libpq/pg_hba.conf.sample
@@ -87,3 +87,9 @@ hostall all ::1/128 @authmethodhost@
 @remove-line-for-nolocal@local   replication all @authmethodlocal@
 hostreplication all 127.0.0.1/32@authmethodhost@
 hostreplication all ::1/128 @authmethodhost@
+
+# hostnossl all   all 0.0.0.0/0   @authmethodhostnossl@
+# hostnossl all   all ::/0@authmethodhostnossl@
+
+# hostssl all all 0.0.0.0/0   @authmethodhostssl@
+# hostssl all all ::/0@authmethodhostssl@
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index 1f6d8939be..392f52d21f 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -136,6 +136,8 @@ static char *pwfilename = NULL;
 static char *superuser_password = NULL;
 static const char *authmethodhost = NULL;
 static const char *authmethodlocal = NULL;
+static const char *authmethodhostssl = NULL;
+static const char *authmethodhostnossl = NULL;
 static bool debug = false;
 static bool noclean = false;
 static bool do_sync = true;
@@ -438,7 +440,6 @@ replace_token(char **lines, const char *token, const char *replacement)
  *
  * a sort of poor man's grep -v
  */
-#ifndef HAVE_UNIX_SOCKETS
 static char **
 filter_lines_with_token(char **lines, const char *token)
 {
@@ -461,7 +462,6 @@ filter_lines_with_token(char **lines, const char *token)
 
 	return result;
 }
-#endif
 
 /*
  * get the lines from a text file
@@ -1326,6 +1326,40 @@ setup_config(void)
 			  "@authcomment@",
 			  (strcmp(authmethodlocal, "trust") == 0 || strcmp(authmethodhost, "trust") == 0) ? AUTHTRUST_WARNING : "");
 
+	if (authmethodhostssl != NULL)
+	{
+		conflines = replace_token(conflines,
+  "# hostssl all all 0.0.0.0/0   @authmethodhostssl@",
+  "hostssl all all 0.0.0.0/0   @authmethodhostssl@");
+		conflines = replace_token(conflines,
+  "# hostssl all all ::/0@authmethodhostssl@",
+  "hostssl all all ::/0@authmethodhostssl@");
+		conflines = replace_token(conflines,
+  "@authmethodhostssl@",
+  authmethodhostssl);
+	}
+	else
+	{
+		conflines = filter_lines_with_token(conflines, "@authmethodhostssl@");
+	}
+
+	if (authmethodhostnossl != NULL)
+	{
+		conflines = replace_token(conflines,
+  "# hostnossl all   all 0.0.0.0/0   @authmethodhostnossl@",
+  "hostnossl all   all 0.0.0.0/0   @authmethodhostnossl@");
+		conflines = replace_token(conflines,
+  "# hostnossl all   all ::/0@authmethodhostnossl@",
+  "hostnossl all   all ::/0@authmethodhostnossl@");
+		conflines = replace_token(conf

Re: Collation versioning

[Thomas Munro]

On Thu, Dec 12, 2019 at 2:45 AM Julien Rouhaud  wrote:
> Hearing no objection in [1], attached v5 with following changes:
>
> - fix the spurious warnings by doing the version check in
> get_relation_info and vacuum_open_relation, and add a flag in
> RelationData to mark the check as already being done
> - change the IsCatalogRelation() check to IsSystemRelation() to also
> ignore toast indexes, as those can also be safely ignored too
> - add a new ALTER INDEX idxname DEPENDS ON COLLATION collname CURRENT
> VERSION to let users remove the warnings for safe library upgrade.
> Documentation and tab completion added
>
> If I'm not mistaken, all issues I was aware of are now fixed.

Thanks!  This is some great progress and I'm feeling positive about
getting this into PostgreSQL 13.  I haven't (re)reviewed the code yet,
but I played with it a bit and have some more feedback.

There are some missing semi-colons on the ALTER INDEX statements in
pg_dump.c that make the pg_upgrade test fail (at least, if LC_ALL is
set).

We create duplicate pg_depend records:

postgres=# create table t (x text);
CREATE TABLE
postgres=# create index on t(x) where x > 'hello';
CREATE INDEX
postgres=# select * from pg_depend where objid = 't_x_idx'::regclass
and refobjversion != '';
 classid | objid | objsubid | refclassid | refobjid | refobjsubid |
refobjversion | deptype
-+---+--++--+-+---+-
1259 | 16424 |0 |   3456 |  100 |   0 |
0:34.0| n
1259 | 16424 |0 |   3456 |  100 |   0 |
0:34.0| n
(2 rows)

I wondered if that was harmless, but for one thing it causes duplicate warnings:

postgres=# update pg_depend set refobjversion = 'BANANA' where
refobjversion = '0:34.0';
UPDATE 2

[new session]
postgres=# select count(*) from t;
WARNING:  index "t_x_idx" depends on collation "default" version
"BANANA", but the current version is "0:34.0"
DETAIL:  The index may be corrupted due to changes in sort order.
HINT:  REINDEX to avoid the risk of corruption.
WARNING:  index "t_x_idx" depends on collation "default" version
"BANANA", but the current version is "0:34.0"
DETAIL:  The index may be corrupted due to changes in sort order.
HINT:  REINDEX to avoid the risk of corruption.

Here's another way to get a duplicate, and in this example you also
get an unnecessary dependency on 100 "default" for this index:

postgres=# create index on t(x collate "fr_FR") where x > 'helicopter'
collate "fr_FR";
CREATE INDEX
postgres=# select * from pg_depend where objid = 't_x_idx'::regclass
and refobjversion != '';
 classid | objid | objsubid | refclassid | refobjid | refobjsubid |
refobjversion | deptype
-+---+--++--+-+---+-
1259 | 16460 |0 |   3456 |12603 |   0 |
0:34.0| n
1259 | 16460 |0 |   3456 |12603 |   0 |
0:34.0| n
1259 | 16460 |0 |   3456 |  100 |   0 |
0:34.0| n
(3 rows)

Or... maybe 100 should be there, by simple analysis of the x in the
WHERE clause, but it's the same if you write x collate "fr_FR" >
'helicopter' collate "fr_FR", and in that case there are no
expressions of collation "default" anywhere.

The indirection through composite types works nicely:

postgres=# create type foo_t as (en text collate "en_CA", fr text
collate "fr_CA");
CREATE TYPE
postgres=# create table t (foo foo_t);
CREATE TABLE
postgres=# create index on t(foo);
CREATE INDEX
postgres=# select * from pg_depend where objid = 't_foo_idx'::regclass
and refobjversion != '';
 classid | objid | objsubid | refclassid | refobjid | refobjsubid |
refobjversion | deptype
-+---+--++--+-+---+-
1259 | 16444 |0 |   3456 |12554 |   0 |
0:34.0| n
1259 | 16444 |0 |   3456 |12597 |   0 |
0:34.0| n
(2 rows)

... but again it shows the extra and technically unnecessary
dependencies (only 12603 "fr_FR" is really needed):

postgres=# create index on t(((foo).fr collate "fr_FR"));
CREATE INDEX
postgres=# select * from pg_depend where objid = 't_fr_idx'::regclass
and refobjversion != '';
 classid | objid | objsubid | refclassid | refobjid | refobjsubid |
refobjversion | deptype
-+---+--++--+-+---+-
1259 | 16445 |0 |   3456 |12603 |   0 |
0:34.0| n
1259 | 16445 |0 |   3456 |12597 |   0 |
0:34.0| n
1259 | 16445 |0 |   3456 |12554 |   0 |
0:34.0| n
(3 rows)

I check that nested types are examined recursively, as appropriate.  I
also tested domains, arrays, arrays of domains, expressions extracting
an element from an array of a domain with an explicit collation, and

Re: Session WAL activity

[Craig Ringer]

On Fri, 6 Dec 2019 at 09:57, Michael Paquier  wrote:

> On Thu, Dec 05, 2019 at 12:23:40PM +0300, Konstantin Knizhnik wrote:
> > Concerning keeping PGPROC size as small as possible, I agree that it is
> > reasonable argument.
> > But even now it is very large (816 bytes) and adding extra 8 bytes will
> > increase it on less than 1%.
>
> It does not mean that we should add all kind of things to PGPROC as
> that's a structure sensitive enough already.


Right. It's not as critical as PGXACT, but PGPROC is still significant for
scalability and connection count limits.

It's a shame we can't really keep most of it in backend-private memory and
copy it to requestors when they want it, say into a temporary DSA or over a
shm_mq. But our single threaded model means we just cannot rely on backends
being responsive in a timely enough manner to supply data on-demand. That
doesn't mean we have to push it to PGPROC though: we could be sending the
parts that don't have to be super-fresh to the stats collector or a new
related process for active session stats and letting it aggregate them.

That's way beyond the scope of this patch though. So personally I'm OK with
the new PGPROC field. Visibility into Pg's activity is woefully limited and
something we need to prioritize more.

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 2ndQuadrant - PostgreSQL Solutions for the Enterprise

Re: get_database_name() from background worker

[Craig Ringer]

On Wed, 11 Dec 2019 at 14:38, Koichi Suzuki  wrote:

> Hello PG hackers;
>
> I'm writing an extension running on background workers and found
> get_database_name() causes SEGV and found internally resource owner was wet
> to NULL.   Could anybody let me know how it happens and how I can use this
> function.   Argument to get_database_name() looks correct.
>
>
I think the main question is answered; if the advice given does not help
please supply your code and a backtrace from the crash obtained from a core
file.

However, this reminds me of something. I'd like to make our
syscache/relcache/catcache and all snapshot access functions
Assert(IsTransactionState()); directly or at key locations. That'd make
these mistakes much more obvious - and as bgworkers become a more popular
way to write code for PostgreSQL that's going to be important.

Similarly, it might make sense to assert that we have a valid snapshot in
the SPI, which we don't presently do for read-only SPI calls. I recall that
one biting me repeatedly when I was learning this stuff.

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 2ndQuadrant - PostgreSQL Solutions for the Enterprise

RE: get_database_name() from background worker

[tsunakawa.ta...@fujitsu.com]

From: Koichi Suzuki 
> I'm not using this.   Is this the must to use get_database_name()?

I don't think pg_background is a must, but the system catalog access by 
get_database_name() should require database connection and transaction.  See 
src/test/modules/worker_spi/worker_spi.c for an example of background worker.  
That uses both of them.


Regards
Takayuki Tsunakawa

Re: Runtime pruning problem

[Tom Lane]

I wrote:
> OK, so here's a finished set of patches for this issue.
> 0001 is the same patch I posted on Tuesday; I kept it separate just
> because it seemed like a largely separable set of changes.  (Note that
> the undesirable regression test output changes are undone by 0002.)
> 0002 implements the map-vars-back-to-the-inheritance parent change
> per my sketch.  Notice that relation aliases and Var names change
> underneath Appends/MergeAppends, but Vars above one are (mostly)
> printed the same as before.  On the whole I think this is a good
> set of test output changes, reflecting a more predictable approach
> to assigning aliases to inheritance children.  But somebody else
> might see it differently I suppose.
> Finally, 0003 is the remaining portion of David's patch to allow
> deletion of all of an Append/MergeAppend's sub-plans during
> executor startup pruning.

I pushed these, and the buildfarm immediately got a bad case of
the measles.  All the members using force_parallel_mode = regress
fail on the new regression test case added by 0003, with diffs
like this:

diff -U3 
/home/pgbf/buildroot/HEAD/pgsql.build/src/test/regress/expected/partition_prune.out
 
/home/pgbf/buildroot/HEAD/pgsql.build/src/test/regress/results/partition_prune.out
--- 
/home/pgbf/buildroot/HEAD/pgsql.build/src/test/regress/expected/partition_prune.out
 Thu Dec 12 00:40:04 2019
+++ 
/home/pgbf/buildroot/HEAD/pgsql.build/src/test/regress/results/partition_prune.out
  Thu Dec 12 00:45:44 2019
@@ -3169,10 +3169,12 @@
 
  Limit (actual rows=0 loops=1)
Output: ma_test.a, ma_test.b
+   Worker 0: actual rows=0 loops=1
->  Merge Append (actual rows=0 loops=1)
  Sort Key: ma_test.b
+ Worker 0: actual rows=0 loops=1
  Subplans Removed: 3
-(5 rows)
+(7 rows)
 
 deallocate mt_q2;
 reset plan_cache_mode;

This looks to me like there's some other part of EXPLAIN that
needs to be updated for the possibility of zero child nodes, but
I didn't find out just where in a few minutes of searching.

As a stopgap to get back to green buildfarm, I removed this
specific test case, but we need to look at it closer.

regards, tom lane

Re: Corruption with duplicate primary key

[Alex Adriaanse]

On Mon, December 9, 2019 at 11:05 AM Finnerty, Jim wrote:
> If you have BEFORE triggers, and a BEFORE trigger signaled failure with 
> RETURN NULL, then this is one known (and documented) issue that I think could 
> cause the behavior you're reporting:
>     
> https://www.postgresql-archive.org/BEFORE-triggers-that-return-NULL-can-circumvent-referential-integrity-tt6056390.html#none
> 
> It's hard to say if this is the cause or not, but if you have any BEFORE 
> triggers that RETURN NULL, you might want to review the documentation very 
> carefully.

We do have a BEFORE INSERT trigger, but it should never return NULL. This 
trigger INSERTs into a different table using an ON CONFLICT DO NOTHING clause 
and then does a RETURN NEW.

Alex

Re: Corruption with duplicate primary key

[Alex Adriaanse]

On Thu., December 5, 2019 at 5:45 PM, Tomas Vondra wrote:
> At first I thought maybe this might be due to collations
> changing and breaking the index silently. What collation are you using?

We're using en_US.utf8. We did not make any collation changes to my knowledge.

> 1) When you do the queries, do they use index scan or sequential scan?
> Perhaps it does sequential scan, and if you force index scan (e.g. by
> rewriting the query) it'll only find one of those rows.

By default it used an index scan. When I re-ran the query today (and confirmed 
that the query used an index only scan) I did not see any duplicates. If I 
force a sequential scan using "SET enable_index[only]scan = false" the 
duplicates reappear.

However, using a backup from a week ago I see duplicates in both the query that 
uses an index only scan as well as the query that uses the sequential scan. So 
somehow over the past week the index got changed to eliminate duplicates.

> 2) Can you check in backups if this data corruption was present in the
> PG10 cluster, before running pg_upgrade? 

Sure. I just checked and did not see any corruption in the PG10 pre-upgrade 
backup. I also re-upgraded that PG10 backup to PG12, and right after the 
upgrade I did not see any corruption either. I checked using both index scans 
and sequential scans.

Alex

Re: Corruption with duplicate primary key

[Alex Adriaanse]

On Thu, December 5, 2019 at 5:34 PM Peter Geoghegan wrote:
> > We have a Postgres 10 database that we recently upgraded to Postgres 12 
> > using pg_upgrade. We recently discovered that there are rows in one of the 
> > tables that have duplicate primary keys:
> 
> What's the timeline here? In other words, does it look like these rows
> were updated and/or deleted before, around the same time as, or after
> the upgrade?

The Postgres 12 upgrade was performed on 2019-11-22, so the affected rows were 
modified after this upgrade (although some of the rows were originally inserted 
before then, before they were modified/duplicated).

> > This database runs inside Docker, with the data directory bind-mounted to a 
> > reflink-enabled XFS filesystem. The VM is running Debian's 4.19.16-1~bpo9+1 
> > kernel inside an AWS EC2 instance. We have Debezium stream data from this 
> > database via pgoutput.
> 
> That seems suspicious, since reflink support for XFS is rather immature.

Good point. Looking at kernel commits since 4.19.16 it appears that there have 
been a few bug fixes in later kernel versions that address a few XFS corruption 
issues. Regardless of whether FS bugs are responsible of this corruption I'll 
plan on upgrading to a newer kernel.

> How did you invoke pg_upgrade? Did you use the --link (hard link) option?

Yes, we first created a backup using "cp -a --reflink=always", ran initdb on 
the new directory, and then upgraded using "pg_upgrade -b ... -B ... -d ... -D 
-k".

Alex

non-exclusive backup cleanup is mildly broken

[Robert Haas]

While reviewing the first patch in Asif Rehman's series of patches for
parallel backup over at
http://postgr.es/m/CADM=Jeg3ZN+kPQpiSfeWCXr=xgplrq4cbqe5zviuxygkq3v...@mail.gmail.com
I discovered that commit 7117685461af50f50c03f43e6a622284c8d54694
introduced a use of cancel_before_shmem_exit which falsifies the
comment for that function. So you can cause a spurious WARNING in the
logs by doing something like this, with max_prepared_transactions set
to a non-zero value:

select pg_start_backup('one', false, false);
begin;
prepare transaction 'nothing';
select pg_stop_backup(false);
\q

in the server log:
WARNING:  aborting backup due to backend exiting before pg_stop_backup
was called

And you can cause an assertion failure like this:

select pg_start_backup('one', false, false);
begin;
prepare transaction 'nothing';
select pg_stop_backup(false);
select pg_start_backup('two');
\q

We've discussed before the idea that it might be good to remove the
limitation that before_shmem_exit() can only remove the
most-recently-added callback, which would be one way to fix this
problem, but I'd like to propose an alternative solution which I think
will work out more nicely for the patch mentioned above: don't use
cancel_before_shmem_exit, and just leave the callback registered for
the lifetime of the backend. That requires some adjustment of the
callback, since it needs to tolerate exclusive backup mode being in
effect.

The attached patch takes that approach. Thoughts welcome on (1) the
approach and (2) whether to back-patch. I think there's little doubt
that this is formally a bug, but it's a pretty minor one.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


0001-Remove-misuse-of-cancel_before_shmem_exit.patch
Description: Binary data

Re: [Proposal] Level4 Warnings show many shadow vars

[Stephen Frost]

Greetings,

* Ranier Vilela (ranier_...@hotmail.com) wrote:
> 1.So I would like to ask you if at least what has consensus could be used.
> Or is it better to leave everything as it is?

As I tried to say before- I'm all for working to eliminate the shadow
variables, but it should be on a case-by-case basis where the change
proposed is a well considered change by someone who has taken the time
to understand what the code is doing, looked at the call sites, made a
well reasoned argument for why the change is an improvement and reduces
confusion (which can't just be "because the compiler said we should make
this change"- compilers aren't nearly intelligent enough to give us the
right answer about what the code should look like- they can only point
out potential issues, and they're often bad at even doing that), and
then proposed a patch for each particular case where the patch is
addressing a specific set of shadow variable cases that somehow go
together.

A patch to address the numTables issues in pg_dump would be great, for
example.  A single patch that renames numTables in pg_dump and then
makes a bunch of completely unrelated changes to things in the backend
isn't what I'd consider a reasonable grouping of changes.

> 2.About local shadow variables, would you find it safe to do redundant 
> declaration removals of the type: int i? Is it worth it to work on that?

I really don't think you're going to find much in the PG code where
there would be general consensus of a broad renaming or modifying of
variables without having to put serious thought into the specific
change.

At least, I hope people would push back on that kind of rote change.

In other words, without looking at the specific cases you're talking
about, I don't know if I'd agree with them or not, but please don't just
submit patches that just rename things without having looked at the
code, gained some understanding of what the code does, and considered if
the change you want to make is a well reasoned improvement and makes the
code easier to read and understand.

Thanks,

Stephen


signature.asc
Description: PGP signature

RE: [Proposal] Level4 Warnings show many shadow vars

[Ranier Vilela]

De: Stephen Frost
Enviadas: Quarta-feira, 11 de Dezembro de 2019 18:57
>I really don't have any doubts that it's going to lead to confusion,
>particularly in a case like the numTables vs. nTables thing you're
>proposing in the one case that I spent some time looking at, and that
>confusion certainly could lead to bugs.  Sure, having shadow variables
>also could- but you haven't identified an actual bug there today, so why
>not just fix it in a way that eliminates the confusion here?
I'm starting to think you're absolutely right.

>Here's an example of my concern- we change the name of the numTables
>variable in these pg_dump functions to nTables as you propose...  And
>then later on someone starts hacking on these functions and they know
>about the global and they start using it, so now we've got two
>variables, both able to be used in the same scope, but one of them is a
>global and the other is a local.  As long as both are always the same,
>sure everything works- but what happens if, for whatever reason, someone
>uses the function in a new way and passes in a different value as an
>argument, one that doesn't match what the global has?  Well, some of the
>code will use the argument, and some of the code won't.  At least today,
>there's no chance that the global variable will be used inside that
>function- it's *always* going to use the argument passed in.
Understood.

>I don't think that's even that far-fetched of a possibility considering
>most of the code is using the global variable directly and these
>functions are really the odd ones where numTables is being passed in as
>an argument, so ending up with a mix in the function looks rather likely
>to happen, and a bug resulting from that inconsistency entirely
>possible.
Yes.

>It's also possbile that the changes you're proposing might themselves
>induce bugs- by keeping the variable and just renaming it, you had
>better be ABSOLUTELY sure you rename every case because, if you don't,
>everything will still work just *fine*, except where you missed a case,
>the code will reference the global and the compiler won't complain and
>it might very well look like everything is working.
I can tell you that I tried to take every precaution in that direction.
But it is really not exempt from this risk.

>Either way, in my view, you had better review the code, have an
>understanding of how it works, and make sure that the change you're
>making is correct and makes sense, and that you've tested it well.
This view is very correct.

>Then I'd suggest that you spend some time looking at each case and
>working through what the best approach is and proposing patches that use
>the best approach in each case.  If you don't wish to spend time on
>that, that's fine, but I don't agree with this approach of just pushing
>through and making things changes just to satisfy a particular compiler
>warning.  I don't anticipate further discussion on this changing my mind
>on this point.

>I've already pointed out why I don't think this is the right approach to
>be addressing these issues, and it seems that you don't disagree with me
>about the recommended changes I've suggested, you've just said that you
>only want to think about or care about renaming of variables and I am
>specifically saying that's not an acceptable approach to addressing
>these issues.

>Why is that an issue?
It's not anymore.

>Feel free to monitor the situation and complain about new patches which
>are proposed that add to them.  I don't have any particular problem with
>that.  Nor do I object to generally pushing forward with the goal of
>eliminating the existing ones.
I would better it safer to do this automatically in the course of development.
Alone, the risk of failure is high.

>Let me lay this out in a different way- we could do the exact same thing
>you're doing here by just mindlessly changing, right before we commit,
>any variables that shadow global variables, we'd eliminate the compiler
>error, but it doesn't directly make anything *better* by itself, and
>ultimately isn't really all that different from the current situation
>where the compiler is essentially doing this for us by manging the
>variables as shadowing the globals thanks to C scoping rules, except
>that we add in the possibility of mixing usage of the local and the
>global throughout the functions therefore adding to the confusion.
This is the light.

>That doesn't actually make any of them bugs though.
Truth.

>I'm all for working to eliminate these shadow variables, but, again, not
>through rote renaming of the locals without putting in any real thought
>about what the code is doing and working out what the right approach to
>such a change to eliminate the shadow variables should be.
>Ok, so this specific "global unshadow patch" is all about bugs, but
>without actually showing that there's actual bugs here, just that there
>are shadowed variables... In which case, the real question is "is this
>change an improvement to the code" and 

Re: Windows buildfarm members vs. new async-notify isolation test

[Tom Lane]

Amit Kapila  writes:
> I am convinced by your points.  So +1 for your proposed patch.  I have
> already reviewed it yesterday and it appears fine to me.

OK, pushed.  Thanks for reviewing!

regards, tom lane

Re: [Proposal] Level4 Warnings show many shadow vars

[Stephen Frost]

Greetings,

* Ranier Vilela (ranier_...@hotmail.com) wrote:
> De: Stephen Frost
> Enviadas: Quarta-feira, 11 de Dezembro de 2019 15:34
> 
> >I agree with not breaking things but that doesn't mean the only
> >reasonable approach is to do the absolute minimum- you might not be
> >breaking something today, but it's going to confuse people later on down
> >the road and may lead to bugs being introduced due to that confusion, or
> >at the very least will add to people's time to figure out what's really
> >going on.
> I don't know how such fixes could lead to more bugs.
> Currently there is a risk of having bugs by mixing access to shadow variables 
> with macros.

I really don't have any doubts that it's going to lead to confusion,
particularly in a case like the numTables vs. nTables thing you're
proposing in the one case that I spent some time looking at, and that
confusion certainly could lead to bugs.  Sure, having shadow variables
also could- but you haven't identified an actual bug there today, so why
not just fix it in a way that eliminates the confusion here?

Here's an example of my concern- we change the name of the numTables
variable in these pg_dump functions to nTables as you propose...  And
then later on someone starts hacking on these functions and they know
about the global and they start using it, so now we've got two
variables, both able to be used in the same scope, but one of them is a
global and the other is a local.  As long as both are always the same,
sure everything works- but what happens if, for whatever reason, someone
uses the function in a new way and passes in a different value as an
argument, one that doesn't match what the global has?  Well, some of the
code will use the argument, and some of the code won't.  At least today,
there's no chance that the global variable will be used inside that
function- it's *always* going to use the argument passed in.

I don't think that's even that far-fetched of a possibility considering
most of the code is using the global variable directly and these
functions are really the odd ones where numTables is being passed in as
an argument, so ending up with a mix in the function looks rather likely
to happen, and a bug resulting from that inconsistency entirely
possible.

It's also possbile that the changes you're proposing might themselves
induce bugs- by keeping the variable and just renaming it, you had
better be ABSOLUTELY sure you rename every case because, if you don't,
everything will still work just *fine*, except where you missed a case,
the code will reference the global and the compiler won't complain and
it might very well look like everything is working.

Either way, in my view, you had better review the code, have an
understanding of how it works, and make sure that the change you're
making is correct and makes sense, and that you've tested it well.

> >I wasn't suggesting to change the names of the global variables in this
> >specific case, though I could see that being a better approach in some
> >instances- but it really depends.  Each case needs to be reviewed and
> >considered and the best approach taken.
> Again, depending on the case, whether the best approach is to promote 
> structure creation, variable removal, and logic changes, for now, is really 
> beyond my reach.

Then I'd suggest that you spend some time looking at each case and
working through what the best approach is and proposing patches that use
the best approach in each case.  If you don't wish to spend time on
that, that's fine, but I don't agree with this approach of just pushing
through and making things changes just to satisfy a particular compiler
warning.  I don't anticipate further discussion on this changing my mind
on this point.

> >I think we need to be looking at the changes and considering them, and
> >the person proposing the changes should be doing that and not just
> >expecting everyone else to do so.
> Again, I am considering only the range of my changes, which are minimal, so 
> less likely to do something wrong, or hinder future development.

I've already pointed out why I don't think this is the right approach to
be addressing these issues, and it seems that you don't disagree with me
about the recommended changes I've suggested, you've just said that you
only want to think about or care about renaming of variables and I am
specifically saying that's not an acceptable approach to addressing
these issues.

> >I'd suggest that we work through that then and get you up to speed on
> >the structures and logic- the pg_dump code is pretty ugly but the
> >specific usage of numTables isn't too bad.  Each of these should be
> >looked at independently and thought about "what's the right way to fix
> >this?"  The right way isn't necessairly to just rename the variables, as
> >I was saying, and doing so may lead to more confusion, not less.
> This way it will take a long time to eliminate all name collisions.

Why is that an issue?

> And worse, in my opin

Re: global / super barriers (for checksums)

[Robert Haas]

On Wed, Dec 11, 2019 at 12:38 PM Andres Freund  wrote:
> I just don't buy this argument. There's a difference in between an
> unpatched version of postgres suddenly potentially running hooks
> everywhere CFI() etc is called, and some user patching postgres to
> behave differently. In the former case we'll have to ask to reproduce
> problems without extension in a lot more cases.  For me code like this
> that runs in pretty low level situations that we've gotten wrong more
> than once, doesn't benefit by being extensible. We just make things more
> fragile, and provide traps for extension authors.

It seems to me that this amounts to an argument that (a) core
developers are smarter than extension developers, and are thus the
only ones entitled to play with sharp tools, and/or (b) core
developers are more important than extension developers, and thus
inconveniencing extension developers is OK if it makes life better for
core developers. Both propositions seem pretty arrogant to me.

> But that's just not how it ends up working in a lot of cases? People
> still report bugs to the list, and the debugging experience of problems
> where an extension causes crashes-at-a-distance is pretty bad.

I agree that crashes at a distance are bad, and that those caused by
extensions are more annoying than those caused by core code, because
in the latter case there is a silent contributor to the problem about
which we may have little information. However, I disagree with the
idea that the right solution is to try to lock things down so that
extension authors can't do stuff. Extensibility is an important part
of what has made PostgreSQL successful, and I think we need more of
it, not less.

We can to some extent mitigate these kinds of problems by adding
assertions to our code that will catch usage errors, which has the
advantage that those things also get caught when somebody accidentally
introduces such bugs into core. To the extent that we can't mitigate
them, I think we should live with them, because I think extensibility
is far too valuable to cast aside over such concerns.

While I have passionate philosophical feelings about this topic, for
purposes of the present thread the really important question (IMV,
anyway) is whether there's any way of getting a patch for global
barriers committed in advance of the first user of such barriers. Both
your version and mine add an enum, and I think that you can't have an
enum with no elements. If you have a suggestion as to how we can
structure things so that we can start out with 0 users of this
mechanism rather than needing to start out with 1, I'd like to hear
them. If not, then I guess we'll need to decide which of checksum
enable/disable and ALTER SYSTEM READ ONLY is going to go first and
commit this only when that patch is ready to go as well. Or, I
suppose, commit it with a dummy placeholder that then gets replaced by
whichever patch goes first, but I'm not sure whether people would be
OK with that.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [Proposal] Level4 Warnings show many shadow vars

[John W Higgins]

On Wed, Dec 11, 2019 at 9:46 AM Ranier Vilela 
wrote:

> I am the author of the patch.
> I'm repeating myself, but come on, I don't have confidence in proposing
> logic-altering changes for now.
>
>
Then you need to stop and patch the holes and not just throw paint on the
wall to cover things up.

Specific example

src/bin/pgdump/common.c

That file discusses the purpose of numTables

 * These variables are static to avoid the notational cruft of having to
pass
 * them into findTableByOid() and friends.

Then the file goes and doesn't follow that logic by passing numTables
around to a bunch of functions within itself.

The fix here, very much appears to be to remove the spurious numTables in
the functions.

However, if you cannot, or will not, take the opportunity to correct it
properly - as has been asked earlier for this specific file - then please
just leave it alone.

There have been plenty of emails on these threads where folks have looked
at your work and discussed whether or not specific things should be changed
based on your analysis - that's an amazing thing to see occur - but that's
getting overwhelmed by your inability to take a step back and stop just
throwing stuff on the wall.

I've mentioned inconsistencies in your patches - that is a product of just
trying to throw something on the wall to cover over the issue - hiding a
hole in the wall with something doesn't remove the hole in the wall.

You would be so much better off taking on one specific instance at a time
and working with folks to learn how the code functions. If you don't think
you can handle the bigger issues - then stick with things like numTables
and the clear issues within your grasp first.

I truly do wish you all the best - but you do not seem to be approaching
these issues with the correct mindset at the moment. Volume is not the
winner over quality here.

John W Higgins

>

Re: PATCH: logical_work_mem and logical streaming of large in-progress transactions

[Robert Haas]

On Mon, Dec 2, 2019 at 3:32 AM Dilip Kumar  wrote:
> I have rebased the patch set on the latest head.

0001 looks like a clever approach, but are you sure it doesn't hurt
performance when many small XLOG records are being inserted? I think
XLogRecordAssemble() can get pretty hot in some workloads.

With regard to 0002, logging a separate WAL record for each
invalidation seems painful; I think most operations that generate
invalidations generate a bunch of them all at once. Perhaps you could
just queue up invalidations as they happen, and then force anything
that's been queued up to be emitted into WAL just before you emit any
WAL record that might need to be decoded.

Regarding 0005, it seems to me that this is no good:

+ errmsg("improper heap_getnext call")));

I think we should be using elog() rather than ereport() here, because
this should only happen if there's a bug in a logical decoding plugin.
At first, I thought maybe this should just be an Assert(), but since
there are third-party logical decoding plugins available, checking
this even in non-assert builds seems like a good idea. However, I
think making it translatable is overkill; users should never see this,
only developers.

I also think that the message is really bad, because it just tells you
did something bad. It gives no inkling as to why it was bad.

0006 contains lots of XXX comments that look like real issues. I guess
those need to be fixed. Also, why don't we do the thing that the
commit message for 0006 says we could "theoretically" do? I don't
understand why we need the k-way merge at all,

+ if (prev_lsn != InvalidXLogRecPtr)
+ Assert(prev_lsn <= change->lsn);

There is no reason to ever write an if statement that contains only an
Assert, and it's bad style. Write Assert(prev_lsn == InvalidXLogRecPtr
|| prev_lsn <= change->lsn), or better yet, use XLogRecPtrIsInvalid.

The purpose and mechanism of the is_schema_sent flag is not clear to
me. The word "schema" here seems to be being used to mean "snapshot,"
which is rather confusing.

I'm also somewhat unclear on what's happening here with invalidations.
Perhaps that's as much a defect in my understanding as it is
reflective of any problem with the patch, but I also don't see any
comments either in 0002 or later patches explaining the theory of
operation. If I've missed some, please point me in the right
direction. Hypothetically speaking, it seems to me that if you just
did InvalidateSystemCaches() every time the snapshot changed, you
wouldn't need anything else (unless we're concerned with
non-transactional invalidation messages like smgr and relmapper
invalidations; not quite sure how those are handled). And, on the
other hand, if we don't do InvalidateSystemCaches() every time the
snapshot changes, then I don't understand why this works now, even
without streaming.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Add .editorconfig

[Andreas Karlsson]

I have not used .editorconfig that much, but would it makes sense to add 
the below?


[*]
end_of_line = lf

--
Andreas Karlsson

RE: [Proposal] Level4 Warnings show many shadow vars

[Ranier Vilela]

De: Stephen Frost
Enviadas: Quarta-feira, 11 de Dezembro de 2019 15:34

>I agree with not breaking things but that doesn't mean the only
>reasonable approach is to do the absolute minimum- you might not be
>breaking something today, but it's going to confuse people later on down
>the road and may lead to bugs being introduced due to that confusion, or
>at the very least will add to people's time to figure out what's really
>going on.
I don't know how such fixes could lead to more bugs.
Currently there is a risk of having bugs by mixing access to shadow variables 
with macros.
I believe, that others has the same opinion.
https://www.postgresql.org/message-id/CA%2BTgmoZsM04VCKn4n8dsXxg_s8drPUHUafshG%3DP0edVjUS3Gew%40mail.gmail.com
and
https://www.postgresql.org/message-id/20191209090329.GC72921%40paquier.xyz

>I wasn't suggesting to change the names of the global variables in this
>specific case, though I could see that being a better approach in some
>instances- but it really depends.  Each case needs to be reviewed and
>considered and the best approach taken.
Again, depending on the case, whether the best approach is to promote structure 
creation, variable removal, and logic changes, for now, is really beyond my 
reach.

>I think we need to be looking at the changes and considering them, and
>the person proposing the changes should be doing that and not just
>expecting everyone else to do so.
Again, I am considering only the range of my changes, which are minimal, so 
less likely to do something wrong, or hinder future development.

>I'd suggest that we work through that then and get you up to speed on
>the structures and logic- the pg_dump code is pretty ugly but the
>specific usage of numTables isn't too bad.  Each of these should be
>looked at independently and thought about "what's the right way to fix
>this?"  The right way isn't necessairly to just rename the variables, as
>I was saying, and doing so may lead to more confusion, not less.
This way it will take a long time to eliminate all name collisions.
And worse, in my opinion, will continue to be adding new cases, since there is 
no rule, so check if this happens in the current development.
Not only are they global, there are dozens, perhaps hundreds of shadow local 
variables.
I was working on this second class of variables, which, in my opinion, would 
lead to less code, less bugs, and more security for the code, but I realize 
that my effort may not be worth it.

>Having shadowed globals, while kinda ugly, doesn't necessairly mean it's
>a bug.  I'm not sure what "minor performance improvements" are being
>claimed here but there's a whole lot of work involved in demonstrating
>that a change is a performance improvement.
I was referring to contributions like this:
https://github.com/postgres/postgres/commit/91da65f4ac2837e0792071e42b2e2101059f1b1b
and not specifically, performance improvements in this global unshadow patch.

>but.. the changes you're proposing are making them inconsistent and
>confusing when there isn't actually a difference between the global and
>the local, it's just the somewhere along the way someone thought they
>needed to pass in numTables when they really didn't, and we should go
>fix *that*, not rename the variable to something else to make someone
>later on go "wait, why did we need to pass in this variable?  how is
>this different from the global?"
I'm confused here, but if you suggest removing the numTables variable is out of 
my reach.
Keeping the same name as the variable, the collision will continue, and the 
purpose of enabling -Wshadow will never be done someday.

>Perhaps I'm a bit confused, but it seems that you're the author of these
>specific changes, and I'm trying to provide feedback as to how you can
>improve what you're proposing in a way that will improve the code base
>overall and reduce the confusion while also eliminating the shadow
>variables.  If the author of a patch isn't open to this kind of review
>and willing to adjust the patch to improve it because they aren't sure
>that their changes will be correct then they could at least post them
>back here and ask, or better, go look at the code and get a better
>understanding of what's going on to build confidence in the change.
I am the author of the patch.
I'm repeating myself, but come on, I don't have confidence in proposing 
logic-altering changes for now.

>The goal here also shouldn't be "we just want to turn on this alert, so
>we're going to make changes to the source without thinking just to
>appease the compiler".
Of course there is a difference in thinking here.
The changes I propose, in my opinion, are consistent, readable and break 
nothing.

>I agree with fixing collisions, but not in a rote way like this.
Glad you think alike about fixing collisions.

regards,
Ranier Vilela

Re: global / super barriers (for checksums)

[Andres Freund]

Hi,

On 2019-12-11 09:12:49 -0500, Robert Haas wrote:
> On Mon, Dec 9, 2019 at 7:37 PM Daniel Gustafsson  wrote:
> > I sort of like the callback idea conceptually, but Andres is making a good
> > point about the extensibility actually making it harder to reason about.
> 
> That objection doesn't hold any water for me, because this is open
> source. People can always patch the core.

I just don't buy this argument. There's a difference in between an
unpatched version of postgres suddenly potentially running hooks
everywhere CFI() etc is called, and some user patching postgres to
behave differently. In the former case we'll have to ask to reproduce
problems without extension in a lot more cases.  For me code like this
that runs in pretty low level situations that we've gotten wrong more
than once, doesn't benefit by being extensible. We just make things more
fragile, and provide traps for extension authors.


> If we don't add hooks, then we make life easier for fork maintainers
> (including my employer) and harder for extension authors. I think
> that's *definitely* the wrong bet for the community to be making; we
> should be trying very hard to help extension authors and minimize the
> need for forks. If you install an extension that uses a hook, any
> hook, and it breaks things, then you get to keep both pieces.

But that's just not how it ends up working in a lot of cases? People
still report bugs to the list, and the debugging experience of problems
where an extension causes crashes-at-a-distance is pretty bad.

Greetings,

Andres Freund

Re: logical decoding bug: segfault in ReorderBufferToastReplace()

[Andres Freund]

Hi,

On 2019-12-11 12:11:03 -0500, Tom Lane wrote:
> I don't think you can make that conclusion.  Perhaps the table once
> needed a toast table because of some wide column that got dropped;
> if so, it'd still have one.  It'd be safer to look at
> pg_class.reltoastrelid to verify existence (or not) of the toast relation.

That was checked in the email I was responding to.


> It strikes me that there could easily be cases where a publisher table
> has a toast relation and a subscriber's doesn't ... maybe this code
> isn't expecting that?

This code is all running on the publisher side, so I don't think it
could matter.

Greetings,

Andres Freund

Re: logical decoding bug: segfault in ReorderBufferToastReplace()

[Robert Haas]

On Wed, Dec 11, 2019 at 12:11 PM Tom Lane  wrote:
> I don't think you can make that conclusion.  Perhaps the table once
> needed a toast table because of some wide column that got dropped;
> if so, it'd still have one.  It'd be safer to look at
> pg_class.reltoastrelid to verify existence (or not) of the toast relation.

I believe that output was already shown earlier in the thread.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: logical decoding bug: segfault in ReorderBufferToastReplace()

[Tom Lane]

Andres Freund  writes:
> This indicates that a toast record was present for that relation,
> despite:
> [ \d that looks like the table isn't wide enough for that ]
> I think we need to see pg_waldump output for the preceding records. That
> might allow us to see why there's a toast record that's being associated
> with this table, despite there not being a toast table.

I don't think you can make that conclusion.  Perhaps the table once
needed a toast table because of some wide column that got dropped;
if so, it'd still have one.  It'd be safer to look at
pg_class.reltoastrelid to verify existence (or not) of the toast relation.

It strikes me that there could easily be cases where a publisher table
has a toast relation and a subscriber's doesn't ... maybe this code
isn't expecting that?

regards, tom lane

Re: error context for vacuum to include block number

[Andres Freund]

Hi,

Thanks for working on this!

On 2019-12-11 08:36:48 -0600, Justin Pryzby wrote:
> On Wed, Dec 11, 2019 at 09:15:07PM +0900, Michael Paquier wrote:
> > On Fri, Dec 06, 2019 at 10:23:25AM -0600, Justin Pryzby wrote:
> > > Find attached updated patch:
> > >  . Use structure to include relation name.
> > >  . Split into a separate patch rename of "StringInfoData buf".
> > > 
> > > 2019-11-27 20:04:53.640 CST [14244] ERROR:  canceling statement due to 
> > > statement timeout
> > > 2019-11-27 20:04:53.640 CST [14244] CONTEXT:  block 2314 of relation t
> > > 2019-11-27 20:04:53.640 CST [14244] STATEMENT:  vacuum t;
> > > 
> > > I tried to use BufferGetTag() to avoid using a 2ndary structure, but 
> > > fails if
> > > the buffer is not pinned.

The tag will not add all that informative details, because the
relfilenode isn't easily mappable to the table name or such.


> diff --git a/src/backend/access/heap/vacuumlazy.c 
> b/src/backend/access/heap/vacuumlazy.c
> index 043ebb4..9376989 100644
> --- a/src/backend/access/heap/vacuumlazy.c
> +++ b/src/backend/access/heap/vacuumlazy.c
> @@ -138,6 +138,12 @@ typedef struct LVRelStats
>   boollock_waiter_detected;
>  } LVRelStats;
>  
> +typedef struct
> +{
> + char *relname;
> + char *relnamespace;
> + BlockNumber blkno;
> +} vacuum_error_callback_arg;

Hm, wonder if could be worthwhile to not use a separate struct here, but
instead extend one of the existing structs to contain the necessary
information. Or perhaps have one new struct with all the necessary
information. There's already quite a few places that do
get_namespace_name(), for example.



> @@ -524,6 +531,8 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, 
> LVRelStats *vacrelstats,
>   PROGRESS_VACUUM_MAX_DEAD_TUPLES
>   };
>   int64   initprog_val[3];
> + ErrorContextCallback errcallback;
> + vacuum_error_callback_arg cbarg;

Not a fan of "cbarg", too generic.

>   pg_rusage_init(&ru0);
>  
> @@ -635,6 +644,15 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, 
> LVRelStats *vacrelstats,
>   else
>   skipping_blocks = false;
>  
> + /* Setup error traceback support for ereport() */
> + errcallback.callback = vacuum_error_callback;
> + cbarg.relname = relname;
> + cbarg.relnamespace = get_namespace_name(RelationGetNamespace(onerel));
> + cbarg.blkno = 0; /* Not known yet */
> + errcallback.arg = (void *) &cbarg;
> + errcallback.previous = error_context_stack;
> + error_context_stack = &errcallback;
> +
>   for (blkno = 0; blkno < nblocks; blkno++)
>   {
>   Buffer  buf;
> @@ -658,6 +676,8 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, 
> LVRelStats *vacrelstats,
>  
>   pgstat_progress_update_param(PROGRESS_VACUUM_HEAP_BLKS_SCANNED, 
> blkno);
>  
> + cbarg.blkno = blkno;
> +
>   if (blkno == next_unskippable_block)
>   {
>   /* Time to advance next_unskippable_block */
> @@ -817,7 +837,6 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, 
> LVRelStats *vacrelstats,
>  
>   buf = ReadBufferExtended(onerel, MAIN_FORKNUM, blkno,
>RBM_NORMAL, 
> vac_strategy);
> -
>   /* We need buffer cleanup lock so that we can prune HOT chains. 
> */
>   if (!ConditionalLockBufferForCleanup(buf))
>   {
> @@ -1388,6 +1407,9 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, 
> LVRelStats *vacrelstats,
>   RecordPageWithFreeSpace(onerel, blkno, freespace);
>   }
>  
> + /* Pop the error context stack */
> + error_context_stack = errcallback.previous;
> +
>   /* report that everything is scanned and vacuumed */
>   pgstat_progress_update_param(PROGRESS_VACUUM_HEAP_BLKS_SCANNED, blkno);
>  
> @@ -2354,3 +2376,15 @@ heap_page_is_all_visible(Relation rel, Buffer buf,
>  
>   return all_visible;
>  }

I think this will misattribute errors that happen when in the:
/*
 * If we are close to overrunning the available space for 
dead-tuple
 * TIDs, pause and do a cycle of vacuuming before we tackle 
this page.
 */
section of lazy_scan_heap(). That will

a) scan the index, during which we presumably don't want the same error
   context, as it'd be quite misleading: The block that was just scanned
   in the loop isn't actually likely to be the culprit for an index
   problem. And we'd not mention the fact that the problem is occurring
   in the index.
b) will report the wrong block, when in lazy_vacuum_heap().

Greetings,

Andres Freund

Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes

[Tom Lane]

Alvaro Herrera  writes:
> I tested this on libreadline 7.x (where #define
> HAVE_RL_FILENAME_COMPLETION_FUNCTION 1).  I noticed that if I enter a
> filename that doesn't exist and then , it adds a closing quote.
> Bash manages to do nothing somehow, which is the desired behavior IMO.

Hmm.  I'll take a look, but I'm not terribly hopeful.  I have looked
briefly at what Bash does for filename completion, and as I recall
it was massive, spaghetti-ish, and way too much in bed with various
implementation details of libreadline --- they don't pretend to work
with libedit.  I'm not prepared to go there.  It's reasonable for Bash
to expend huge effort on filename completion, because that's such a core
use-case for them, but I don't think it deserves as much work in psql.

> I don't quite understand why a readline library that doesn't have
> rl_filename_completion_function is known to have a
> filename_completion_function, ie. this bit 

> #ifdef HAVE_RL_FILENAME_COMPLETION_FUNCTION
> #define filename_completion_function rl_filename_completion_function
> #else
> /* decl missing in some header files, but function exists anyway */
> extern char *filename_completion_function();
> #endif

I think the point is that before rl_filename_completion_function the
function existed but was just called filename_completion_function.
It's possible that that's obsolete --- I've not really checked.

regards, tom lane

Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes

[Tom Lane]

Robert Haas  writes:
> On Wed, Dec 11, 2019 at 10:52 AM Tom Lane  wrote:
>> I think it's Debian's problem, not ours, if that doesn't work.  It is
>> not unreasonable for a package to probe existence of a library function
>> at configure time.  It's up to them to make sure that the headers match
>> the actual library.

> That seems like an unhelpful attitude. Debian is a mainstream
> platform, and no doubt feels that they have important reasons for what
> they are doing.

Nonetheless, if they're doing that, it's *their* bug not ours when
the run-time library fails to match what was supplied to compile
against.  I think it would fall to them to patch either libedit
or readline to make those two agree.  This is not different in any
way from the expectation that a platform supply a libc whose ABI
is stable.

In any case, this discussion is a bit hypothetical isn't it?
If I understand correctly, your concern is that the proposed
patch might fail to take advantage of functionality that actually
might be present at runtime.  So what?  It's no worse than before.
More, it's likely that there are other similar losses of functionality
already in our code and/or other peoples'.  Debian bought into that
tradeoff, not us.

regards, tom lane

Re: logical decoding bug: segfault in ReorderBufferToastReplace()

[Andres Freund]

Hi,

On 2019-12-11 08:17:01 +, Drouvot, Bertrand wrote:
> >>Core was generated by `postgres: walsender 
> >>(31712)'.
> >>Program terminated with signal 11, Segmentation fault.
> >>#0  ReorderBufferToastReplace (rb=0x3086af0, txn=0x3094a78,
> >>relation=0x2b79177249c8, relation=0x2b79177249c8, change=0x30ac938)
> >>at reorderbuffer.c:3034
> >>3034reorderbuffer.c: No such file or directory.
> >>...
> >>(gdb) #0  ReorderBufferToastReplace (rb=0x3086af0, txn=0x3094a78,
> >>relation=0x2b79177249c8, relation=0x2b79177249c8, change=0x30ac938)
> >>at reorderbuffer.c:3034
> >>#1  ReorderBufferCommit (rb=0x3086af0, xid=xid@entry=1358809,
> >>commit_lsn=9430473346032, end_lsn=,
> >>commit_time=commit_time@entry=628712466364268,
> >>origin_id=origin_id@entry=0, origin_lsn=origin_lsn@entry=0) at
> >>reorderbuffer.c:1584

This indicates that a toast record was present for that relation,
despite:

> 
> \d+ rel_having_issue
>  Table 
> "public.rel_having_issue"
>  Column |   Type   | Collation | Nullable |   
>   Default | Storage  | Stats target | Description
> +--+---+--+-+--+--+-
>  id | integer  |   | not null | 
> nextval('rel_having_issue_id_seq'::regclass) | plain|  |
>  field1   | character varying(255)   |   |  | 
> | extended |  |
>  field2  | integer  |   |  |  
>| plain|  |
>  field3 | timestamp with time zone |   |  |   
>   | plain|  |
> Indexes:
> "rel_having_issue_pkey" PRIMARY KEY, btree (id)
> 
> select relname,relfilenode,reltoastrelid from pg_class where 
> relname='rel_having_issue';
>relname   | relfilenode | reltoastrelid
> -+-+---
>  rel_having_issue |   16428 | 0


I think we need to see pg_waldump output for the preceding records. That
might allow us to see why there's a toast record that's being associated
with this table, despite there not being a toast table.

Seems like we clearly should add an elog(ERROR) here, so we error out,
rather than crash.

Has there been DDL to this table?

Could you print out *change?

Is this version of postgres effectively unmodified in any potentially
relevant region (snapshot computations, generation of WAL records, ...)?

Greetings,

Andres Freund

Re: VACUUM memory management

[Ibrar Ahmed]

On Wed, Dec 11, 2019 at 7:29 PM Robert Haas  wrote:

> On Mon, Dec 9, 2019 at 2:02 PM Ibrar Ahmed  wrote:
> >> Did you see this thread?
> >>
> https://postgr.es/m/CAGTBQpbDCaR6vv9=scXzuT8fSbckf=a3ngzdwfwzbdvugvh...@mail.gmail.com
> >>
> > Yes, and somehow did what is explained.
>
> Did you modify Claudio's patch or write a totally new one?


I wrote completely new patch. I tried multiple techniques like using a list
instead of fixed size array which I thought was most suitable here, but
leave that because of conflict with Parallel Vacuum.


> In either case, why did you choose that approach?


This is the simplest technique. I just divided the maintenance_work_mem in
chunks and allocate chunks as needed. This technique change minimum code
and do what we want to achieve.


> If you wrote a totally new one, have you compared your work with
> Claudio's, to see if he covered
> anything you might need to cover?


No, this part I missed, I will do that and will share my thoughts.



> Please explain why your patch is
> better/different than his.
>
>

> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>


-- 
Ibrar Ahmed

Re: Add .editorconfig

[Peter Eisentraut]

On 2019-12-11 17:09, Daniel Gustafsson wrote:

On 11 Dec 2019, at 17:00, Peter Eisentraut  
wrote:

There were a couple of recent threads that wanted to add an .editorconfig file 
but never actually ended up doing so.[0][1]  Here is a patch.  It is meant to 
match more or less what's in .dir-locals.el.


+[*.{c,h,l,y,pl,pm}]

What about *.sh?

Not that there is much of Python in the tree, but shouldn't those also be
covered if we do this?

[*.py]
indent_style = space
indent_size = 4


Those were not in the Emacs config either.

I'd be inclined to leave the Python stuff alone, in particular.  The 
PEP-8 style appears to be universally accepted as the default, so we 
don't need to specify it here.  We only need to specify what's different 
or where no sensible default exists.


--
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Add .editorconfig

[Daniel Gustafsson]

> On 11 Dec 2019, at 17:00, Peter Eisentraut  
> wrote:
> 
> There were a couple of recent threads that wanted to add an .editorconfig 
> file but never actually ended up doing so.[0][1]  Here is a patch.  It is 
> meant to match more or less what's in .dir-locals.el.

+[*.{c,h,l,y,pl,pm}]

What about *.sh?

Not that there is much of Python in the tree, but shouldn't those also be
covered if we do this?

[*.py]
indent_style = space
indent_size = 4

cheers ./daniel

Add .editorconfig

[Peter Eisentraut]

There were a couple of recent threads that wanted to add an 
.editorconfig file but never actually ended up doing so.[0][1]  Here is 
a patch.  It is meant to match more or less what's in .dir-locals.el.


I have only tested this with the GitHub view, not with an actual editor.

[0]: 
https://www.postgresql.org/message-id/flat/20180605172252.GA2433%40fetter.org
[1]: 
https://www.postgresql.org/message-id/flat/20180529153617.c72llpzbvt46chr6%40alap3.anarazel.de


--
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From fcf66f8a2e29818b55b0f2c4754bd8bef15bc546 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Wed, 11 Dec 2019 16:53:48 +0100
Subject: [PATCH] Add .editorconfig

---
 .editorconfig | 14 ++
 1 file changed, 14 insertions(+)
 create mode 100644 .editorconfig

diff --git a/.editorconfig b/.editorconfig
new file mode 100644
index 00..d69a3d1dc4
--- /dev/null
+++ b/.editorconfig
@@ -0,0 +1,14 @@
+root = true
+
+[*.{c,h,l,y,pl,pm}]
+indent_style = tab
+indent_size = tab
+tab_width = 4
+
+[*.{sgml,xml}]
+indent_style = space
+indent_size = 1
+
+[*.xsl]
+indent_style = space
+indent_size = 2
-- 
2.24.0

Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes

[Robert Haas]

On Wed, Dec 11, 2019 at 10:52 AM Tom Lane  wrote:
> I think it's Debian's problem, not ours, if that doesn't work.  It is
> not unreasonable for a package to probe existence of a library function
> at configure time.  It's up to them to make sure that the headers match
> the actual library.

That seems like an unhelpful attitude. Debian is a mainstream
platform, and no doubt feels that they have important reasons for what
they are doing.

That's not to say that I'm against the patch, but I don't believe it's
right to treat the concerns of mainstream Linux distributions in
anything less than a serious manner.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: logical decoding bug: segfault in ReorderBufferToastReplace()

[Robert Haas]

On Wed, Dec 11, 2019 at 3:17 AM Drouvot, Bertrand  wrote:
>Here it is:
>
> \d+ rel_having_issue

You did a heck of a job choosing the name of that table. I bet nobody
was surprised when it had an issue!

:-)

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes

[Tom Lane]

Alvaro Herrera  writes:
> On 2019-Dec-11, Robert Haas wrote:
>> Are people still compiling against libedit and then redirecting to
>> libreadline at runtime? I seem to recall some discussion about this
>> being a thing, many years ago.

> Yeah, Debian did that out of licensing concerns.  It seems they still do
> that, based on
> https://packages.debian.org/bullseye/postgresql-client-12

I think it's Debian's problem, not ours, if that doesn't work.  It is
not unreasonable for a package to probe existence of a library function
at configure time.  It's up to them to make sure that the headers match
the actual library.

regards, tom lane

Re: allowing broader use of simplehash

[Robert Haas]

On Tue, Dec 10, 2019 at 4:59 PM Andres Freund  wrote:
> 3) For lots of one-off uses of hashtables that aren't performance
>critical, we want a *simple* API. That IMO would mean that key/value
>end up being separately allocated pointers, and that just a
>comparator is provided when creating the hashtable.

I think the simplicity of the API is a key point. Some things that are
bothersome about dynahash:

- It knows about memory contexts and insists on having its own.
- You can't just use a hash table in shared memory; you have to
"attach" to it first and have an object in backend-private memory.
- The usual way of getting a shared hash table is ShmemInitHash(), but
that means that the hash table has its own named chunk and that it's
in the main shared memory segment. If you want to put it inside
another chunk or put it in DSM or whatever, it doesn't work.
- It knows about LWLocks and if it's a shared table it needs its own
tranche of them.
- hash_search() is hard to wrap your head around.

One thing I dislike about simplehash is that the #define-based
interface is somewhat hard to use. It's not that it's a bad design.
It's just you have to sit down and think for a while to figure out
which things you need to #define in order to get it to do what you
want. I'm not sure that's something that can or needs to be fixed, but
it's something to consider. Even dynahash, as annoying as it is, is in
some ways easier to get up and running.

Probably the two most common uses cases are: (1) a fixed-sized shared
memory hash table of fixed-size entries where the key is the first N
bytes of the entry and it never grows, or (2) a backend-private or
perhaps frontend hash table of fixed-size entries where the key is the
first N bytes of the entry, and it grows without limit. I think should
consider having specialized APIs for those two cases and then more
general APIs that you can use when that's not enough.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Optimization of NestLoop join in the case of guaranteed empty inner subtree

[Tom Lane]

Andrey Lepikhov  writes:
> During NestLoop execution we have bad corner case: if outer subtree 
> contains tuples the join node will scan inner subtree even if it does 
> not return any tuples.

So the first question about corner-case optimizations like this is always
"how much overhead does it add in the normal case where it fails to gain
anything?".  I see no performance numbers in your proposal.

I do not much like anything about the code, either: as written it's
only helpful for an especially narrow corner case (so narrow that
I wonder if it really ever helps at all: surely calling a nodeMaterial
whose tuplestore is empty doesn't cost much).  But that doesn't stop it
from adding a bool to the generic PlanState struct, with global
implications.  What I'd expected from your text description is that
nodeNestLoop would remember whether its inner child had returned zero rows
the first time, and assume that subsequent executions could be skipped
unless the inner child's parameters change.

regards, tom lane

Re: adding partitioned tables to publications

[Peter Eisentraut]

On 2019-12-06 08:48, Amit Langote wrote:

0001:  Adding a partitioned table to a publication implicitly adds all
its partitions.  The receiving side must have tables matching the
published partitions, which is typically the case, because the same
partition tree is defined on both nodes.


This looks pretty good to me now.  But you need to make all the changed 
queries version-aware so that you can still replicate from and to older 
versions.  (For example, pg_partition_tree is not very old.)


This part looks a bit fishy:

+   /*
+* If either table is partitioned, skip copying.  Individual 
partitions

+* will be copied instead.
+*/
+   if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE ||
+   remote_relkind == RELKIND_PARTITIONED_TABLE)
+   {
+   logicalrep_rel_close(relmapentry, NoLock);
+   return;
+   }

I don't think you want to filter out a partitioned table on the local 
side, since (a) COPY can handle that, and (b) it's (as of this patch) an 
error to have a partitioned table in the subscription table set.


I'm not a fan of the new ValidateSubscriptionRel() function.  It's too 
obscure, especially the return value.  Doesn't seem worth it.


--
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Fetching timeline during recovery

[Stephen Frost]

Greetings,

* Michael Paquier (mich...@paquier.xyz) wrote:
> On Wed, Dec 11, 2019 at 10:16:29AM -0500, Stephen Frost wrote:
> > I've not followed this discussion very closely but I agree entirely that
> > it's really nice to have the timeline be able to be queried in a more
> > timely manner than asking through pg_control_checkpoint() gives you.
> > 
> > I'm not sure about adding a text argument to such a function though, I
> > would think you'd either have multiple rows if it's an SRF that gives
> > you the information on each row and allows a user to filter with a WHERE
> > clause, or do something like what pg_stat_replication has and just have
> > a bunch of columns.
> 
> With a NULL added for the values which cannot be defined then, like
> trying to use the function on a primary for the fields which can only
> show up at recovery?  

Sure, the function would only return those values that make sense for
the state that the system is in.

> That would be possible, still my heart tells me
> that a function returning one row is a more natural approach for
> this stuff.  I may be under too much used to what we have in the TAP
> tests though.

I'm confused- wouldn't the above approach be a function that's returning
only one row, if you had a bunch of columns and then had NULL values for
those cases that didn't apply..?  Or, if you were thinking about the SRF
approach that you suggested, you could use a WHERE clause to make it
only one row...  Though I can see how it's nicer to just have one row in
some cases which is why I was suggesting the "bunch of columns"
approach.

Thanks,

Stephen


signature.asc
Description: PGP signature

Re: [Proposal] Level4 Warnings show many shadow vars

[Stephen Frost]

Greetings,

Didn't see this previously (it's our typical approach to 'reply-all' to
people), though I don't think it changes my feelings about the latest
proposed patch.

* Ranier Vilela (ranier_...@hotmail.com) wrote:
> De: Stephen Frost
> Enviadas: Terça-feira, 10 de Dezembro de 2019 17:52
> 
> >There's multiple ways to get there though and I think what you're seeing
> >is that the "just change it to something else" answer isn't necessairly
> >going to be viewed as an improvement (or, at least, not enough of an
> >improvement to accept the cost of the change).
> Well, I was trying to apply another non-implied rule, "break nothing".

I agree with not breaking things but that doesn't mean the only
reasonable approach is to do the absolute minimum- you might not be
breaking something today, but it's going to confuse people later on down
the road and may lead to bugs being introduced due to that confusion, or
at the very least will add to people's time to figure out what's really
going on.

> >Why not change the variables?  Changes that also improve the code itself
> >along with eliminating the shadowing of the global variable are going to
> >be a lot easier to be accepted.
> Contrary to what I was initially thinking, it seems to me that changing the 
> names of global variables is more acceptable to the people of the project.

I wasn't suggesting to change the names of the global variables in this
specific case, though I could see that being a better approach in some
instances- but it really depends.  Each case needs to be reviewed and
considered and the best approach taken.

> >Sure, but have you looked at how it's used?  Instead of just renaming
> >the numTables variables in the functions that accept it- could those
> >variables just be removed instead of changing their name to make it look
> >like they're something different when they aren't actually different?
> No. I didn't look.

I think we need to be looking at the changes and considering them, and
the person proposing the changes should be doing that and not just
expecting everyone else to do so.

> >I've only spent a bit of time looking at it, but it sure looks like the
> >variables could just be removed, and doing so doesn't break the
> >regression tests, which supports the idea that maybe there's a better
> >way to deal with those particular variables rather than renaming them.
> >Another approach to consider might be to move some global variables into
> >structures that are then global with better names to indicate that's
> >what they are.
> It does not seem reasonable to me what you are asking.
> Because as I was told here and I agree in part. I do not have the necessary 
> knowledge of structures and logic to propose big changes.

I'd suggest that we work through that then and get you up to speed on
the structures and logic- the pg_dump code is pretty ugly but the
specific usage of numTables isn't too bad.  Each of these should be
looked at independently and thought about "what's the right way to fix
this?"  The right way isn't necessairly to just rename the variables, as
I was saying, and doing so may lead to more confusion, not less.

> For the work I set out to, find bugs and make minor performance improvements, 
> I believe, can contribute safely and without ruining anything.

Having shadowed globals, while kinda ugly, doesn't necessairly mean it's
a bug.  I'm not sure what "minor performance improvements" are being
claimed here but there's a whole lot of work involved in demonstrating
that a change is a performance improvement.

> By just changing the names of variables to something consistent and readable, 
> the goal will be done without break anything.

but.. the changes you're proposing are making them inconsistent and
confusing when there isn't actually a difference between the global and
the local, it's just the somewhere along the way someone thought they
needed to pass in numTables when they really didn't, and we should go
fix *that*, not rename the variable to something else to make someone
later on go "wait, why did we need to pass in this variable?  how is
this different from the global?"

> Who is best to make these changes, are the authors and reviewers. Once we no 
> longer have the problem of shadow variables, we can turn on the alert without 
> breaking automatic compilation, as Tom Lane is concerned.

Perhaps I'm a bit confused, but it seems that you're the author of these
specific changes, and I'm trying to provide feedback as to how you can
improve what you're proposing in a way that will improve the code base
overall and reduce the confusion while also eliminating the shadow
variables.  If the author of a patch isn't open to this kind of review
and willing to adjust the patch to improve it because they aren't sure
that their changes will be correct then they could at least post them
back here and ask, or better, go look at the code and get a better
understanding of what's going on to build confidence in the change.

Th

Re: error context for vacuum to include block number

[Alvaro Herrera]

On 2019-Dec-11, Justin Pryzby wrote:

> @@ -635,6 +644,15 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, 
> LVRelStats *vacrelstats,
>   else
>   skipping_blocks = false;
>  
> + /* Setup error traceback support for ereport() */
> + errcallback.callback = vacuum_error_callback;
> + cbarg.relname = relname;
> + cbarg.relnamespace = get_namespace_name(RelationGetNamespace(onerel));
> + cbarg.blkno = 0; /* Not known yet */

Shouldn't you use InvalidBlockNumber for this initialization?

> @@ -658,6 +676,8 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, 
> LVRelStats *vacrelstats,
>  
>   pgstat_progress_update_param(PROGRESS_VACUUM_HEAP_BLKS_SCANNED, 
> blkno);
>  
> + cbarg.blkno = blkno;

I would put this before pgstat_progress_update_param, just out of
paranoia.

> @@ -817,7 +837,6 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, 
> LVRelStats *vacrelstats,
>  
>   buf = ReadBufferExtended(onerel, MAIN_FORKNUM, blkno,
>RBM_NORMAL, 
> vac_strategy);
> -
>   /* We need buffer cleanup lock so that we can prune HOT chains. 
> */
>   if (!ConditionalLockBufferForCleanup(buf))
>   {

Lose this hunk?

> @@ -2354,3 +2376,15 @@ heap_page_is_all_visible(Relation rel, Buffer buf,
>  
>   return all_visible;
>  }
> +
> +/*
> + * Error context callback for errors occurring during vacuum.
> + */
> +static void
> +vacuum_error_callback(void *arg)
> +{
> + vacuum_error_callback_arg *cbarg = arg;
> +
> + errcontext("while scanning block %u of relation \"%s.%s\"",
> + cbarg->blkno, cbarg->relnamespace, cbarg->relname);
> +}

I would put this function around line 1512 (just after lazy_scan_heap)
rather than at bottom of file.  (And move its prototype accordingly, to
line 156.)  Or do you intend that this is going to be used for
lazy_vacuum_heap too?  Maybe it should.

Patch looks good to me otherwise.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Fetching timeline during recovery

[Michael Paquier]

On Wed, Dec 11, 2019 at 10:16:29AM -0500, Stephen Frost wrote:
> I've not followed this discussion very closely but I agree entirely that
> it's really nice to have the timeline be able to be queried in a more
> timely manner than asking through pg_control_checkpoint() gives you.
> 
> I'm not sure about adding a text argument to such a function though, I
> would think you'd either have multiple rows if it's an SRF that gives
> you the information on each row and allows a user to filter with a WHERE
> clause, or do something like what pg_stat_replication has and just have
> a bunch of columns.

With a NULL added for the values which cannot be defined then, like
trying to use the function on a primary for the fields which can only
show up at recovery?  That would be possible, still my heart tells me
that a function returning one row is a more natural approach for
this stuff.  I may be under too much used to what we have in the TAP
tests though.
--
Michael


signature.asc
Description: PGP signature

Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes

[Alvaro Herrera]

On 2019-Dec-11, Alvaro Herrera wrote:

> On 2019-Dec-11, Robert Haas wrote:

> > If it were being done it would be
> > advantageous to have the checks be runtime rather than compile-time,
> > although I guess that would probably be tough to make work.
> 
> Yeah.  On the other hand, I suppose Debian uses the BSD version of the
> libraries, not the Apple version, so I think it should be fine?

... actually, grepping libedit's source at
http://deb.debian.org/debian/pool/main/libe/libedit/libedit_3.1-20191025.orig.tar.gz
there's no occurrence of rl_filename_quoting_function.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [Proposal] Level4 Warnings show many shadow vars

[Stephen Frost]

Greetings,

* Ranier Vilela (ranier_...@hotmail.com) wrote:
> New version the global patch unshadow.
> * names more consistent and readable.
> * without big changes.
> * goal,, unshadow all global variables, only, even the simplest.

This didn't address any of the comments that I raised elsewhere on this
thread...  I certainly don't like the changes being proposed here for
pg_dump.

Thanks,

Stephen


signature.asc
Description: PGP signature

Re: Fetching timeline during recovery

[Stephen Frost]

Greetings,

* Michael Paquier (mich...@paquier.xyz) wrote:
> I would be actually tempted to do the following: one single SRF
> function, say pg_wal_info which takes a text argument in input with
> the following values: flush, write, insert, receive, replay.  Thinking
> more about it that would be rather neat, and more extensible than the
> rest discussed until now.  See for example PostgresNode::lsn.

I've not followed this discussion very closely but I agree entirely that
it's really nice to have the timeline be able to be queried in a more
timely manner than asking through pg_control_checkpoint() gives you.

I'm not sure about adding a text argument to such a function though, I
would think you'd either have multiple rows if it's an SRF that gives
you the information on each row and allows a user to filter with a WHERE
clause, or do something like what pg_stat_replication has and just have
a bunch of columns.

Given that we've already gone with the "bunch of columns" approach
elsewhere, it seems like that approach would be more consistent.

Thanks,

Stephen


signature.asc
Description: PGP signature

Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes

[Alvaro Herrera]

On 2019-Dec-11, Robert Haas wrote:

> On Sun, Nov 3, 2019 at 5:40 PM Tom Lane  wrote:
> > Of course, this only works if we have those hooks :-(.  So far as
> > I can tell, all libreadline variants that might still exist in the wild
> > do have them; but libedit doesn't, at least not in the version Apple
> > is shipping.  Hence, what the attached patch does is to make configure
> > probe for the existence of the hook variables; if they're not there,
> > fall back to what I proposed previously.
> 
> Are people still compiling against libedit and then redirecting to
> libreadline at runtime? I seem to recall some discussion about this
> being a thing, many years ago.

Yeah, Debian did that out of licensing concerns.  It seems they still do
that, based on
https://packages.debian.org/bullseye/postgresql-client-12

> If it were being done it would be
> advantageous to have the checks be runtime rather than compile-time,
> although I guess that would probably be tough to make work.

Yeah.  On the other hand, I suppose Debian uses the BSD version of the
libraries, not the Apple version, so I think it should be fine?

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: allowing broader use of simplehash

[Robert Haas]

On Tue, Dec 10, 2019 at 4:59 PM Andres Freund  wrote:
> Neat!

Thanks.

> > A significant problem in either case is that a simplehash wants to
> > live in a memory context; no such thing exists either for data in
> > shared memory nor in frontend code. However, it seems to be quite easy
> > to provide a way for simplehash to be defined so that it doesn't care
> > about memory contexts. See 0001.
>
> I wonder if we shouldn't instead just go for an "implicit" memory
> context instead. It's a bit ugly to have a growing set of different
> signatures.

I don't really know what you mean by this. I don't actually think the
different signatures are a big deal. It affects a pretty limited
number of functions, and that seems less ugly than trying to create
some sort of dummy not-really-a-context object that can live in
frontend code, and a lot easier than actually making contexts work in
frontend code. The latter might be the better long-term solution, but
I don't think we should insist on doing it first.

Another way forward would be to replace the MemoryContext references
with a void * that happens, in the case of the backend, to be a
MemoryContext, and could be NULL when none is required. However, that
would give up some type-checking for no current benefit. If simplehash
becomes more widely used and at some point it's clear that this would
be a net win, we can change it then.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: error context for vacuum to include block number

[Justin Pryzby]

On Wed, Dec 11, 2019 at 09:15:07PM +0900, Michael Paquier wrote:
> On Fri, Dec 06, 2019 at 10:23:25AM -0600, Justin Pryzby wrote:
> > Find attached updated patch:
> >  . Use structure to include relation name.
> >  . Split into a separate patch rename of "StringInfoData buf".
> > 
> > 2019-11-27 20:04:53.640 CST [14244] ERROR:  canceling statement due to 
> > statement timeout
> > 2019-11-27 20:04:53.640 CST [14244] CONTEXT:  block 2314 of relation t
> > 2019-11-27 20:04:53.640 CST [14244] STATEMENT:  vacuum t;
> > 
> > I tried to use BufferGetTag() to avoid using a 2ndary structure, but fails 
> > if
> > the buffer is not pinned.
> 
> No problem from me to add more context directly in lazy_scan_heap().

Do you mean without a callback ?  I think that's necessary, since the IO errors
would happen within ReadBufferExtended, but we don't want to polute that with
errcontext.  And cannot call errcontext on its own:
FATAL:  errstart was not called

> So I would suggest the following instead:
> "while scanning block %u of relation \"%s.%s\"" 

Done in the attached.
>From b78505f3008b501b1d427c491bc0f8b796d68879 Mon Sep 17 00:00:00 2001
From: Justin Pryzby 
Date: Wed, 20 Nov 2019 14:53:20 -0600
Subject: [PATCH v3] vacuum errcontext to show block being processed

As requested here.
https://www.postgresql.org/message-id/20190807235154.erbmr4o4bo6vgnjv%40alap3.anarazel.de
---
 src/backend/access/heap/vacuumlazy.c | 36 +++-
 1 file changed, 35 insertions(+), 1 deletion(-)

diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 043ebb4..9376989 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -138,6 +138,12 @@ typedef struct LVRelStats
 	bool		lock_waiter_detected;
 } LVRelStats;
 
+typedef struct
+{
+	char *relname;
+	char *relnamespace;
+	BlockNumber blkno;
+} vacuum_error_callback_arg;
 
 /* A few variables that don't seem worth passing around as parameters */
 static int	elevel = -1;
@@ -175,6 +181,7 @@ static bool lazy_tid_reaped(ItemPointer itemptr, void *state);
 static int	vac_cmp_itemptr(const void *left, const void *right);
 static bool heap_page_is_all_visible(Relation rel, Buffer buf,
 	 TransactionId *visibility_cutoff_xid, bool *all_frozen);
+static void vacuum_error_callback(void *arg);
 
 
 /*
@@ -524,6 +531,8 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats,
 		PROGRESS_VACUUM_MAX_DEAD_TUPLES
 	};
 	int64		initprog_val[3];
+	ErrorContextCallback errcallback;
+	vacuum_error_callback_arg cbarg;
 
 	pg_rusage_init(&ru0);
 
@@ -635,6 +644,15 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats,
 	else
 		skipping_blocks = false;
 
+	/* Setup error traceback support for ereport() */
+	errcallback.callback = vacuum_error_callback;
+	cbarg.relname = relname;
+	cbarg.relnamespace = get_namespace_name(RelationGetNamespace(onerel));
+	cbarg.blkno = 0; /* Not known yet */
+	errcallback.arg = (void *) &cbarg;
+	errcallback.previous = error_context_stack;
+	error_context_stack = &errcallback;
+
 	for (blkno = 0; blkno < nblocks; blkno++)
 	{
 		Buffer		buf;
@@ -658,6 +676,8 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats,
 
 		pgstat_progress_update_param(PROGRESS_VACUUM_HEAP_BLKS_SCANNED, blkno);
 
+		cbarg.blkno = blkno;
+
 		if (blkno == next_unskippable_block)
 		{
 			/* Time to advance next_unskippable_block */
@@ -817,7 +837,6 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats,
 
 		buf = ReadBufferExtended(onerel, MAIN_FORKNUM, blkno,
  RBM_NORMAL, vac_strategy);
-
 		/* We need buffer cleanup lock so that we can prune HOT chains. */
 		if (!ConditionalLockBufferForCleanup(buf))
 		{
@@ -1388,6 +1407,9 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats,
 			RecordPageWithFreeSpace(onerel, blkno, freespace);
 	}
 
+	/* Pop the error context stack */
+	error_context_stack = errcallback.previous;
+
 	/* report that everything is scanned and vacuumed */
 	pgstat_progress_update_param(PROGRESS_VACUUM_HEAP_BLKS_SCANNED, blkno);
 
@@ -2354,3 +2376,15 @@ heap_page_is_all_visible(Relation rel, Buffer buf,
 
 	return all_visible;
 }
+
+/*
+ * Error context callback for errors occurring during vacuum.
+ */
+static void
+vacuum_error_callback(void *arg)
+{
+	vacuum_error_callback_arg *cbarg = arg;
+
+	errcontext("while scanning block %u of relation \"%s.%s\"",
+			cbarg->blkno, cbarg->relnamespace, cbarg->relname);
+}
-- 
2.7.4

Re: VACUUM memory management

[Robert Haas]

On Mon, Dec 9, 2019 at 2:02 PM Ibrar Ahmed  wrote:
>> Did you see this thread?
>> https://postgr.es/m/CAGTBQpbDCaR6vv9=scXzuT8fSbckf=a3ngzdwfwzbdvugvh...@mail.gmail.com
>>
> Yes, and somehow did what is explained.

Did you modify Claudio's patch or write a totally new one? In either
case, why did you choose that approach? If you wrote a totally new
one, have you compared your work with Claudio's, to see if he covered
anything you might need to cover? Please explain why your patch is
better/different than his.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: VACUUM memory management

[Robert Haas]

On Mon, Dec 9, 2019 at 4:37 PM Greg Stark  wrote:
> On Mon, 9 Dec 2019 at 14:03, Ibrar Ahmed  wrote:
> > I'd
> > actually argue that the segment size should be substantially smaller
> > than 1 GB, like say 64MB; there are still some people running systems
> > which are small enough that allocating 1 GB when we may need only 6
> > bytes can drive the system into OOM."
>
> I don't even see why you would allocated as much as 64MB. I would
> think something around 1MB would be more sensible. So you might need
> an array of segment pointers as long as a few thousand pointers, big
> deal. We can handle repalloc on 8kB arrays pretty easily.

See 
https://www.postgresql.org/message-id/9bf3fe70-7aac-cbf7-62f7-acdaa4306ccb%40iki.fi

Another consideration is that, if we have parallel VACUUM, this all
needs to be done using DSM or DSA, neither of which is going to do a
fantastic job with lots of 1MB allocations. If you allocate 1MB DSMs,
you'll run out of DSM slots. If you allocate 1MB chunks from DSA,
it'll allocate progressively larger DSMs and give you 1MB chunks from
them. That's probably OK, but you're just wasting whatever memory from
the chunk you don't end up allocating.

I suggested 64MB because I don't think many people these days run out
of memory because VACUUM overshoots its required memory budget by a
few tens of megabytes.  The problem is when it overruns by hundreds of
megabytes, and people would like large maintenance_work_mem settings
where the overrun might be gigabytes.

Perhaps there are contrary arguments, but I don't think the cost of
repalloc() is really the issue here.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes

[Robert Haas]

On Sun, Nov 3, 2019 at 5:40 PM Tom Lane  wrote:
> Of course, this only works if we have those hooks :-(.  So far as
> I can tell, all libreadline variants that might still exist in the wild
> do have them; but libedit doesn't, at least not in the version Apple
> is shipping.  Hence, what the attached patch does is to make configure
> probe for the existence of the hook variables; if they're not there,
> fall back to what I proposed previously.

Are people still compiling against libedit and then redirecting to
libreadline at runtime? I seem to recall some discussion about this
being a thing, many years ago. If it were being done it would be
advantageous to have the checks be runtime rather than compile-time,
although I guess that would probably be tough to make work.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: global / super barriers (for checksums)

[Robert Haas]

On Mon, Dec 9, 2019 at 7:37 PM Daniel Gustafsson  wrote:
> I've read through the patchset and played around with it to try and break it
> and understand it (not in that order).  Being a bit out of my comfort zone, I
> can't offer the deep insights that Andres has done; but in reading the code it
> all makes sense, and it works as I expect it to.

Stellar. If nobody objects in the meantime, I plan to commit 0001-0003
next week.

> The patch has a tiny typo though, s/procesess/processes/:

But I'll fix that first. Thanks for the review.

> Something which can be observed in order to hook up a test for it, but which
> has no sideeffects?  A NOOP barrier which only does a debug elog?

It would be a bit hard to hook up a test for that, because a normal
test doesn't see the logs, and a TAP test could, but in this case what
you want to check for is that there are no stragglers who should have
gotten the memo and didn't, which supposes you have a list of everyone
who ought to get the memo. You could manually list out all the
background processes but that seems like it would be at risk of
becoming obsolete. You could check that everything in pg_stat_activity
emitted a message except for a known list of exceptions, which would
be less likely to become obsolete, but would also be complex. Also, I
think it's a crock to have something like that in there long term just
for testing purposes.

> I sort of like the callback idea conceptually, but Andres is making a good
> point about the extensibility actually making it harder to reason about.

That objection doesn't hold any water for me, because this is open
source. People can always patch the core. If we don't add hooks, then
we make life easier for fork maintainers (including my employer) and
harder for extension authors. I think that's *definitely* the wrong
bet for the community to be making; we should be trying very hard to
help extension authors and minimize the need for forks. If you install
an extension that uses a hook, any hook, and it breaks things, then
you get to keep both pieces.

And that approach would let us do spiffy testing without having to
leave cruft in core.

> FWIW I've rebased the online checksums patch on top of this patchset instead 
> of
> Andres' previous patch and everything seems to work fine.  All tests pass, and
> I've been unable to trigger anything unexpected.  I'll post the new version of
> said patch shortly.

Cool.

> > and for the thing that got me
> > motivated to work on this, which is ALTER SYSTEM READ ONLY.
>
> Aha, thats a pretty neat usecase.

Thanks.

> In order to play around with and try to understand the patchset I wrote a PoC
> PGC_PROCSIGNALBARRIER class for GUCs and converted a few PGC_SIGHUP GUCs to 
> use
> a barrier rather than a HUP.  The idea was to test interacting with the API a
> bit more than the online checksum patch does, as its needs are pretty basic.
> In hacking this up I didn't really come across anything in particular that I
> lacked, and the result worked fine (insofar as a PoC can be considered
> working).

Cool.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Optimization of NestLoop join in the case of guaranteed empty inner subtree

[Andrey Lepikhov]

During NestLoop execution we have bad corner case: if outer subtree 
contains tuples the join node will scan inner subtree even if it does 
not return any tuples.


To reproduce the problem see 'problem.sql' in attachment:
Out of explain analyze see in 'problem_explain.txt'

As you can see, executor scan each of 1e5 outer tuples despite the fact 
that inner can't return any tuples.


Teodor Sigaev and I developed a patch to solve this problem. Result of 
explain analyze procedure can be found in the 'optimized_execution.txt'.


--
Andrey Lepikhov
Postgres Professional
https://postgrespro.com
The Russian Postgres Company


problem.sql
Description: application/sql
QUERY PLAN  
  
--
 Limit  (cost=0.29..1515.83 rows=1 width=80) (actual time=25.194..25.194 rows=0 
loops=1)
   ->  Nested Loop  (cost=0.29..504674.12 rows=333 width=80) (actual 
time=25.193..25.193 rows=0 loops=1)
 Join Filter: (big.id = small.id)
 ->  Index Scan Backward using big_pkey on big  (cost=0.29..5152.29 
rows=10 width=44) (actual time=0.005..12.933 rows=10 loops=1)
 ->  Materialize  (cost=0.00..22.66 rows=333 width=36) (actual 
time=0.000..0.000 rows=0 loops=10)
   ->  Seq Scan on small  (cost=0.00..21.00 rows=333 width=36) 
(actual time=0.145..0.145 rows=0 loops=1)
 Filter: ((id)::numeric > '1000'::numeric)
 Rows Removed by Filter: 1000
 Planning Time: 0.215 ms
 Execution Time: 25.214 ms
(10 rows)

>From 40ffbd2d9ee5954e5ae09f1e5a929ae2f2b17b8c Mon Sep 17 00:00:00 2001
From: "Andrey V. Lepikhov" 
Date: Mon, 9 Dec 2019 18:25:04 +0500
Subject: [PATCH] Skip scan of outer subtree if inner of the NestedLoop node is
 guaranteed empty.

---
 src/backend/executor/nodeMaterial.c   | 11 +++
 src/backend/executor/nodeNestloop.c   |  5 +
 src/include/nodes/execnodes.h |  4 
 src/test/regress/expected/partition_prune.out |  8 
 4 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/src/backend/executor/nodeMaterial.c b/src/backend/executor/nodeMaterial.c
index cc93bbe45b..98771647f6 100644
--- a/src/backend/executor/nodeMaterial.c
+++ b/src/backend/executor/nodeMaterial.c
@@ -135,6 +135,8 @@ ExecMaterial(PlanState *pstate)
 		if (TupIsNull(outerslot))
 		{
 			node->eof_underlying = true;
+			if (tuplestorestate && tuplestore_tuple_count(tuplestorestate) == 0)
+node->ss.ps.guaranteed_empty = true;
 			return NULL;
 		}
 
@@ -348,6 +350,9 @@ ExecReScanMaterial(MaterialState *node)
 			node->tuplestorestate = NULL;
 			if (outerPlan->chgParam == NULL)
 ExecReScan(outerPlan);
+			else
+/* At first look no cases can lead to this code. But still. */
+node->ss.ps.guaranteed_empty = false;
 			node->eof_underlying = false;
 		}
 		else
@@ -363,6 +368,12 @@ ExecReScanMaterial(MaterialState *node)
 		 */
 		if (outerPlan->chgParam == NULL)
 			ExecReScan(outerPlan);
+
+		/*
+		 * The node suppress tuplestore and guaranteed_empty trick isn't work.
+		 */
+		Assert(node->ss.ps.guaranteed_empty == false);
+
 		node->eof_underlying = false;
 	}
 }
diff --git a/src/backend/executor/nodeNestloop.c b/src/backend/executor/nodeNestloop.c
index fc6667ef82..ea26a7d9d1 100644
--- a/src/backend/executor/nodeNestloop.c
+++ b/src/backend/executor/nodeNestloop.c
@@ -164,6 +164,11 @@ ExecNestLoop(PlanState *pstate)
 		{
 			ENL1_printf("no inner tuple, need new outer tuple");
 
+			if (innerPlan->guaranteed_empty &&
+(node->js.jointype == JOIN_INNER ||
+ node->js.jointype == JOIN_SEMI))
+return NULL;
+
 			node->nl_NeedNewOuter = true;
 
 			if (!node->nl_MatchedOuter &&
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 692438d6df..c7f1a14526 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -1007,6 +1007,9 @@ typedef struct PlanState
 	 * ExecConditionalAssignProjectionInfo()).  If no projection is necessary
 	 * ExecConditionalAssignProjectionInfo() defaults those fields to the scan
 	 * operations.
+	 *
+	 * If guaranteed_empty field is true, this means that no tuple will be
+	 * returned by the node.
 	 */
 	const TupleTableSlotOps *scanops;
 	const TupleTableSlotOps *outerops;
@@ -1020,6 +1023,7 @@ typedef struct PlanState
 	bool		outeropsset;
 	bool		inneropsset;
 	bool		resultopsset;
+	bool		guaranteed_empty;
 } PlanState;
 
 /* 
diff --git a/src/test/regress/expected/partition_prune.out b/src/test/regress/expected/partition_prune.out
index f9eeda60e6..04cfe0944e 100644
--- a/src/test/regress/expected/partition_prune.out
+++ b/src/test/regress/expected/partition_prune.out
@@ -2455,9 +2455,9 @@ update ab_a1 set b = 3 from ab where ab.a = 1 an

RE: [Proposal] Level4 Warnings show many shadow vars

[Ranier Vilela]

New version the global patch unshadow.
* names more consistent and readable.
* without big changes.
* goal,, unshadow all global variables, only, even the simplest.

regards,
Ranier Vilela
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 6bc1a6b46d..a496727e34 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -892,8 +892,8 @@ static int	emode_for_corrupt_record(int emode, XLogRecPtr RecPtr);
 static void XLogFileClose(void);
 static void PreallocXlogFiles(XLogRecPtr endptr);
 static void RemoveTempXlogFiles(void);
-static void RemoveOldXlogFiles(XLogSegNo segno, XLogRecPtr RedoRecPtr, XLogRecPtr endptr);
-static void RemoveXlogFile(const char *segname, XLogRecPtr RedoRecPtr, XLogRecPtr endptr);
+static void RemoveOldXlogFiles(XLogSegNo segno, XLogRecPtr lastRedoRecPtr, XLogRecPtr endptr);
+static void RemoveXlogFile(const char *segname, XLogRecPtr lastRedoRecPtr, XLogRecPtr endptr);
 static void UpdateLastRemovedPtr(char *filename);
 static void ValidateXLOGDirectoryStructure(void);
 static void CleanupBackupHistory(void);
@@ -2299,7 +2299,7 @@ assign_checkpoint_completion_target(double newval, void *extra)
  * XLOG segments? Returns the highest segment that should be preallocated.
  */
 static XLogSegNo
-XLOGfileslop(XLogRecPtr RedoRecPtr)
+XLOGfileslop(XLogRecPtr lastRedoRecPtr)
 {
 	XLogSegNo	minSegNo;
 	XLogSegNo	maxSegNo;
@@ -2311,9 +2311,9 @@ XLOGfileslop(XLogRecPtr RedoRecPtr)
 	 * correspond to. Always recycle enough segments to meet the minimum, and
 	 * remove enough segments to stay below the maximum.
 	 */
-	minSegNo = RedoRecPtr / wal_segment_size +
+	minSegNo = lastRedoRecPtr / wal_segment_size +
 		ConvertToXSegs(min_wal_size_mb, wal_segment_size) - 1;
-	maxSegNo = RedoRecPtr / wal_segment_size +
+	maxSegNo = lastRedoRecPtr / wal_segment_size +
 		ConvertToXSegs(max_wal_size_mb, wal_segment_size) - 1;
 
 	/*
@@ -2328,7 +2328,7 @@ XLOGfileslop(XLogRecPtr RedoRecPtr)
 	/* add 10% for good measure. */
 	distance *= 1.10;
 
-	recycleSegNo = (XLogSegNo) ceil(((double) RedoRecPtr + distance) /
+	recycleSegNo = (XLogSegNo) ceil(((double) lastRedoRecPtr + distance) /
 	wal_segment_size);
 
 	if (recycleSegNo < minSegNo)
@@ -3949,12 +3949,12 @@ RemoveTempXlogFiles(void)
 /*
  * Recycle or remove all log files older or equal to passed segno.
  *
- * endptr is current (or recent) end of xlog, and RedoRecPtr is the
+ * endptr is current (or recent) end of xlog, and lastRedoRecPtr is the
  * redo pointer of the last checkpoint. These are used to determine
  * whether we want to recycle rather than delete no-longer-wanted log files.
  */
 static void
-RemoveOldXlogFiles(XLogSegNo segno, XLogRecPtr RedoRecPtr, XLogRecPtr endptr)
+RemoveOldXlogFiles(XLogSegNo segno, XLogRecPtr lastRedoRecPtr, XLogRecPtr endptr)
 {
 	DIR		   *xldir;
 	struct dirent *xlde;
@@ -3997,7 +3997,7 @@ RemoveOldXlogFiles(XLogSegNo segno, XLogRecPtr RedoRecPtr, XLogRecPtr endptr)
 /* Update the last removed location in shared memory first */
 UpdateLastRemovedPtr(xlde->d_name);
 
-RemoveXlogFile(xlde->d_name, RedoRecPtr, endptr);
+RemoveXlogFile(xlde->d_name, lastRedoRecPtr, endptr);
 			}
 		}
 	}
@@ -4071,14 +4071,14 @@ RemoveNonParentXlogFiles(XLogRecPtr switchpoint, TimeLineID newTLI)
 /*
  * Recycle or remove a log file that's no longer needed.
  *
- * endptr is current (or recent) end of xlog, and RedoRecPtr is the
+ * endptr is current (or recent) end of xlog, and lastRedoRecPtr is the
  * redo pointer of the last checkpoint. These are used to determine
  * whether we want to recycle rather than delete no-longer-wanted log files.
- * If RedoRecPtr is not known, pass invalid, and the function will recycle,
+ * If lastRedoRecPtr is not known, pass invalid, and the function will recycle,
  * somewhat arbitrarily, 10 future segments.
  */
 static void
-RemoveXlogFile(const char *segname, XLogRecPtr RedoRecPtr, XLogRecPtr endptr)
+RemoveXlogFile(const char *segname, XLogRecPtr lastRedoRecPtr, XLogRecPtr endptr)
 {
 	char		path[MAXPGPATH];
 #ifdef WIN32
@@ -4094,10 +4094,10 @@ RemoveXlogFile(const char *segname, XLogRecPtr RedoRecPtr, XLogRecPtr endptr)
 		 * Initialize info about where to try to recycle to.
 		 */
 		XLByteToSeg(endptr, endlogSegNo, wal_segment_size);
-		if (RedoRecPtr == InvalidXLogRecPtr)
+		if (lastRedoRecPtr == InvalidXLogRecPtr)
 			recycleSegNo = endlogSegNo + 10;
 		else
-			recycleSegNo = XLOGfileslop(RedoRecPtr);
+			recycleSegNo = XLOGfileslop(lastRedoRecPtr);
 	}
 	else
 		recycleSegNo = 0;		/* keep compiler quiet */
@@ -7158,11 +7158,11 @@ StartupXLOG(void)
 
 	if (info == XLOG_CHECKPOINT_SHUTDOWN)
 	{
-		CheckPoint	checkPoint;
+		CheckPoint	chkptrec;
 
-		memcpy(&checkPoint, XLogRecGetData(xlogreader), sizeof(CheckPoint));
-		newTLI = checkPoint.ThisTimeLineID;
-		prevTLI = checkPoint.PrevTimeLineID;
+		memcpy(&chkptrec, XLogRecGetData(xlogreader), sizeof(CheckPoint));

Re: error context for vacuum to include block number

[Michael Paquier]

On Fri, Dec 06, 2019 at 10:23:25AM -0600, Justin Pryzby wrote:
> Find attached updated patch:
>  . Use structure to include relation name.
>  . Split into a separate patch rename of "StringInfoData buf".
> 
> 2019-11-27 20:04:53.640 CST [14244] ERROR:  canceling statement due to 
> statement timeout
> 2019-11-27 20:04:53.640 CST [14244] CONTEXT:  block 2314 of relation t
> 2019-11-27 20:04:53.640 CST [14244] STATEMENT:  vacuum t;
> 
> I tried to use BufferGetTag() to avoid using a 2ndary structure, but fails if
> the buffer is not pinned.

No problem from me to add more context directly in lazy_scan_heap().

+   // errcallback.arg = (void *) &buf;
The first patch is full of that, please make sure to clean it up.  

Let's keep also the message simple, still I think that it should be a
bit more explicative:
1) Let's the schema name, and quote the relation name.
2) Let's mention the scanning (or vacuuming) operation.

So I would suggest the following instead:
"while scanning block %u of relation \"%s.%s\"" 
--
Michael


signature.asc
Description: PGP signature

Re: BUG #16059: Tab-completion of filenames in COPY commands removes required quotes

[Alvaro Herrera]

All in all, after testing this for a bit, I think this patch is a clear
improvement over the statu quo.  Thanks for working on this.

I suggest to indicate in complete_from_files where to find the hook
functions it refers to (say "see quote_file_name, below", or something.)

I tested this on libreadline 7.x (where #define
HAVE_RL_FILENAME_COMPLETION_FUNCTION 1).  I noticed that if I enter a
filename that doesn't exist and then , it adds a closing quote.
Bash manages to do nothing somehow, which is the desired behavior IMO.

(I tried to make sense of these hooks, but couldn't readily and I don't
have the readline documentation installed, so I have no opinion on
whether this problem is fixable.  Maybe the trick is to let
quote_if_needed know that this is a completion for a filename, and have
it test for file existence?)

Also, some commands such as \cd want a directory rather than just any
file.  Not sure rl_filename_completion_function has a way to pass this
down.  (This point is a bit outside this patch's charter perhaps, but
may as well think about it since we're here ...)

I don't quite understand why a readline library that doesn't have
rl_filename_completion_function is known to have a
filename_completion_function, ie. this bit 

#ifdef HAVE_RL_FILENAME_COMPLETION_FUNCTION
#define filename_completion_function rl_filename_completion_function
#else
/* decl missing in some header files, but function exists anyway */
extern char *filename_completion_function();
#endif

What's going on here?  How does this ever work?

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: PATCH: logical_work_mem and logical streaming of large in-progress transactions

[Amit Kapila]

On Mon, Dec 9, 2019 at 1:27 PM Dilip Kumar  wrote:
>
> I have review the patch set and here are few comments/questions
>
> 1.
> +static void
> +pg_decode_stream_change(LogicalDecodingContext *ctx,
> + ReorderBufferTXN *txn,
> + Relation relation,
> + ReorderBufferChange *change)
> +{
> + OutputPluginPrepareWrite(ctx, true);
> + appendStringInfo(ctx->out, "streaming change for TXN %u", txn->xid);
> + OutputPluginWrite(ctx, true);
> +}
>
> Should we show the tuple in the streamed change like we do for the
> pg_decode_change?
>

I think so.  The patch shows the message in
pg_decode_stream_message(), so why to prohibit showing tuple here?

> 2. pg_logical_slot_get_changes_guts
> It recreate the decoding slot [ctx =
> CreateDecodingContext(InvalidXLogRecPtr] but doesn't set the streaming
> to false, should we pass a parameter to
> pg_logical_slot_get_changes_guts saying whether we want streamed results or 
> not
>

CreateDecodingContext internally calls StartupDecodingContext which
sets the value of streaming based on if the plugin has provided
callbacks for streaming functions. Isn't that sufficient?  Why do we
need additional parameters here?

> 3.
> + XLogRecPtr prev_lsn = InvalidXLogRecPtr;
>   ReorderBufferChange *change;
>   ReorderBufferChange *specinsert = NULL;
>
> @@ -1565,6 +1965,16 @@ ReorderBufferCommit(ReorderBuffer *rb, TransactionId 
> xid,
>   Relation relation = NULL;
>   Oid reloid;
>
> + /*
> + * Enforce correct ordering of changes, merged from multiple
> + * subtransactions. The changes may have the same LSN due to
> + * MULTI_INSERT xlog records.
> + */
> + if (prev_lsn != InvalidXLogRecPtr)
> + Assert(prev_lsn <= change->lsn);
> +
> + prev_lsn = change->lsn;
> I did not understand, how this change is relavent to this patch
>

This is just to ensure that changes are in LSN order.  I think as we
are merging the changes before commit for streaming, it is good to
have such an Assertion for ReorderBufferStreamTXN.   And, if we want
to have it in ReorderBufferStreamTXN, then there is no harm in keeping
it in ReorderBufferCommit() at least to keep the code consistent.  Do
you see any problem with this?

> 4.
> + /*
> + * TOCHECK: We have to rebuild historic snapshot to be sure it includes all
> + * information about subtransactions, which could arrive after streaming 
> start.
> + */
> + if (!txn->is_schema_sent)
> + snapshot_now = ReorderBufferCopySnap(rb, txn->base_snapshot,
> + txn, command_id);
>
> In which case, txn->is_schema_sent will be true, because at the end of
> the stream in ReorderBufferExecuteInvalidations we are always setting
> it false,
> so while sending next stream it will always be false.  That means we
> never required snapshot_now variable in ReorderBufferTXN.
>

You are probably right, but as discussed we need to change this part
of design/code (when to send schema changes) due to the issues
discovered.  So, I think this part will anyway change when we fix that
problem.

> 5.
> @@ -2299,6 +2746,23 @@ ReorderBufferXidSetCatalogChanges(ReorderBuffer
> *rb, TransactionId xid,
>   txn = ReorderBufferTXNByXid(rb, xid, true, NULL, lsn, true);
>
>   txn->txn_flags |= RBTXN_HAS_CATALOG_CHANGES;
> +
> + /*
> + * We read catalog changes from WAL, which are not yet sent, so
> + * invalidate current schema in order output plugin can resend
> + * schema again.
> + */
> + txn->is_schema_sent = false;
>
> Same as point 4, during decode time it will never be true.
>

Sure, my previous point's reply applies here as well.

> 6.
> + /* send fields */
> + pq_sendint64(out, commit_lsn);
> + pq_sendint64(out, txn->end_lsn);
> + pq_sendint64(out, txn->commit_time);
>
> Commit_time and end_lsn is used in standby_feedback
>

I don't understand what you mean by this.  Can you be a bit more clear?

>
> 7.
> + /* FIXME optimize the search by bsearch on sorted data */
> + for (i = nsubxacts; i > 0; i--)
> + {
> + if (subxacts[i - 1].xid == subxid)
> + {
> + subidx = (i - 1);
> + found = true;
> + break;
> + }
> + }
> We can not rollback intermediate subtransaction without rollbacking
> latest sub-transaction, so why do we need
> to search in the array?  It will always be the the last subxact no?
>

The same thing is already mentioned in the comments above this code
("XXX Or perhaps we can rely on the aborts to arrive in the reverse
order, i.e. from the inner-most subxact (when nested)? In which case
we could simply check the last element.").  I think what you are
saying is probably right, but we can leave this as it is for now
because this is a minor optimization which can be done later as well
if required.  However, if you see any correctness issue, then we can
discuss.


> 8.
> + /*
> + * send feedback to upstream
> + *
> + * XXX Probably should send a valid LSN. But which one?
> + */
> + send_feedback(InvalidXLogRecPtr, false, false);
>
> Why feedback is sent for every change?
>

I will study this part of the patch and let you know my opinion.

Few comments on this patch series:

0001-Immediately-

Re: pause recovery if pitr target not reached

[Leif Gunnar Erlandsen]

Adding patch written for 13dev from git

"Michael Paquier"  skrev 1. desember 2019 kl. 03:08:

> On Fri, Nov 22, 2019 at 11:26:59AM +, Leif Gunnar Erlandsen wrote:
> 
>> No it does not. It works well to demonstrate its purpose though.
>> And it might be to stop with FATAL would be more correct.
> 
> This is still under active discussion. Please note that the latest
> patch does not apply, so a rebase would be nice to have. I have moved
> the patch to next CF, waiting on author.
> --
> Michael


0004-pause-recovery-if-pitr-target-not-reached.patch
Description: Binary data

RE: [Proposal] Level4 Warnings show many shadow vars

[Ranier Vilela]

De: Stephen Frost
Enviadas: Terça-feira, 10 de Dezembro de 2019 17:52

>There's multiple ways to get there though and I think what you're seeing
>is that the "just change it to something else" answer isn't necessairly
>going to be viewed as an improvement (or, at least, not enough of an
>improvement to accept the cost of the change).
Well, I was trying to apply another non-implied rule, "break nothing".

>Why not change the variables?  Changes that also improve the code itself
>along with eliminating the shadowing of the global variable are going to
>be a lot easier to be accepted.
Contrary to what I was initially thinking, it seems to me that changing the 
names of global variables is more acceptable to the people of the project.

>Sure, but have you looked at how it's used?  Instead of just renaming
>the numTables variables in the functions that accept it- could those
>variables just be removed instead of changing their name to make it look
>like they're something different when they aren't actually different?
No. I didn't look.

>I've only spent a bit of time looking at it, but it sure looks like the
>variables could just be removed, and doing so doesn't break the
>regression tests, which supports the idea that maybe there's a better
>way to deal with those particular variables rather than renaming them.
>Another approach to consider might be to move some global variables into
>structures that are then global with better names to indicate that's
>what they are.
It does not seem reasonable to me what you are asking.
Because as I was told here and I agree in part. I do not have the necessary 
knowledge of structures and logic to propose big changes.
For the work I set out to, find bugs and make minor performance improvements, I 
believe, can contribute safely and without ruining anything.
By just changing the names of variables to something consistent and readable, 
the goal will be done without break anything.
Who is best to make these changes, are the authors and reviewers. Once we no 
longer have the problem of shadow variables, we can turn on the alert without 
breaking automatic compilation, as Tom Lane is concerned.
That's why I'd like to fix all collisions of variables, even the dumbest.

>In short, a hack-and-slash patch that doesn't really spend much time
>considering the changes beyond "let's just change these to be different
>to avoid shadowing globals" isn't really a good way to go about
>addressing these cases and has a good chance of making things more
>confusing, not less.
This is totally contrary to what I think about it.

regards,
Ranier Vilela

Re: Start Walreceiver completely before shut down it on standby server.

[Michael Paquier]

On Wed, Dec 11, 2019 at 04:06:26PM +0800, jiankang liu wrote:
> During my use of PG, I encountered such errors "incorrect resource manager
> data checksum in record at 0/5013730",
> it will keeps printing this error message and never stops on standby
> server，at same time, the walreceier process is lost.
> Maybe disk problem, but also the line is operating system problems, leading
> off the data disk error occurred.

An incorrect resource manager checksum points to the checksum of a
record to be busted, because the full record has been read from disk,
but it is failing a basic sanity check.  That's basically a sign of
corruption.  Do you have data checksums enabled?  If your WAL records
are broken, you have unfortunately good chances of having problems in
other areas of your data folder :(

At this stage, it would be wiser to run diagnostics on your server,
and be very careful with your chosen steps.  Here are guidelines on
the wiki:
https://wiki.postgresql.org/wiki/Corruption
--
Michael


signature.asc
Description: PGP signature

Re: get_database_name() from background worker

[Michael Paquier]

On Wed, Dec 11, 2019 at 05:17:00PM +0900, Koichi Suzuki wrote:
> Not using this extension, sorry.

I have no idea what you are trying to do, but get_database_name()
accesses the system cache, which means two things:
- The access needs to be done in the context of a transaction.  That's
a trick we use in a couple of places in core, see for example
IdentifySystem() in walsender.c which looks for the database name.  In
this case, you need to do the call in-between StartTransactionCommand
and CommitTransactionCommand, and you should make sure that the memory
context does not point to the one of the transaction to have an access
to the syscache data after committing the inner transaction used for
the lookup.
- Your background worker needs a database access, so bgw_flags needs
to be BGWORKER_SHMEM_ACCESS | BGWORKER_BACKEND_DATABASE_CONNECTION,
and also make sure to use BackgroundWorkerInitializeConnection at the
beginning of the main loop of your worker.

Hope that helps.
--
Michael


signature.asc
Description: PGP signature

Re: logical decoding : exceeded maxAllocatedDescs for .spill files

[Amit Khandekar]

On Sat, 7 Dec 2019 at 11:37, Amit Kapila  wrote:
>
> On Fri, Dec 6, 2019 at 5:00 PM Amit Khandekar  wrote:
> >
> > On Fri, 6 Dec 2019 at 15:40, Amit Kapila  wrote:
> > >
> > >
> > > Few comments:
> > > --
> > >
> > > 1.
> > > + /* Now that the state fields are initialized, it is safe to return it. 
> > > */
> > > + *iter_state = state;
> > > +
> > >   /* allocate heap */
> > >   state->heap =
> > > binaryheap_allocate(state->nr_txns,
> > > ReorderBufferIterCompare,
> > >
> > > Is there a reason for not initializing iter_state after
> > > binaryheap_allocate?  If we do so, then we don't need additional check
> > > you have added in ReorderBufferIterTXNFinish.
> >
> > If iter_state is initialized *after* binaryheap_allocate, then we
> > won't be able to close the vfds if binaryheap_allocate() ereports().
> >
>
> Is it possible to have vfds opened before binaryheap_allocate(), if so how?
No it does not look possible for the vfds to be opened before
binaryheap_allocate(). But actually, the idea behind placing the
iter_state at the place where I put is that, we should return back the
iter_state at the *earliest* place in the code where it is safe to
return.


> > I couldn't reproduce the original problem (on HEAD) reported with the
> > test case in the patch.  So, I can't verify the fix.  I think it is
> > because of recent commits cec2edfa7859279f36d2374770ca920c59c73dd8 and
> > 9290ad198b15d6b986b855d2a58d087a54777e87.  It seems you need to either
> > change the value of logical_decoding_work_mem or change the test in
> > some way so that the original problem can be reproduced.
Amit Khandekar  wrote:
> Yeah, it does seem like the commit cec2edfa78592 must have caused the
> test to not reproduce on your env, although the test does fail for me
> still. Setting logical_decoding_work_mem to a low value does sound
> like a good idea. Will work on it.

I checked that setting logical_decoding_work_mem to its min value
(64KB) causes early serialization. So I think if you set this, you
should be able to reproduce with the spill.sql test that has the new
testcase. I will anyway set this value in the test. Also check below
...

>> 4. I think we should also check how much time increase will happen for
>> test_decoding regression test after the test added by this patch?
Amit Khandekar  wrote:
> Yeah, it currently takes noticeably longer compared to the others.
> Let's see if setting logical_decoding_work_mem to a min value allows
> us to reproduce the test with much lesser number of inserts.

The test in the patch takes around 20 seconds, as compared to the max
time of 2 seconds any of the other tests take in that test suite.

But if we set the max_files_per_process to a very low value (say 26)
as against the default 1000, we can reproduce the issue with as low as
20 sub-transactions as against the 600 that I used in spill.sql test.
And with this, the test runs in around 4 seconds, so this is good. But
the problem is : max_files_per_process needs server restart. So either
we have to shift this test to src/test/recovery in one of the
logical_decoding test, or retain it in contrib/test_decoding and let
it run for 20 seconds. Let me know if you figure out any other
approach.


>
> > >
> > > 5. One naive question about the usage of PathNameOpenFile().  When it
> > > reaches the max limit, it will automatically close one of the files,
> > > but how will that be reflected in the data structure (TXNEntryFile)
> > > you are managing.  Basically, when PathNameOpenFile closes some file,
> > > how will the corresponding vfd in TXNEntryFile be changed. Because if
> > > it is not changed, then won't it start pointing to some wrong
> > > filehandle?
> >
> > In PathNameOpenFile(), excess kernel fds could be closed
> > (ReleaseLruFiles). But with that, the vfds themselves don't get
> > invalidated. Only the underlying kernel fd gets closed, and the
> > vfd->fd is marked VFD_CLOSED. The vfd array element remains valid (a
> > non-null vfd->fileName means the vfd slot is valid; check
> > FileIsValid). So later, when FileRead(vfd1) is called and that vfd1
> > happens to be the one that had got it's kernel fd closed, it gets
> > opened again through FileAccess().
> >
>
> I was under impression that once the fd is closed due to excess kernel
> fds that are opened, the slot in VfdCache array could be resued by
> someone else, but on closer inspection that is not true.  It will be
> only available for reuse after we explicitly call FileClose, right?

Yes, that's right.


-- 
Thanks,
-Amit Khandekar
EnterpriseDB Corporation
The Postgres Database Company

Re: Session WAL activity

[Konstantin Knizhnik]

On 11.12.2019 7:26, Kyotaro Horiguchi wrote:


Still I'm not sure non-atomic write is acceptable, but I agree on the
necessity of updating it during a transaction. Couldn't we update
shared stats every n bytes (XLOG_BLCKSZ or such) or every command end?

I think we should refrain from inserting an instruction within the
WALInsertLock section, but I'm not sure which is better between "var
+= var" within the section and "if (inserted) var += var;" outside. If
we can ignore the possitbility of the case where xlogswitch is
omitted, the "if (inserted)" is not needed.


I think that 32-bit Postgres installations are really exotic, but I 
agree that showing incorrect result (even with very small probability)
is not acceptable behavior in this case. I attached new versoin of the 
patch with use pg_atomic_write_u64 for updating walWritten field.
As far as at 64-bit systems, pg_atomic_write_u64and pg_atomic_read_u64 
are translated to ordinary memory access, them should not have some 
negative

impact on performance.


--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 5f0ee50..6a4b209 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -1486,6 +1486,9 @@ CopyXLogRecordToWAL(int write_len, bool isLogSwitch, XLogRecData *rdata,
 	currpos = GetXLogBuffer(CurrPos);
 	freespace = INSERT_FREESPACE(CurrPos);
 
+	/* Accumulate amount of data written to WAL for pg_xact_activity */
+	pg_atomic_write_u64(&MyProc->walWritten, pg_atomic_read_u64(&MyProc->walWritten) + write_len);
+
 	/*
 	 * there should be enough space for at least the first field (xl_tot_len)
 	 * on this page.
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 000cff3..037d313 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -758,6 +758,9 @@ CREATE VIEW pg_stat_activity AS
 LEFT JOIN pg_database AS D ON (S.datid = D.oid)
 LEFT JOIN pg_authid AS U ON (S.usesysid = U.oid);
 
+CREATE VIEW pg_stat_wal_activity AS
+SELECT a.*,pg_stat_get_wal_activity(a.pid) as wal_written FROM pg_stat_activity a;
+
 CREATE VIEW pg_stat_replication AS
 SELECT
 S.pid,
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index fff0628..c9a63a1 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -390,6 +390,8 @@ InitProcess(void)
 	MyPgXact->xid = InvalidTransactionId;
 	MyPgXact->xmin = InvalidTransactionId;
 	MyProc->pid = MyProcPid;
+	pg_atomic_init_u64(&MyProc->walWritten, 0);
+
 	/* backendId, databaseId and roleId will be filled in later */
 	MyProc->backendId = InvalidBackendId;
 	MyProc->databaseId = InvalidOid;
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index 05240bf..1745815 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -919,6 +919,22 @@ pg_stat_get_activity(PG_FUNCTION_ARGS)
 
 
 Datum
+pg_stat_get_wal_activity(PG_FUNCTION_ARGS)
+{
+	int32	pid = PG_GETARG_INT32(0);
+	PGPROC* proc = BackendPidGetProc(pid);
+	if (proc == NULL)
+	{
+		proc = AuxiliaryPidGetProc(pid);
+	}
+	if (proc == NULL)
+		PG_RETURN_NULL();
+	else
+		PG_RETURN_INT64(pg_atomic_read_u64(&proc->walWritten));
+}
+
+
+Datum
 pg_backend_pid(PG_FUNCTION_ARGS)
 {
 	PG_RETURN_INT32(MyProcPid);
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index b759b15..6d31afd 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -5152,6 +5152,10 @@
   proargmodes => '{i,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o}',
   proargnames => '{pid,datid,pid,usesysid,application_name,state,query,wait_event_type,wait_event,xact_start,query_start,backend_start,state_change,client_addr,client_hostname,client_port,backend_xid,backend_xmin,backend_type,ssl,sslversion,sslcipher,sslbits,sslcompression,ssl_client_dn,ssl_client_serial,ssl_issuer_dn,gss_auth,gss_princ,gss_enc}',
   prosrc => 'pg_stat_get_activity' },
+{ oid => '2121', descr => 'statistics: WAL activity',
+  proname => 'pg_stat_get_wal_activity', provolatile => 's', proisstrict => 't',
+  proparallel => 'r', prorettype => 'int8', proargtypes => 'int4',
+  prosrc => 'pg_stat_get_wal_activity' },
 { oid => '3318',
   descr => 'statistics: information about progress of backends running maintenance command',
   proname => 'pg_stat_get_progress_info', prorows => '100', proretset => 't',
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 281e1db..3aa2efb 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -203,6 +203,11 @@ struct PGPROC
 	PGPROC	   *lockGroupLeader;	/* lock group leader, if I'm a member */
 	dlist_head	lockGroupMembers;	/* list of members, if I'm a leader */
 	dlist_node	lockGroupLink;	/* my me

RE: get_database_name() from background worker

[ROS Didier]

So, What are you using to create background workers ? Can you show me an 
extract of your code ?
TIA
Best Regards
Didier

De : koi...@2ndquadrant.com [mailto:koi...@2ndquadrant.com]
Envoyé : mercredi 11 décembre 2019 09:16
À : ROS Didier 
Cc : tsunakawa.ta...@fujitsu.com; pgsql-hackers@lists.postgresql.org
Objet : Re: get_database_name() from background worker

I'm not using this.   Is this the must to use get_database_name()?
---
Koichi Suzuki


2019年12月11日(水) 16:26 ROS Didier mailto:didier@edf.fr>>:
Hi
I would like to know : Are you using pg_background extension to work 
with backgroud workers ?

Thanks in advance

Best Regards

Didier ROS
Expertise SGBD
EDF - DTEO - DSIT - IT DMA


-Message d'origine-
De : tsunakawa.ta...@fujitsu.com 
[mailto:tsunakawa.ta...@fujitsu.com]
Envoyé : mercredi 11 décembre 2019 08:21
À : 'Koichi Suzuki' mailto:koi...@2ndquadrant.com>>
Cc : 
pgsql-hackers@lists.postgresql.org
Objet : RE: get_database_name() from background worker

From: Koichi Suzuki mailto:koi...@2ndquadrant.com>>
> I'm writing an extension running on background workers and found
> get_database_name() causes SEGV and found internally resource owner was
> wet to NULL.   Could anybody let me know how it happens and how I can use
> this function.   Argument to get_database_name() looks correct.

Did you specify BGWORKER_BACKGROUND_DATABASE_CONNECTION when registering the 
background worker?
Did you start transaction by calling StartTransactionCommand()?


Regards
Takayuki Tsunakawa





Ce message et toutes les pièces jointes (ci-après le 'Message') sont établis à 
l'intention exclusive des destinataires et les informations qui y figurent sont 
strictement confidentielles. Toute utilisation de ce Message non conforme à sa 
destination, toute diffusion ou toute publication totale ou partielle, est 
interdite sauf autorisation expresse.

Si vous n'êtes pas le destinataire de ce Message, il vous est interdit de le 
copier, de le faire suivre, de le divulguer ou d'en utiliser tout ou partie. Si 
vous avez reçu ce Message par erreur, merci de le supprimer de votre système, 
ainsi que toutes ses copies, et de n'en garder aucune trace sur quelque support 
que ce soit. Nous vous remercions également d'en avertir immédiatement 
l'expéditeur par retour du message.

Il est impossible de garantir que les communications par messagerie 
électronique arrivent en temps utile, sont sécurisées ou dénuées de toute 
erreur ou virus.


This message and any attachments (the 'Message') are intended solely for the 
addressees. The information contained in this Message is confidential. Any use 
of information contained in this Message not in accord with its purpose, any 
dissemination or disclosure, either whole or partial, is prohibited except 
formal approval.

If you are not the addressee, you may not copy, forward, disclose or use any 
part of it. If you have received this message in error, please delete it and 
all copies from your system and notify the sender immediately by return message.

E-mail communication cannot be guaranteed to be timely secure, error or 
virus-free.



Ce message et toutes les pièces jointes (ci-après le 'Message') sont établis à 
l'intention exclusive des destinataires et les informations qui y figurent sont 
strictement confidentielles. Toute utilisation de ce Message non conforme à sa 
destination, toute diffusion ou toute publication totale ou partielle, est 
interdite sauf autorisation expresse.

Si vous n'êtes pas le destinataire de ce Message, il vous est interdit de le 
copier, de le faire suivre, de le divulguer ou d'en utiliser tout ou partie. Si 
vous avez reçu ce Message par erreur, merci de le supprimer de votre système, 
ainsi que toutes ses copies, et de n'en garder aucune trace sur quelque support 
que ce soit. Nous vous remercions également d'en avertir immédiatement 
l'expéditeur par retour du message.

Il est impossible de garantir que les communications par messagerie 
électronique arrivent en temps utile, sont sécurisées ou dénuées de toute 
erreur ou virus.


This message and any attachments (the 'Message') are intended solely for the 
addressees. The information contained in this Message is confidential. Any use 
of information contained in this Message not in accord with its purpose, any 
dissemination or disclosure, either whole or partial, is prohibited except 
formal approval.

If you are not the addressee, you may not copy, forward, disclose or use any 
part of it. If you have received this message in error, please delete it and 
all copies from your system and notify the sender immediately by return message.

E-mail communication cannot be guaranteed to be timely secure, error or 
virus-free.

Re: Start Walreceiver completely before shut down it on standby server.

[jiankang liu]

I'm sorry I did not say it clearly.

During my use of PG, I encountered such errors "incorrect resource manager
data checksum in record at 0/5013730",
it will keeps printing this error message and never stops on standby
server，at same time, the walreceier process is lost.
In a few months, we encountered this situation twice, each time testing
more than 200 connections to read/write, after 2 or 3 days of continuous
operation.
Maybe disk problem, but also the line is operating system problems, leading
off the data disk error occurred.

It will print this error never stops, and the walreceiver is lost, if we do
nothing.
Just restart standby server of PG, it only print this error message once,
and then connect to master. Everthing is OK.
Why we neet to restart my server, it can not fixs that problem online? Why
the walreceiver is lost?

The record has been flushed to disk by the walreceiver, and the the startup
process always read record and apply it. When it reads an invalid record,
it will shut down the walreceiver by signal SIGTERM. Then, it will read
from ARCHIVE/PG_WAL, just read files from pg_wal. Read an invalid record
again or read the file end(the read len is not equals XLOG_BLCKSZ), the
startup process will starts the walreceiver by RequestXLogStreaming() and
switch to read from XLOG_FROM_STREAM.

In RequestXLogStreaming(), set the walrcv->receiveStart = recptr, the
walreceiver will get the WAL from master start at  walrcv->receiveStart. So
we can read the new data which streaming from master by the walreceiver
this time, instead of the wrong data on disk. It should not print the error
message never stop and the walreceiver should not be lost after we read an
invalid record. But the fact is not work.

What  happened?
The previous step, the startup process starts the walreceiver, and switch
to read from XLOG_FROM_STREAM. Then, check the walreceiver is active before
we read it, even the postmaster does not start the walreceiver,  but the
walrcv->walRcvState == STARTING, we think the walreceiver is active, and
ready to read.

Now, begin to read data if  new data has arrived. How to check it?
If the Recptr, which is pointer we read, is lower than
walrcv->receivedUpto, we can read the data, even if the walreceiver does
not start completely and the data is OLD  which has invalid reccord.
Read it, and read an invalid reccord again, just stop the walreceiver
again(the walreceiver does not start completely, it has not pid, just set
walrcv->walRcvState = WALRCV_STOPPED). When the walreceiver starts, running
into WalReceiverMain(), check the walrcv->walRcvState == WALRCV_STOPPED,
myself has been shut down by others, just exit. So the walreceiver starts,
exit again and again.
The startup process next to do is, starts the walreceiver, read data(read
the invalid record), shut down the walreceiver, also agiain and again.

Why restart standby server of PG will be OK?
The startup process begin to REDO, reads an invalid record, prints the
error message, and starts the walreceiver by RequestXLogStreaming() and
switch to read from XLOG_FROM_STREAM. This is first time to start the
walreceiver, set walrcv->receivedUpto = walrcv->receiveStart = recptr.
The startup process ready to read new data, but RecPtr >=
walrcv->receivedUpto, wait the walreceiver get WAL from master.
So that, we get the WAL from master instead of the WAL on disk, by restart
standby server of PG.

By my fix, ervery time we start the walreceiver, the startup process will
wait for new data instead of read OLD data, Just like restart standby
server.
So, we can fix the problem online and the walreceiver will not be lost.


Kyotaro Horiguchi  于2019年12月11日周三 下午1:38写道：

> At Tue, 10 Dec 2019 10:40:53 -0800, Ashwin Agrawal 
> wrote in
> > On Tue, Dec 10, 2019 at 3:06 AM jiankang liu 
> wrote:
> >
> > > Start Walreceiver completely before shut down it on standby server.
> > >
> > > The walreceiver will be shut down, when read an invalid record in the
> > > WAL streaming from master.And then, we retry from archive/pg_wal again.
> > >
> > > After that, we start walreceiver in RequestXLogStreaming(), and read
> > > record from the WAL streaming. But before walreceiver starts, we read
> > > data from file which be streamed over and present in pg_wal by last
> > > time, because of walrcv->receivedUpto > RecPtr and the wal is actually
> > > flush on disk. Now, we read the invalid record again, what the next to
> > > do? Shut down the walreceiver and do it again.
> > >
> >
> > I am missing something here, if walrcv->receivedUpto > RecPtr, why are we
> > getting / reading invalid record?
>
> I bet on that the standby is connecting to a wrong master. For
> example, something like happens when the master has been reinitalized
> from a backup and experienced another history, then the standby was
> initialized from the reborn master but the stale archive files on the
> standby are left alone.
>
> Anyway that cannot happen on correctly running replication set and
> what to 

Re: logical decoding bug: segfault in ReorderBufferToastReplace()

[Drouvot, Bertrand]

On 12/9/19, 10:10 AM, "Tomas Vondra"  wrote:
>On Wed, Dec 04, 2019 at 05:36:16PM -0800, Jeremy Schneider wrote:
>>On 9/8/19 14:01, Tom Lane wrote:
>>> Fix RelationIdGetRelation calls that weren't bothering with error 
checks.
>>>
>>> ...
>>>
>>> Details
>>> ---
>>> 
https://git.postgresql.org/pg/commitdiff/69f883fef14a3fc5849126799278abcc43f40f56
>>
>>We had two different databases this week (with the same schema) both
>>independently hit the condition of this recent commit from Tom. It's on
>>11.5 so we're actually segfaulting and restarting rather than just
>>causing the walsender process to ERROR, but regardless there's still
>>some underlying bug here.
>>
>>We have core files and we're still working to see if we can figure out
>>what's going on, but I thought I'd report now in case anyone has extra
>>ideas or suggestions.  The segfault is on line 3034 of reorderbuffer.c.
>>

>>https://github.com/postgres/postgres/blob/REL_11_5/src/backend/replication/logical/reorderbuffer.c#L3034
>>
>>3033 toast_rel = 
RelationIdGetRelation(relation->rd_rel->reltoastrelid);
>>3034 toast_desc = RelationGetDescr(toast_rel);
>>
>>We'll keep looking; let me know any feedback! Would love to track down
>>whatever bug is in the logical decoding code, if that's what it is.
>>
>>==
>>
>>backtrace showing the call stack...
>>
>>Core was generated by `postgres: walsender 
>>(31712)'.
>>Program terminated with signal 11, Segmentation fault.
>>#0  ReorderBufferToastReplace (rb=0x3086af0, txn=0x3094a78,
>>relation=0x2b79177249c8, relation=0x2b79177249c8, change=0x30ac938)
>>at reorderbuffer.c:3034
>>3034reorderbuffer.c: No such file or directory.
>>...
>>(gdb) #0  ReorderBufferToastReplace (rb=0x3086af0, txn=0x3094a78,
>>relation=0x2b79177249c8, relation=0x2b79177249c8, change=0x30ac938)
>>at reorderbuffer.c:3034
>>#1  ReorderBufferCommit (rb=0x3086af0, xid=xid@entry=1358809,
>>commit_lsn=9430473346032, end_lsn=,
>>commit_time=commit_time@entry=628712466364268,
>>origin_id=origin_id@entry=0, origin_lsn=origin_lsn@entry=0) at
>>reorderbuffer.c:1584
>>#2  0x00716248 in DecodeCommit (xid=1358809,
>>parsed=0x7ffc4ce123f0, buf=0x7ffc4ce125b0, ctx=0x3068f70) at decode.c:637
>>#3  DecodeXactOp (ctx=0x3068f70, buf=buf@entry=0x7ffc4ce125b0) at
>>decode.c:245
>>#4  0x0071655a in LogicalDecodingProcessRecord (ctx=0x3068f70,
>>record=0x3069208) at decode.c:117
>>#5  0x00727150 in XLogSendLogical () at walsender.c:2886
>>#6  0x00729192 in WalSndLoop (send_data=send_data@entry=0x7270f0
>>) at walsender.c:2249
>>#7  0x00729f91 in StartLogicalReplication (cmd=0x30485a0) at
>>walsender.c:
>>#8  exec_replication_command (
>>cmd_string=cmd_string@entry=0x2f968b0 "START_REPLICATION SLOT
>>\"\" LOGICAL 893/38002B98 (proto_version '1',
>>publication_names '\"\"')") at walsender.c:1628
>>#9  0x0076e939 in PostgresMain (argc=,
>>argv=argv@entry=0x2fea168, dbname=0x2fea020 "",
>>username=) at postgres.c:4182
>>#10 0x004bdcb5 in BackendRun (port=0x2fdec50) at postmaster.c:4410
>>#11 BackendStartup (port=0x2fdec50) at postmaster.c:4082
>>#12 ServerLoop () at postmaster.c:1759
>>#13 0x007062f9 in PostmasterMain (argc=argc@entry=7,
>>argv=argv@entry=0x2f92540) at postmaster.c:1432
>>#14 0x004be73b in main (argc=7, argv=0x2f92540) at main.c:228
>>
>>==
>>
>>Some additional context...
>>
>># select * from pg_publication_rel;
>> prpubid | prrelid
>>-+-
>>   71417 |   16453
>>   71417 |   54949
>>(2 rows)
>>
>>(gdb) print toast_rel
>>$4 = (struct RelationData *) 0x0
>>
>>(gdb) print *relation->rd_rel
>>$11 = {relname = {data = "", '\000' },
>>relnamespace = 16402, reltype = 16430, reloftype = 0,
>>relowner = 16393, relam = 0, relfilenode = 16428, reltablespace = 0,
>>relpages = 0, reltuples = 0, relallvisible = 0, reltoastrelid = 0,

>Hmmm, so reltoastrelid = 0, i.e. the relation does not have a TOAST
>relation. Yet we're calling ReorderBufferToastReplace on the decoded
>record ... interesting.
>
>Can you share structure of the relation causing the issue?

   Here it is:

\d+ rel_having_issue
 Table 
"public.rel_having_issue"
 Column |   Type   | Collation | Nullable | 
Default | Storage  | Stats target | Description
+--+---+--+-+--+--+-
 id | integer  

Re: about allow_system_table_mods and SET STATISTICS

[Peter Eisentraut]

On 2019-12-10 17:23, Tom Lane wrote:

Peter Eisentraut  writes:

Good point.  Done in the attached patch.
(If someone wanted to revive the original functionality, it would
nowadays probably be easier to add a flag ATT_SYSTEM_TABLE to
ATSimplePermissions(), so there is really no reason to keep the old
function separate.)


Yeah --- that way, the behavior would also be conveniently available
to other ALTER TABLE subcommands.

This patch looks good, with one trivial nitpick: it looks a bit odd
to insert the relkind check into ATExecSetStatistics between the
assignment of "newtarget" and the validity check for same.  I'd
put it either before or after that whole stanza.  Just a cosmetic
thing though.


Committed that way.  Thanks.

--
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: get_database_name() from background worker

[Koichi Suzuki]

Not using this extension, sorry.
---
Koichi Suzuki


2019年12月11日(水) 16:26 ROS Didier :

> Hi
> I would like to know : Are you using pg_background extension to
> work with backgroud workers ?
>
> Thanks in advance
>
> Best Regards
>
> Didier ROS
> Expertise SGBD
> EDF - DTEO - DSIT - IT DMA
>
>
> -Message d'origine-
> De : tsunakawa.ta...@fujitsu.com [mailto:tsunakawa.ta...@fujitsu.com]
> Envoyé : mercredi 11 décembre 2019 08:21
> À : 'Koichi Suzuki' 
> Cc : pgsql-hackers@lists.postgresql.org
> Objet : RE: get_database_name() from background worker
>
> From: Koichi Suzuki 
> > I'm writing an extension running on background workers and found
> > get_database_name() causes SEGV and found internally resource owner was
> > wet to NULL.   Could anybody let me know how it happens and how I can use
> > this function.   Argument to get_database_name() looks correct.
>
> Did you specify BGWORKER_BACKGROUND_DATABASE_CONNECTION when registering
> the background worker?
> Did you start transaction by calling StartTransactionCommand()?
>
>
> Regards
> Takayuki Tsunakawa
>
>
>
>
>
> Ce message et toutes les pièces jointes (ci-après le 'Message') sont
> établis à l'intention exclusive des destinataires et les informations qui y
> figurent sont strictement confidentielles. Toute utilisation de ce Message
> non conforme à sa destination, toute diffusion ou toute publication totale
> ou partielle, est interdite sauf autorisation expresse.
>
> Si vous n'êtes pas le destinataire de ce Message, il vous est interdit de
> le copier, de le faire suivre, de le divulguer ou d'en utiliser tout ou
> partie. Si vous avez reçu ce Message par erreur, merci de le supprimer de
> votre système, ainsi que toutes ses copies, et de n'en garder aucune trace
> sur quelque support que ce soit. Nous vous remercions également d'en
> avertir immédiatement l'expéditeur par retour du message.
>
> Il est impossible de garantir que les communications par messagerie
> électronique arrivent en temps utile, sont sécurisées ou dénuées de toute
> erreur ou virus.
> 
>
> This message and any attachments (the 'Message') are intended solely for
> the addressees. The information contained in this Message is confidential.
> Any use of information contained in this Message not in accord with its
> purpose, any dissemination or disclosure, either whole or partial, is
> prohibited except formal approval.
>
> If you are not the addressee, you may not copy, forward, disclose or use
> any part of it. If you have received this message in error, please delete
> it and all copies from your system and notify the sender immediately by
> return message.
>
> E-mail communication cannot be guaranteed to be timely secure, error or
> virus-free.
>

Re: get_database_name() from background worker

[Koichi Suzuki]

I'm not using this.   Is this the must to use get_database_name()?
---
Koichi Suzuki


2019年12月11日(水) 16:26 ROS Didier :

> Hi
> I would like to know : Are you using pg_background extension to
> work with backgroud workers ?
>
> Thanks in advance
>
> Best Regards
>
> Didier ROS
> Expertise SGBD
> EDF - DTEO - DSIT - IT DMA
>
>
> -Message d'origine-
> De : tsunakawa.ta...@fujitsu.com [mailto:tsunakawa.ta...@fujitsu.com]
> Envoyé : mercredi 11 décembre 2019 08:21
> À : 'Koichi Suzuki' 
> Cc : pgsql-hackers@lists.postgresql.org
> Objet : RE: get_database_name() from background worker
>
> From: Koichi Suzuki 
> > I'm writing an extension running on background workers and found
> > get_database_name() causes SEGV and found internally resource owner was
> > wet to NULL.   Could anybody let me know how it happens and how I can use
> > this function.   Argument to get_database_name() looks correct.
>
> Did you specify BGWORKER_BACKGROUND_DATABASE_CONNECTION when registering
> the background worker?
> Did you start transaction by calling StartTransactionCommand()?
>
>
> Regards
> Takayuki Tsunakawa
>
>
>
>
>
> Ce message et toutes les pièces jointes (ci-après le 'Message') sont
> établis à l'intention exclusive des destinataires et les informations qui y
> figurent sont strictement confidentielles. Toute utilisation de ce Message
> non conforme à sa destination, toute diffusion ou toute publication totale
> ou partielle, est interdite sauf autorisation expresse.
>
> Si vous n'êtes pas le destinataire de ce Message, il vous est interdit de
> le copier, de le faire suivre, de le divulguer ou d'en utiliser tout ou
> partie. Si vous avez reçu ce Message par erreur, merci de le supprimer de
> votre système, ainsi que toutes ses copies, et de n'en garder aucune trace
> sur quelque support que ce soit. Nous vous remercions également d'en
> avertir immédiatement l'expéditeur par retour du message.
>
> Il est impossible de garantir que les communications par messagerie
> électronique arrivent en temps utile, sont sécurisées ou dénuées de toute
> erreur ou virus.
> 
>
> This message and any attachments (the 'Message') are intended solely for
> the addressees. The information contained in this Message is confidential.
> Any use of information contained in this Message not in accord with its
> purpose, any dissemination or disclosure, either whole or partial, is
> prohibited except formal approval.
>
> If you are not the addressee, you may not copy, forward, disclose or use
> any part of it. If you have received this message in error, please delete
> it and all copies from your system and notify the sender immediately by
> return message.
>
> E-mail communication cannot be guaranteed to be timely secure, error or
> virus-free.
>