Re: Why is src/test/modules/committs/t/002_standby.pl flaky?

[Alexander Lakhin]

Hello Tom,
09.01.2022 04:17, Tom Lane wrote:
>> So for some reason, on these machines detection of walsender-initiated
>> connection close is unreliable ... or maybe, the walsender didn't close
>> the connection, but is somehow still hanging around?  Don't have much idea
>> where to dig beyond that, but maybe someone else will.  I wonder in
>> particular if this could be related to our recent discussions about
>> whether to use shutdown(2) on Windows --- could we need to do the
>> equivalent of 6051857fc/ed52c3707 on walsender connections?
> ... wait a minute.  After some more study of the buildfarm logs,
> it was brought home to me that these failures started happening
> just after 6051857fc went in:
>
> https://buildfarm.postgresql.org/cgi-bin/show_failures.pl?max_days=90&branch=&member=&stage=module-commit_tsCheck&filter=Submit
>
> The oldest matching failure is jacana's on 2021-12-03.
> (The above sweep finds an unrelated-looking failure on 2021-11-11,
> but no others before 6051857fc went in on 2021-12-02.  Also, it
> looks likely that ed52c3707 on 2021-12-07 made the failure more
> probable, because jacana's is the only matching failure before 12-07.)
>
> So I'm now thinking it's highly likely that those commits are
> causing it somehow, but how?
>
I've managed to reproduce this failure too.
Removing "shutdown(MyProcPort->sock, SD_SEND);" doesn't help here, so
the culprit is exactly "closesocket(MyProcPort->sock);".
I've added `system("netstat -ano");` before die() in 002_standby.pl and see:
# Postmaster PID for node "primary" is 944
  Proto  Local Address  Foreign Address    State   PID
...
  TCP    127.0.0.1:58545    127.0.0.1:61995    FIN_WAIT_2  944
...
  TCP    127.0.0.1:61995    127.0.0.1:58545    CLOSE_WAIT  1352

(Replacing SD_SEND with SD_BOTH doesn't change the behaviour.)

Looking at the libpqwalreceiver.c:
    /* Now that we've consumed some input, try again */
        rawlen = PQgetCopyData(conn->streamConn, &conn->recvBuf, 1);
here we get -1 on the primary disconnection.
Then we get COMMAND_OK here:
        res = libpqrcv_PQgetResult(conn->streamConn);
        if (PQresultStatus(res) == PGRES_COMMAND_OK)
and finally just hang at:
            /* Verify that there are no more results. */
            res = libpqrcv_PQgetResult(conn->streamConn);
until the standby gets interrupted by the TAP test. (That call can also
return NULL and then the test completes successfully.)
Going down through the call chain, I see that at the end of it
WaitForMultipleObjects() hangs while waiting for the primary connection
socket event. So it looks like the socket, that is closed by the
primary, can get into a state unsuitable for WaitForMultipleObjects().
I tried to check the socket state with the WSAPoll() function and
discovered that it returns POLLHUP for the "problematic" socket.
The following draft addition in latch.c:
int
WaitLatchOrSocket(Latch *latch, int wakeEvents, pgsocket sock,
                  long timeout, uint32 wait_event_info)
{
    int            ret = 0;
    int            rc;
    WaitEvent    event;

#ifdef WIN32
    if (wakeEvents & WL_SOCKET_MASK) {
        WSAPOLLFD pollfd;
        pollfd.fd = sock;
        pollfd.events = POLLRDNORM | POLLWRNORM;
        pollfd.revents = 0;
        int rc = WSAPoll(&pollfd, 1, 0);
        if ((rc == 1) && (pollfd.revents & POLLHUP)) {
            elog(WARNING, "WaitLatchOrSocket: A stream-oriented
connection was either disconnected or aborted.");
            return WL_SOCKET_MASK;
        }
    }
#endif

makes the test 002_stanby.pl pass (100 of 100 iterations, while without
the fix I get failures roughly on each third). I'm not sure where to
place this check, maybe it's better to move it up to
libpqrcv_PQgetResult() to minimize it's footprint or to find less
Windows-specific approach, but I'd prefer a client-side fix anyway, as
graceful closing a socket by a server seems a legitimate action.

Best regards,
Alexander

Re: [PATCH] reduce page overlap of GiST indexes built using sorted method

[sergei sh.]

Hi,

here are some benchmark results for GiST patch: 
https://gist.github.com/mngr777/88ae200c9c30ba5656583d92e8d2cf9e


Code used for benchmarking: https://github.com/mngr777/pg_index_bm2,
see README for the list of test queries.

Results show query performance close to indexes built with no 
sortsupport function, with index creation time reduced approx. by half.


On 1/8/22 10:20 PM, Aliaksandr Kalenik wrote:
After further testing, here is v2 where the issue that rightlink can be 
set when an index page is already flushed is fixed.


On Sat, Dec 25, 2021 at 4:35 PM Andrey Borodin > wrote:



Hi Aliaksandr!

Thanks for working on this!

 > Benchmark summary:
 >
 > create index roads_rdr_idx on roads_rdr using gist (geom);
 >
 > with sort support before patch / CREATE INDEX 76.709 ms
 >
 > with sort support after patch / CREATE INDEX 225.238 ms
 >
 > without sort support / CREATE INDEX 446.071 ms
 >
 > select count(*) from roads_rdr a, roads_rdr b where a.geom && b.geom;
 >
 > with sort support before patch / SELECT 5766.526 ms
 >
 > with sort support after patch / SELECT 2646.554 ms
 >
 > without sort support / SELECT 2721.718 ms
 >
 > index size
 >
 > with sort support before patch / IDXSIZE 2940928 bytes
 >
 > with sort support after patch / IDXSIZE 4956160 bytes
 >
 > without sort support / IDXSIZE 5447680 bytes

The numbers are impressive, newly build index is actually performing
better!
I've conducted some tests over points, not PostGIS geometry. For
points build is 2x slower now, but this is the cost of faster scans.

Some strong points of this index building technology.
The proposed algorithm is not randomly chosen as anything that
performs better than the original sorting build. We actually
researched every idea we knew from literature and intuition.
Although we consciously limited the search area by existing GiST API.
Stuff like penalty-based choose-subtree and split in equal halves
seriously limit possible solutions. If anyone knows an any better
way to build GiST faster or with better scan performance - please
let us know.
The proposed algorithm contains the current algorithm as a special
case: there is a parameter - the number of buffers accumulated
before calling Split. If this parameter is 1 proposed algorithm will
produce exactly the same output.

At this stage, we would like to hear some feedback from Postgres and
PostGIS community. What other performance aspects should we test?

Current patch implementation has some known deficiencies:
1. We need a GUC instead of the hard-coded buffer of 8 pages.
2. Is GiST sorting build still deterministic? If not - we should add
a fixed random seed into pageinspect tests.
3. It would make sense to check the resulting indexes with amcheck
[0], although it's not committed.
4. We cannot make an exact fillfactor due to the behavior of
picksplit. But can we improve anything here? I think if not - it's
still OK.
5. GistSortedBuildPageState is no more page state. It's Level state
or something like that.
6. The patch desperately needs comments.


Thanks!

Best regards, Andrey Borodin.

[0]

https://www.postgresql.org/message-id/flat/59D0DA6B-1652-4D44-B0EF-A582D5824F83%40yandex-team.ru

Re: Multiple Query IDs for a rewritten parse tree

[Dmitry Dolgov]

> On Sat, Jan 08, 2022 at 07:49:59PM -0500, Tom Lane wrote:
>
> The idea I'd been vaguely thinking about is to allow attaching a list
> of query-hash nodes to a Query, where each node would contain a "tag"
> identifying the specific hash calculation method, and also the value
> of the query's hash calculated according to that method.  We could
> probably get away with saying that all such hash values must be uint64.
> The main difference from your function-OID idea, I think, is that
> I'm envisioning the tags as being small integers with well-known
> values, similarly to the way we manage stakind values in pg_statistic.
> In this way, an extension that wants a hash that the core knows how
> to calculate doesn't need its own copy of the code, and similarly
> one extension could publish a calculation method for use by other
> extensions.

An extension that wants a slightly modified version of hash calculation
implementation from the core would still need to copy everything. The
core probably has to provide more than one (hash, method) pair to cover
some basic needs.

Re: Multiple Query IDs for a rewritten parse tree

[Julien Rouhaud]

On Sun, Jan 09, 2022 at 12:43:06PM +0100, Dmitry Dolgov wrote:
>
> An extension that wants a slightly modified version of hash calculation
> implementation from the core would still need to copy everything. The
> core probably has to provide more than one (hash, method) pair to cover
> some basic needs.

Or just GUC(s) to adapt the behavior.  But in any case there isn't much that
can be done that won't result in a huge performance drop (like e.g. the wanted
stability over logical replication or backup/restore).

Re: Multiple Query IDs for a rewritten parse tree

[Julien Rouhaud]

On Sat, Jan 08, 2022 at 07:49:59PM -0500, Tom Lane wrote:
>
> The idea I'd been vaguely thinking about is to allow attaching a list
> of query-hash nodes to a Query, where each node would contain a "tag"
> identifying the specific hash calculation method, and also the value
> of the query's hash calculated according to that method.

For now the queryid mixes two different things: fingerprinting and query text
normalization.  Should each calculation method be allowed to do a different
normalization too, and if yes where should be stored the state data needed for
that?  If not, we would need some kind of primary hash for that purpose.

Looking at Andrey's use case for wanting multiple hashes, I don't think that
adaptive optimization needs a normalized query string.  The only use would be
to output some statistics, but this could be achieved by storing a list of
"primary queryid" for each adaptive entry.  That's probably also true for
anything that's not monitoring intended.  Also, all monitoring consumers should
probably agree on the same queryid, both fingerprint and normalized string, as
otherwise it's impossible to cross-reference metric data.

Re: null iv parameter passed to combo_init()

[Zhihong Yu]

On Sat, Jan 8, 2022 at 11:32 PM Tom Lane  wrote:

> Noah Misch  writes:
> > On further thought, I would write it this way:
>
> > - else
> > + else if (ivlen != 0)
> >   memcpy(ivbuf, iv, ivlen);
>
> FWIW, I liked the "ivlen > 0" formulation better.  They should be
> equivalent, because ivlen is unsigned, but it just seems like "> 0"
> is more natural.
>
> regards, tom lane
>

Patch v4 is attached.

Cheers


0004-memcpy-null.patch
Description: Binary data

Add lasterrno setting for dir_existsfile()

[Wei Sun]

Hi,



Some time ago,the following patch clean up error handling in pg_basebackup's 
walmethods.c.
https://github.com/postgres/postgres/commit/248c3a9


This patch keep the error state in the DirectoryMethodData struct,
in most functions, the lasterrno is set correctly, but in function 
dir_existsfile(), 
the lasterrno is not set when the file fails to open.




If this is a correction omission, I think this patch can fix this.


Cheers

add_lasterrno_setting.patch
Description: Binary data

Re: null iv parameter passed to combo_init()

[Noah Misch]

On Sun, Jan 09, 2022 at 04:37:32AM -0800, Zhihong Yu wrote:
> On Sat, Jan 8, 2022 at 11:32 PM Tom Lane  wrote:
> > Noah Misch  writes:
> > > On further thought, I would write it this way:
> >
> > > - else
> > > + else if (ivlen != 0)
> > >   memcpy(ivbuf, iv, ivlen);
> >
> > FWIW, I liked the "ivlen > 0" formulation better.  They should be
> > equivalent, because ivlen is unsigned, but it just seems like "> 0"
> > is more natural.

If I were considering the one code site in isolation, I'd pick "ivlen > 0".
But of the four sites identified so far, three have signed length variables.
Since we're likely to get more examples of this pattern, some signed and some
unsigned, I'd rather use a style that does the optimal thing whether or not
the variable is signed.  What do you think?

> Patch v4 is attached.

Does this pass the test procedure shown upthread?

Re: Why is src/test/modules/committs/t/002_standby.pl flaky?

[Tom Lane]

Alexander Lakhin  writes:
> 09.01.2022 04:17, Tom Lane wrote:
>> ... wait a minute.  After some more study of the buildfarm logs,
>> it was brought home to me that these failures started happening
>> just after 6051857fc went in:

> I've managed to reproduce this failure too.
> Removing "shutdown(MyProcPort->sock, SD_SEND);" doesn't help here, so
> the culprit is exactly "closesocket(MyProcPort->sock);".

Ugh.  Did you try removing the closesocket and keeping shutdown?
I don't recall if we tried that combination before.

> ...  I'm not sure where to
> place this check, maybe it's better to move it up to
> libpqrcv_PQgetResult() to minimize it's footprint or to find less
> Windows-specific approach, but I'd prefer a client-side fix anyway, as
> graceful closing a socket by a server seems a legitimate action.

What concerns me here is whether this implies that other clients
(libpq, jdbc, etc) are going to need changes as well.  Maybe
libpq is okay, because we've not seen failures of the isolation
tests that use pg_cancel_backend(), but still it's worrisome.
I'm not entirely sure whether the isolationtester would notice
that a connection that should have died didn't.

regards, tom lane

Re: null iv parameter passed to combo_init()

[Tom Lane]

Noah Misch  writes:
> On Sun, Jan 09, 2022 at 04:37:32AM -0800, Zhihong Yu wrote:
>> On Sat, Jan 8, 2022 at 11:32 PM Tom Lane  wrote:
>>> FWIW, I liked the "ivlen > 0" formulation better.  They should be
>>> equivalent, because ivlen is unsigned, but it just seems like "> 0"
>>> is more natural.

> If I were considering the one code site in isolation, I'd pick "ivlen > 0".
> But of the four sites identified so far, three have signed length variables.

Oh, hmm.  Unless we want to start changing those to unsigned, I agree
a not-equal test is a safer convention.

regards, tom lane

Re: Non-superuser subscription owners

[Jeff Davis]

On Sat, 2022-01-08 at 12:57 -0500, Tom Lane wrote:
> ... btw, I'd like to complain that this new test script consumes
> a completely excessive amount of time.

Should be fixed now; I brought the number of tests down from 100 to 14.
It's not running in 2 seconds on my machine, but it's in line with the
other tests.

Regards,
Jeff Davis

Re: [PATCH] Prefer getenv("HOME") to find the UNIX home directory

[Tom Lane]

Anders Kaseorg  writes:
> On 10/20/21 04:55, Daniel Gustafsson wrote:
>> Is the proposed change portable across all linux/unix systems we support?
>> Reading aobut indicates that it's likely to be, but neither NetBSD nor 
>> FreeBSD
>> have the upthread referenced wording in their manpages.

> Since the proposed change falls back to the old behavior if HOME is 
> unset or empty, I assume this is a question about convention and not 
> literally about whether it will work on these systems. I don’t find it 
> surprising that this convention isn’t explicitly called out in every 
> system’s manpage for the wrong function, but it still applies to these 
> systems.

Given the POSIX requirements, it's basically impossible to believe
that there are interesting cases where $HOME isn't set.  Thus, it
seems to me that keeping the getpwuid calls will just mean carrying
untestable dead code, so we should simplify matters by ripping
those out and *only* consulting $HOME.

The v1 patch also neglects the matter of documentation.  I think
the simplest and most transparent thing to do is just to explicitly
mention $HOME everyplace we talk about files that are sought there,
in place of our current convention to write "~".  (I'm too lazy
to go digging in the git history, but I have a feeling that this is
undoing somebody's intentional change from a long time back.)

BTW, not directly impacted by this patch but adjacent to it,
I noted that on Windows psql's \cd defaults to changing to "/".
That seems a bit surprising, and we definitely fail to document it.
I settled for noting it in the documentation, but should we make
it do something else?

PFA v2 patch.

regards, tom lane

diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 14f35d37f6..faf36f051f 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -1214,7 +1214,7 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
   
Specifies the name of the file used to store passwords
(see ).
-   Defaults to ~/.pgpass, or
+   Defaults to $HOME/.pgpass, or
%APPDATA%\postgresql\pgpass.conf on Microsoft Windows.
(No error is reported if this file does not exist.)
   
@@ -1670,7 +1670,7 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname

 This parameter specifies the file name of the client SSL
 certificate, replacing the default
-~/.postgresql/postgresql.crt.
+$HOME/.postgresql/postgresql.crt.
 This parameter is ignored if an SSL connection is not made.

   
@@ -1683,7 +1683,7 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
 This parameter specifies the location for the secret key used for
 the client certificate. It can either specify a file name that will
 be used instead of the default
-~/.postgresql/postgresql.key, or it can specify a key
+$HOME/.postgresql/postgresql.key, or it can specify a key
 obtained from an external engine (engines are
 OpenSSL loadable modules).  An external engine
 specification should consist of a colon-separated engine name and
@@ -1733,7 +1733,7 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
 certificate authority (CA) certificate(s).
 If the file exists, the server's certificate will be verified
 to be signed by one of these authorities.  The default is
-~/.postgresql/root.crt.
+$HOME/.postgresql/root.crt.

   
  
@@ -1749,7 +1749,7 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
  nor
  is set, this setting is
 taken as
-~/.postgresql/root.crl.
+$HOME/.postgresql/root.crl.

   
  
@@ -7776,7 +7776,7 @@ myEventProc(PGEventId evtId, void *evtInfo, void *passThrough)
   
   PGSERVICEFILE specifies the name of the per-user
   connection service file.  If not set, it defaults
-  to ~/.pg_service.conf
+  to $HOME/.pg_service.conf
   (see ).
  
 
@@ -8151,7 +8151,7 @@ myEventProc(PGEventId evtId, void *evtInfo, void *passThrough)
system-wide file.  If the same service name exists in both the user
and the system file, the user file takes precedence.
By default, the per-user service file is located
-   at ~/.pg_service.conf; this can be overridden by
+   at $HOME/.pg_service.conf; this can be overridden by
setting the environment variable PGSERVICEFILE.
The system-wide file is named pg_service.conf.
By default it is sought in the etc directory
@@ -8354,8 +8354,8 @@ ldap://ldap.acme.com/cn=dbserver,cn=hosts?pgconnectinfo?base?(objectclass=*)
 
   
To allow server certificate verification, one or more root certificates
-   must be placed in the file ~/.postgresql/root.crt
-   in the user's home directory.  (On Microsoft Windows the file is named
+   must be placed in the file $HOME/.postgresql/root.crt.
+   (On Microsoft Windows the file is named
%APPDATA%\postgresql\r

Re: Why is src/test/modules/committs/t/002_standby.pl flaky?

[Thomas Munro]

On Mon, Jan 10, 2022 at 12:00 AM Alexander Lakhin  wrote:
> Going down through the call chain, I see that at the end of it
> WaitForMultipleObjects() hangs while waiting for the primary connection
> socket event. So it looks like the socket, that is closed by the
> primary, can get into a state unsuitable for WaitForMultipleObjects().

I wonder if FD_CLOSE is edge-triggered, and it's already told us once.
I think that's what these Python Twisted guys are saying:

https://stackoverflow.com/questions/7598936/how-can-a-disconnected-tcp-socket-be-reliably-detected-using-msgwaitformultipleo

> I tried to check the socket state with the WSAPoll() function and
> discovered that it returns POLLHUP for the "problematic" socket.

Good discovery.  I guess if the above theory is right, there's a
memory somewhere that makes this level-triggered as expected by users
of poll().

Re: Non-superuser subscription owners

[Tom Lane]

Jeff Davis  writes:
> On Sat, 2022-01-08 at 12:57 -0500, Tom Lane wrote:
>> ... btw, I'd like to complain that this new test script consumes
>> a completely excessive amount of time.

> Should be fixed now; I brought the number of tests down from 100 to 14.
> It's not running in 2 seconds on my machine, but it's in line with the
> other tests.

Thanks, I appreciate that.

regards, tom lane

Re: Adding CI to our tree

[Justin Pryzby]

I noticed a patch failing in cfbot everywhere except windows:

https://commitfest.postgresql.org/36/3476/
| Invalid relcache when ADD PRIMARY KEY USING INDEX

It's because vcregress skips tests which have NO_INSTALLCHECK=1.

Is it desirable to enable more module/contrib tests for windows CI ?

This does a few, but there's a few others which would require the server to
be restarted to set shared_preload_libraries for each module.

diff --git a/.cirrus.yml b/.cirrus.yml
index 19b3737fa11..c427b468334 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -390,7 +390,7 @@ task:
 - perl src/tools/msvc/vcregress.pl check parallel
   startcreate_script:
 # paths to binaries need backslashes
-- tmp_install\bin\pg_ctl.exe initdb -D tmp_check/db -l tmp_check/initdb.log
+- tmp_install\bin\pg_ctl.exe initdb -D tmp_check/db -l 
tmp_check/initdb.log --options=--no-sync
 - echo include '%TEMP_CONFIG%' >> tmp_check/db/postgresql.conf
 - tmp_install\bin\pg_ctl.exe start -D tmp_check/db -l 
tmp_check/postmaster.log
   test_pl_script:
diff --git a/contrib/test_decoding/Makefile b/contrib/test_decoding/Makefile
index 9a31e0b8795..14fd847ba7f 100644
--- a/contrib/test_decoding/Makefile
+++ b/contrib/test_decoding/Makefile
@@ -10,7 +10,7 @@ ISOLATION = mxact delayed_startup ondisk_startup 
concurrent_ddl_dml \
oldest_xmin snapshot_transfer subxact_without_top concurrent_stream \
twophase_snapshot
 
-REGRESS_OPTS = --temp-config $(top_srcdir)/contrib/test_decoding/logical.conf
+REGRESS_OPTS = --temp-config=$(top_srcdir)/contrib/test_decoding/logical.conf
 ISOLATION_OPTS = --temp-config $(top_srcdir)/contrib/test_decoding/logical.conf
 
 # Disabled because these tests require "wal_level=logical", which
diff --git a/src/tools/ci/pg_ci_base.conf b/src/tools/ci/pg_ci_base.conf
index d8faa9c26c1..52cdb697a57 100644
--- a/src/tools/ci/pg_ci_base.conf
+++ b/src/tools/ci/pg_ci_base.conf
@@ -12,3 +12,24 @@ log_connections = true
 log_disconnections = true
 log_line_prefix = '%m [%p][%b] %q[%a][%v:%x] '
 log_lock_waits = true
+
+# test_decoding
+wal_level = logical
+max_replication_slots = 4
+logical_decoding_work_mem = 64kB
+
+# commit_ts
+track_commit_timestamp = on
+
+## worker_spi
+#shared_preload_libraries = worker_spi
+#worker_spi.database = contrib_regression
+
+## pg_stat_statements
+##shared_preload_libraries=pg_stat_statements
+
+## test_rls_hooks
+#shared_preload_libraries=test_rls_hooks
+
+## snapshot_too_old
+#old_snapshot_threshold = 60min
diff --git a/src/tools/msvc/vcregress.pl b/src/tools/msvc/vcregress.pl
index 8f3e3fa937b..7e2cc971a42 100644
--- a/src/tools/msvc/vcregress.pl
+++ b/src/tools/msvc/vcregress.pl
@@ -443,6 +443,7 @@ sub plcheck
 sub subdircheck
 {
my $module = shift;
+   my $obey_installcheck = shift || 1;
 
if (   !-d "$module/sql"
|| !-d "$module/expected"
@@ -452,7 +453,7 @@ sub subdircheck
}
 
chdir $module;
-   my @tests = fetchTests();
+   my @tests = fetchTests($obey_installcheck);
 
# Leave if no tests are listed in the module.
if (scalar @tests == 0)
@@ -516,6 +517,14 @@ sub contribcheck
my $status = $? >> 8;
$mstat ||= $status;
}
+
+   subdircheck('test_decoding', -1);
+   $mstat ||= $? >> 8;
+
+   # The DB would need to be restarted
+   #subdircheck('pg_stat_statements', -1);
+   #$mstat ||= $? >> 8;
+
exit $mstat if $mstat;
return;
 }
@@ -530,6 +539,19 @@ sub modulescheck
my $status = $? >> 8;
$mstat ||= $status;
}
+
+   subdircheck('commit_ts', -1);
+   $mstat ||= $? >> 8;
+
+   subdircheck('test_rls_hooks', -1);
+   $mstat ||= $? >> 8;
+
+   ## The DB would need to be restarted
+   #subdircheck('worker_spi', -1);
+   #$mstat ||= $? >> 8;
+
+   # src/test/modules/snapshot_too_old/Makefile
+
exit $mstat if $mstat;
return;
 }
@@ -726,6 +748,7 @@ sub fetchTests
my $m = <$handle>;
close($handle);
my $t = "";
+   my $obey_installcheck = shift || 1;
 
$m =~ s{\\\r?\n}{}g;
 
@@ -733,7 +756,7 @@ sub fetchTests
# so bypass its run by returning an empty set of tests.
if ($m =~ /^\s*NO_INSTALLCHECK\s*=\s*\S+/m)
{
-   return ();
+   return () if $obey_installcheck == 1;
}
 
if ($m =~ /^REGRESS\s*=\s*(.*)$/gm)

Re: Adding CI to our tree

[Andres Freund]

Hi,

On 2022-01-09 13:16:50 -0600, Justin Pryzby wrote:
> I noticed a patch failing in cfbot everywhere except windows:
> 
> https://commitfest.postgresql.org/36/3476/
> | Invalid relcache when ADD PRIMARY KEY USING INDEX
> 
> It's because vcregress skips tests which have NO_INSTALLCHECK=1.

> Is it desirable to enable more module/contrib tests for windows CI ?

Yes. I think the way we run windows tests is pretty bad - it's not reasonable
that each developer needs to figure out 20 magic incantations to run all tests
on windows.


> This does a few, but there's a few others which would require the server to
> be restarted to set shared_preload_libraries for each module.
> 
> diff --git a/.cirrus.yml b/.cirrus.yml
> index 19b3737fa11..c427b468334 100644
> --- a/.cirrus.yml
> +++ b/.cirrus.yml
> @@ -390,7 +390,7 @@ task:
>  - perl src/tools/msvc/vcregress.pl check parallel
>startcreate_script:
>  # paths to binaries need backslashes
> -- tmp_install\bin\pg_ctl.exe initdb -D tmp_check/db -l 
> tmp_check/initdb.log
> +- tmp_install\bin\pg_ctl.exe initdb -D tmp_check/db -l 
> tmp_check/initdb.log --options=--no-sync
>  - echo include '%TEMP_CONFIG%' >> tmp_check/db/postgresql.conf
>  - tmp_install\bin\pg_ctl.exe start -D tmp_check/db -l 
> tmp_check/postmaster.log
>test_pl_script:

> diff --git a/contrib/test_decoding/Makefile b/contrib/test_decoding/Makefile
> index 9a31e0b8795..14fd847ba7f 100644
> --- a/contrib/test_decoding/Makefile
> +++ b/contrib/test_decoding/Makefile
> @@ -10,7 +10,7 @@ ISOLATION = mxact delayed_startup ondisk_startup 
> concurrent_ddl_dml \
>   oldest_xmin snapshot_transfer subxact_without_top concurrent_stream \
>   twophase_snapshot
>  
> -REGRESS_OPTS = --temp-config $(top_srcdir)/contrib/test_decoding/logical.conf
> +REGRESS_OPTS = --temp-config=$(top_srcdir)/contrib/test_decoding/logical.conf
>  ISOLATION_OPTS = --temp-config 
> $(top_srcdir)/contrib/test_decoding/logical.conf
>

Not sure why these are part of the diff?


> diff --git a/src/tools/ci/pg_ci_base.conf b/src/tools/ci/pg_ci_base.conf
> index d8faa9c26c1..52cdb697a57 100644
> --- a/src/tools/ci/pg_ci_base.conf
> +++ b/src/tools/ci/pg_ci_base.conf
> @@ -12,3 +12,24 @@ log_connections = true
>  log_disconnections = true
>  log_line_prefix = '%m [%p][%b] %q[%a][%v:%x] '
>  log_lock_waits = true
> +
> +# test_decoding
> +wal_level = logical
> +max_replication_slots = 4
> +logical_decoding_work_mem = 64kB
> [ more ]

This doesn't really seem like a scalable path forward - duplicating
configuration in more places doesn't seem sane. It seems it'd make more sense
to teach vcregress.pl to run NO_INSTALLCHECK targets properly? ISTM that
changing the options passed to pg_regress based on fetchTests() return value
wouldn't be too hard?

Greetings,

Andres Freund

Re: warn if GUC set to an invalid shared library

[Maciek Sakrejda]

On Sat, Jan 8, 2022 at 2:07 PM Justin Pryzby  wrote:
> Unfortunately, the output for dlopen() is not portable, which (I think) means
> most of what I wrote can't be made to work..  Since it doesn't work to call
> dlopen() when using SET, I tried using just stat().  But that also fails on
> windows, since one of the regression tests has an invalid filename involving
> unbalanced quotes, which cause it to return EINVAL rather than ENOENT.  So SET
> cannot warn portably, unless it includes no details at all (or we specially
> handle the windows case), or change the pre-existing regression test.  But
> there's a 2nd instability, too, apparently having to do with timing.  So I'm
> planning to drop the 0001 patch.

Hmm. I think 001 is a big part of the usability improvement here.
Could we not at least warn generically, without relaying the
underlying error? The notable thing in this situation is that the
specified library could not be loaded (and that it will almost
certainly cause problems on restart). The specific error would be nice
to have, but it's less important. What is the timing instability?

> > On Tue, Dec 28, 2021 at 9:45 AM Justin Pryzby  wrote:
> > For whatever reason, I get slightly different (and somewhat redundant)
> > output on failing to start:
> >
> > 2022-01-08 12:59:36.784 PST [324482] WARNING:  could not load library: 
> > $libdir/plugins/totally bogus: cannot open shared object file: No such file 
> > or directory
> > 2022-01-08 12:59:36.787 PST [324482] FATAL:  could not load library: 
> > totally bogus: cannot open shared object file: No such file or directory
> > 2022-01-08 12:59:36.787 PST [324482] LOG:  database system is shut down
>
> I think the first WARNING is from the GUC mechanism "setting" the library.
> And then the FATAL is from trying to apply the GUC.
> It looks like you didn't apply the 0002 patch for that test so got no CONTEXT 
> ?

I still had the terminal open where I tested this, and the scrollback
did show me applying the patch (and building after). I tried a make
clean and applying the patch again, and I do see the CONTEXT line now.
I'm not sure what the problem was but seems like PEBKAC--sorry about
that.

Thanks,
Maciek

Re: null iv parameter passed to combo_init()

[Zhihong Yu]

On Sun, Jan 9, 2022 at 8:48 AM Noah Misch  wrote:

> On Sun, Jan 09, 2022 at 04:37:32AM -0800, Zhihong Yu wrote:
> > On Sat, Jan 8, 2022 at 11:32 PM Tom Lane  wrote:
> > > Noah Misch  writes:
> > > > On further thought, I would write it this way:
> > >
> > > > - else
> > > > + else if (ivlen != 0)
> > > >   memcpy(ivbuf, iv, ivlen);
> > >
> > > FWIW, I liked the "ivlen > 0" formulation better.  They should be
> > > equivalent, because ivlen is unsigned, but it just seems like "> 0"
> > > is more natural.
>
> If I were considering the one code site in isolation, I'd pick "ivlen > 0".
> But of the four sites identified so far, three have signed length
> variables.
> Since we're likely to get more examples of this pattern, some signed and
> some
> unsigned, I'd rather use a style that does the optimal thing whether or not
> the variable is signed.  What do you think?
>
> > Patch v4 is attached.
>
> Does this pass the test procedure shown upthread?
>
Hi,
I installed gcc 4.9.3

When I ran:
./configure CFLAGS='-fsanitize=undefined
-fsanitize-undefined-trap-on-error'

I saw:

configure:3977: $? = 0
configure:3966: gcc -V >&5
gcc: error: unrecognized command line option '-V'
gcc: fatal error: no input files
compilation terminated.
configure:3977: $? = 1
configure:3966: gcc -qversion >&5
gcc: error: unrecognized command line option '-qversion'
gcc: fatal error: no input files
compilation terminated.
configure:3977: $? = 1
configure:3997: checking whether the C compiler works
configure:4019: gcc -fsanitize=undefined -fsanitize-undefined-trap-on-error
  conftest.c  >&5
gcc: error: unrecognized command line option
'-fsanitize-undefined-trap-on-error'
configure:4023: $? = 1
configure:4061: result: no

I wonder if a higher version gcc is needed.

FYI

Re: [PATCH] Prefer getenv("HOME") to find the UNIX home directory

[Anders Kaseorg]

On 1/9/22 10:59, Tom Lane wrote:

Given the POSIX requirements, it's basically impossible to believe
that there are interesting cases where $HOME isn't set.  Thus, it
seems to me that keeping the getpwuid calls will just mean carrying
untestable dead code, so we should simplify matters by ripping
those out and *only* consulting $HOME.


While POSIX requires that the login program put you in a conforming 
environment, nothing stops the user from building a non-conforming 
environment, such as with ‘env -i’.  One could argue that such a user 
deserves whatever broken behavior they might get.  But to me it seems 
prudent to continue working there if it worked before.



The v1 patch also neglects the matter of documentation.  I think
the simplest and most transparent thing to do is just to explicitly
mention $HOME everyplace we talk about files that are sought there,
in place of our current convention to write "~".  (I'm too lazy
to go digging in the git history, but I have a feeling that this is
undoing somebody's intentional change from a long time back.)


The reason I didn’t change the documentation is that this is already 
what “~” is supposed to mean according to POSIX and common 
implementations.  See previous discussion:


https://www.postgresql.org/message-id/163425265.90107%40mit.edu
https://www.postgresql.org/message-id/d452fd57-8c34-0a94-79c1-4498eb4ffbdc%40mit.edu

I consider my patch a bug fix that implements the behavior one would 
already expect from the existing documentation.


Therefore, I still prefer my v1 patch on both counts.  I am willing to 
be overruled if you still disagree, but I wanted to explain my reasoning.


Anders

Re: [PATCH] Prefer getenv("HOME") to find the UNIX home directory

[Tom Lane]

Anders Kaseorg  writes:
> On 1/9/22 10:59, Tom Lane wrote:
>> Given the POSIX requirements, it's basically impossible to believe
>> that there are interesting cases where $HOME isn't set.  Thus, it
>> seems to me that keeping the getpwuid calls will just mean carrying
>> untestable dead code, so we should simplify matters by ripping
>> those out and *only* consulting $HOME.

> While POSIX requires that the login program put you in a conforming 
> environment, nothing stops the user from building a non-conforming 
> environment, such as with ‘env -i’.  One could argue that such a user 
> deserves whatever broken behavior they might get.  But to me it seems 
> prudent to continue working there if it worked before.

The only case that the v1 patch helps such a user for is if they
unset HOME or set it precisely to ''.  If they set it to anything
else, it's still broken from their perspective.  So I do not find
that that argument holds water.

Moreover, ISTM that the only plausible use-case for unsetting HOME
is to prevent programs from finding stuff in your home directory.
What would be the point otherwise?  So it's pretty hard to envision
a case where somebody is actually using, and happy with, the
behavior you argue we ought to keep.

>> The v1 patch also neglects the matter of documentation.

> The reason I didn’t change the documentation is that this is already 
> what “~” is supposed to mean according to POSIX and common 
> implementations.

The point here is precisely that we're changing what *we* think ~
means.  I don't think we can just leave the docs unchanged.

regards, tom lane

Re: [PATCH] Prefer getenv("HOME") to find the UNIX home directory

[Anders Kaseorg]

On 1/9/22 13:04, Tom Lane wrote:

The only case that the v1 patch helps such a user for is if they
unset HOME or set it precisely to ''.  If they set it to anything
else, it's still broken from their perspective.  So I do not find
that that argument holds water.

Moreover, ISTM that the only plausible use-case for unsetting HOME
is to prevent programs from finding stuff in your home directory.
What would be the point otherwise?  So it's pretty hard to envision
a case where somebody is actually using, and happy with, the
behavior you argue we ought to keep.


Obviously a user who intentionally breaks their environment should 
expect problems.  But what I’m saying is that a user could have written 
a script that unsets HOME by *accident* while intending to clear *other* 
things out of the environment.  They might have developed it by starting 
with an empty environment and adding back the minimal set of variables 
they needed to get something to work.  Since most programs (including 
most libcs and shells) do in fact fall back to getpwuid when HOME is 
unset, they may not have noticed an unset HOME as a problem.  Unsetting 
HOME does not, in practice, prevent most programs from finding stuff in 
your home directory.


Anders

Re: null iv parameter passed to combo_init()

[Zhihong Yu]

On Sun, Jan 9, 2022 at 12:38 PM Zhihong Yu  wrote:

>
>
> On Sun, Jan 9, 2022 at 8:48 AM Noah Misch  wrote:
>
>> On Sun, Jan 09, 2022 at 04:37:32AM -0800, Zhihong Yu wrote:
>> > On Sat, Jan 8, 2022 at 11:32 PM Tom Lane  wrote:
>> > > Noah Misch  writes:
>> > > > On further thought, I would write it this way:
>> > >
>> > > > - else
>> > > > + else if (ivlen != 0)
>> > > >   memcpy(ivbuf, iv, ivlen);
>> > >
>> > > FWIW, I liked the "ivlen > 0" formulation better.  They should be
>> > > equivalent, because ivlen is unsigned, but it just seems like "> 0"
>> > > is more natural.
>>
>> If I were considering the one code site in isolation, I'd pick "ivlen >
>> 0".
>> But of the four sites identified so far, three have signed length
>> variables.
>> Since we're likely to get more examples of this pattern, some signed and
>> some
>> unsigned, I'd rather use a style that does the optimal thing whether or
>> not
>> the variable is signed.  What do you think?
>>
>> > Patch v4 is attached.
>>
>> Does this pass the test procedure shown upthread?
>>
> Hi,
> I installed gcc 4.9.3
>
> When I ran:
> ./configure CFLAGS='-fsanitize=undefined
> -fsanitize-undefined-trap-on-error'
>
> I saw:
>
> configure:3977: $? = 0
> configure:3966: gcc -V >&5
> gcc: error: unrecognized command line option '-V'
> gcc: fatal error: no input files
> compilation terminated.
> configure:3977: $? = 1
> configure:3966: gcc -qversion >&5
> gcc: error: unrecognized command line option '-qversion'
> gcc: fatal error: no input files
> compilation terminated.
> configure:3977: $? = 1
> configure:3997: checking whether the C compiler works
> configure:4019: gcc -fsanitize=undefined
> -fsanitize-undefined-trap-on-error   conftest.c  >&5
> gcc: error: unrecognized command line option
> '-fsanitize-undefined-trap-on-error'
> configure:4023: $? = 1
> configure:4061: result: no
>
> I wonder if a higher version gcc is needed.
>
> FYI
>

After installing gcc-11, ./configure passed (with 0003-memcpy-null.patch).
In the output of `make check-world`, I don't see `runtime error`.
Though there was a crash (maybe specific to my machine):

Core was generated by
`/nfusr/dev-server/zyu/postgres/tmp_install/usr/local/pgsql/bin/postgres
--singl'.
Program terminated with signal SIGILL, Illegal instruction.
#0  0x0050642d in write_item.cold ()
Missing separate debuginfos, use: debuginfo-install
glibc-2.17-325.el7_9.x86_64 nss-pam-ldapd-0.8.13-25.el7.x86_64
sssd-client-1.16.5-10.el7_9.10.x86_64
(gdb) bt
#0  0x0050642d in write_item.cold ()
#1  0x00ba9d1b in write_relcache_init_file ()
#2  0x00bb58f7 in RelationCacheInitializePhase3 ()
#3  0x00bd5cb5 in InitPostgres ()
#4  0x00a0a9ea in PostgresMain ()

FYI

Re: [PATCH] Prefer getenv("HOME") to find the UNIX home directory

[Justin Pryzby]

On Sun, Jan 09, 2022 at 01:59:02PM -0500, Tom Lane wrote:
> Given the POSIX requirements, it's basically impossible to believe
> that there are interesting cases where $HOME isn't set.

I've run into this before - children of init may not have HOME set.

It's easy enough to add it if it's needed, but should probably be called out in
the release notes.

-- 
Justin

Re: [PATCH] Prefer getenv("HOME") to find the UNIX home directory

[Christoph Moench-Tegeder]

## Tom Lane (t...@sss.pgh.pa.us):

> Given the POSIX requirements, it's basically impossible to believe
> that there are interesting cases where $HOME isn't set.

When I look at a random Debian with the usual PGDG packages, the
postmaster process (and every backend) has a rather minimal environment
without HOME. When I remember the code correctly, walreceiver uses
the functions from fe-connect.c and may need to find the service file,
a password file or certificates. If I'm correct with that, requiring
HOME to be set would be a significant change for existing "normal"
installations.
What about containers and similar "reduced" environments?

Regards,
Christoph

-- 
Spare Space

Re: [PATCH] Prefer getenv("HOME") to find the UNIX home directory

[Tom Lane]

Christoph Moench-Tegeder  writes:
> ## Tom Lane (t...@sss.pgh.pa.us):
>> Given the POSIX requirements, it's basically impossible to believe
>> that there are interesting cases where $HOME isn't set.

> When I look at a random Debian with the usual PGDG packages, the
> postmaster process (and every backend) has a rather minimal environment
> without HOME. When I remember the code correctly, walreceiver uses
> the functions from fe-connect.c and may need to find the service file,
> a password file or certificates. If I'm correct with that, requiring
> HOME to be set would be a significant change for existing "normal"
> installations.
> What about containers and similar "reduced" environments?

Isn't that a flat out violation of POSIX 8.3 Other Environment Variables?

HOME
The system shall initialize this variable at the time of login to
be a pathname of the user's home directory. See .

To claim it's not, you have to claim these programs aren't logged in,
in which case where did they get any privileges from?

regards, tom lane

Re: [PATCH] Prefer getenv("HOME") to find the UNIX home directory

[Christoph Moench-Tegeder]

## Tom Lane (t...@sss.pgh.pa.us):

> Isn't that a flat out violation of POSIX 8.3 Other Environment Variables?
> 
> HOME
> The system shall initialize this variable at the time of login to
> be a pathname of the user's home directory. See .
> 
> To claim it's not, you have to claim these programs aren't logged in,
> in which case where did they get any privileges from?

After poking around across some Linuxes, it looks like people silently
agreed that "services" are not logged-in users: among the daemons,
having HOME set (as observed in /proc/*/environ) is an exception,
not the norm. I'm not sure if that's a "new" thing with systemd,
I don't have a linux with pure SysV-init available (but I guess those
are rare animals anyways).

Regards,
Christoph

-- 
Spare Space

Re: [PATCH] Prefer getenv("HOME") to find the UNIX home directory

[Tom Lane]

Christoph Moench-Tegeder  writes:
> ## Tom Lane (t...@sss.pgh.pa.us):
>> Isn't that a flat out violation of POSIX 8.3 Other Environment Variables?

> After poking around across some Linuxes, it looks like people silently
> agreed that "services" are not logged-in users: among the daemons,
> having HOME set (as observed in /proc/*/environ) is an exception,
> not the norm.

Meh.  I guess there's not much point in arguing with facts on the
ground.  Anders' proposed behavior seems like the way to go then,
at least so far as libpq is concerned.  (I remain skeptical that
psql would be run in such an environment, but there's no value
in making it act different from libpq.)

regards, tom lane

Re: Windows crash / abort handling

[Andres Freund]

Hi,

On 2021-10-06 14:11:51 +0800, Craig Ringer wrote:
> On Wed, 6 Oct 2021, 03:30 Andres Freund,  wrote:
>
> > Hi,
> >
> >
> > My first attempt was to try to use the existing crashdump stuff in
> > pgwin32_install_crashdump_handler(). That's not really quite what I want,
> > because it only handles postmaster rather than any binary, but I thought
> > it'd
> > be a good start. But outside of toy situations it didn't work for me.
> >
>
> Odd. It usually has for me, and definitely not limited to the postmaster.
> But it will fall down for OOM, smashed stack, and other states where
> in-process self-debugging is likely to fail.

I think it's a question of running debug vs optimized builds. At least in
debug builds it doesn't appear to work because the debug c runtime abort
preempts it.


> I patch this out when working on windows because it's a real pain.
>
> I keep meaning to propose that we remove this functionality entirely. It's
> obsolete. It was introduced back in the days where DrWatson.exe "windows
> error reporting") used to launch an interactive prompt asking the user what
> to do when a process crashed. This would block the crashed process from
> exiting, making everything grind to a halt until the user interacted with
> the
> UI. Even for a service process.

> Not fun on a headless or remote server.

Yea, the way we do it right now is definitely not helpful. Especially because
it doesn't actually prevent the "hang" issue - the CRT boxes at least cause
precisely such stalls.

We've had a few CI hangs due to such errors.


> These days Windows handles all this a lot more sensibly, and blocking crash
> reporting is quite obsolete and unhelpful.

>From what I've seen it didn't actually get particularly sensible, just
different and more complicated.

>From what I've seen one needs at least:
- _set_abort_behavior(_CALL_REPORTFAULT | _WRITE_ABORT_MSG)
- _set_error_mode(_OUT_TO_STDERR)
- _CrtSetReportMode(_CRT_ASSERT/ERROR, _CRTDBG_MODE_FILE | _CRTDBG_MODE_DEBUG)
- SetErrorMode(SEM_FAILCRITICALERRORS)

There's many things this hocuspocus can be called, but sensible isn't among my
word choices for it.


> I'd like to just remove it.

I think we need to remove the SEM_NOGPFAULTERRORBOX, but I don't think we can
remove the SEM_FAILCRITICALERRORS, and I think we need the rest of the above
to prevent error boxes from happening.


I think we ought to actually apply these to all binaries, not just
postgres. One CI hung was due to psql asserting. But there's currently no easy
hook point for all binaries afaict. If we were to introduce something it
should probably be in pgport? But I'd defer to that to a later step.


I've attached a patch implementing these changes.

I also made one run with that intentionally fail (with an Assert(false)), and
with the changes the debugger is invoked and creates a backtrace etc:
https://cirrus-ci.com/task/5447300246929408
(click on crashlog-> at the top)

Greetings,

Andres Freund
>From 4979447881836c86f0bf24a164d3ca920323f9eb Mon Sep 17 00:00:00 2001
From: Andres Freund 
Date: Thu, 9 Sep 2021 17:49:39 -0700
Subject: [PATCH v6 1/2] windows: Improve crash / assert / exception handling.

---
 src/backend/main/main.c | 48 +++--
 .cirrus.yml |  6 +-
 2 files changed, 51 insertions(+), 3 deletions(-)

diff --git a/src/backend/main/main.c b/src/backend/main/main.c
index 9124060bde7..111e7867cc7 100644
--- a/src/backend/main/main.c
+++ b/src/backend/main/main.c
@@ -22,6 +22,10 @@
 
 #include 
 
+#if defined(WIN32)
+#include 
+#endif
+
 #if defined(__NetBSD__)
 #include 
 #endif
@@ -237,8 +241,48 @@ startup_hacks(const char *progname)
 			exit(1);
 		}
 
-		/* In case of general protection fault, don't show GUI popup box */
-		SetErrorMode(SEM_FAILCRITICALERRORS | SEM_NOGPFAULTERRORBOX);
+		/*
+		 * By default abort() only generates a crash-dump in *non* debug
+		 * builds. As our Assert() / ExceptionalCondition() uses abort(),
+		 * leaving the default in place would make debugging harder.
+		 */
+#if !defined(__MINGW32__) && !defined(__MINGW64__)
+		_set_abort_behavior(_CALL_REPORTFAULT | _WRITE_ABORT_MSG,
+			_CALL_REPORTFAULT | _WRITE_ABORT_MSG);
+#endif /* MINGW */
+
+		/*
+		 * SEM_FAILCRITICALERRORS causes more errors to be reported to
+		 * callers.
+		 *
+		 * We used to also specify SEM_NOGPFAULTERRORBOX, but that prevents
+		 * windows crash reporting from working. Which includes registered
+		 * just-in-time debuggers. Now we try to disable sources of popups
+		 * separately below (note that SEM_NOGPFAULTERRORBOX didn't actually
+		 * prevent all sources of such popups).
+		 */
+		SetErrorMode(SEM_FAILCRITICALERRORS);
+
+		/*
+		 * Show errors on stderr instead of popup box (note this doesn't
+		 * affect errors originating in the C runtime, see below).
+		 */
+		_set_error_mode(_OUT_TO_STDERR);
+
+		/*
+		 * In DEBUG builds, errors, including assertions, C runtime errors are
+		 * reported via _CrtDbgReport. By default 

Re: Adding CI to our tree

[Andres Freund]

Hi,

On 2021-10-02 12:59:09 -0700, Andres Freund wrote:
> On 2021-10-02 11:05:20 -0400, Tom Lane wrote:
> > I don't know enough about Windows to evaluate 0001, but I'm a little
> > worried about it because it looks like it's changing our *production*
> > error handling on that platform.
> 
> Yea. It's clearly not ready as-is - it's the piece that I was planning to
> write a separate email about.

> 
> It's hard to understand what *precisely* SEM_NOGPFAULTERRORBOX etc do.
> 
> What I do know is that without the _set_abort_behavior() stuff abort() doesn't
> trigger windows' "crash" paths in at least debugging builds, and that the
> SetErrorMode() and _CrtSetReportMode() changes are necessary to get segfaults
> to reach the crash paths.
> 
> The in-tree behaviour turns out to make debugging on windows a major pain, at
> least when compiling with msvc. Crashes never trigger core dumps or "just in
> time" debugging (their term for invoking a debugger upon crash), so one has to
> attach to processes before they crash, to have any chance of debugging.
> 
> As far as I can tell this also means that at least for debugging builds,
> pgwin32_install_crashdump_handler() is pretty much dead weight -
> crashDumpHandler() never gets invoked. I think it may get invoked for abort()s
> in production builds, but probably not for segfaults.
> 
> And despite SEM_NOGPFAULTERRORBOX we display those annoying "popup" boxes
> telling us about the crash and giving the option to retry, ignore, something
> something.   It's all a bit baffling.

FWIW, the latest version of this patch (including an explanation why
SEM_NOGPFAULTERRORBOX isn't useful for our purposes [anymore]) is at (and
above)
https://postgr.es/m/20220110005704.es4el6i2nxlxzwof%40alap3.anarazel.de

Greetings,

Andres Freund

Re: Is it worth adding ReplicationSlot active_pid to ReplicationSlotPersistentData?

[Bharath Rupireddy]

On Wed, Jan 5, 2022 at 12:13 PM Kyotaro Horiguchi 
wrote:
> > Here's the v2 patch. Please provide your thoughts.
>
> Thanks!  I have three comments on this version.

Thanks for your review.

> - I still think "acquire/release" logs on creation/dropping should be
>   silenced.  Adding the third parameter to ReplicationSlotAcquire()
>   that can mute the acquiring (and as well as the corresponding
>   releasing) log will work.

Done.

> can piggy-back on log_replication_commands for the purpose, changing
> its meaning slightly to "log replication commands and related
> activity".
> - Need to mute the logs by log_replication_commands.  (We could add
>   another choice for the variable for this purpose but I think we
>   don't need it.)

Done.

> - The messages are not translatable as the variable parts are
>   adjectives. They need to consist of static sentences.  The
>   combinations of the two properties are 6 (note that persistence is
>   tristate) but I don't think we accept that complexity for the
>   information.  So I recommend to just remove the variable parts from
>   the messages.

Done.

Here's v3, please review it further.

Regards,
Bharath Rupireddy.
From 46792e7b33c15afeda83769655e2512674c33269 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy 
Date: Sun, 9 Jan 2022 09:38:36 +
Subject: [PATCH v3] Add LOG messages when replication slots become active and
 inactive

These logs will be extremely useful on production servers to debug
and analyze inactive replication slot issues.
---
 doc/src/sgml/config.sgml  |  6 ++--
 .../replication/logical/logicalfuncs.c|  4 +--
 src/backend/replication/slot.c| 30 ---
 src/backend/replication/slotfuncs.c   | 10 +++
 src/backend/replication/walsender.c   | 12 
 src/backend/storage/lmgr/proc.c   |  2 +-
 src/backend/tcop/postgres.c   |  2 +-
 src/backend/utils/misc/guc.c  |  2 +-
 src/include/replication/slot.h|  4 +--
 9 files changed, 47 insertions(+), 25 deletions(-)

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index afbb6c35e3..5f95fdffc8 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -7284,9 +7284,9 @@ log_line_prefix = '%m [%p] %q%u@%d/%a '
   
   

-Causes each replication command to be logged in the server log.
-See  for more information about
-replication command. The default value is off.
+Causes each replication command and related activity to be logged in
+the server log. See  for more
+information about replication command. The default value is off.
 Only superusers can change this setting.

   
diff --git a/src/backend/replication/logical/logicalfuncs.c b/src/backend/replication/logical/logicalfuncs.c
index 4f633888b4..ef19e48719 100644
--- a/src/backend/replication/logical/logicalfuncs.c
+++ b/src/backend/replication/logical/logicalfuncs.c
@@ -215,7 +215,7 @@ pg_logical_slot_get_changes_guts(FunctionCallInfo fcinfo, bool confirm, bool bin
 	else
 		end_of_wal = GetXLogReplayRecPtr(NULL);
 
-	ReplicationSlotAcquire(NameStr(*name), true);
+	ReplicationSlotAcquire(NameStr(*name), true, true);
 
 	PG_TRY();
 	{
@@ -331,7 +331,7 @@ pg_logical_slot_get_changes_guts(FunctionCallInfo fcinfo, bool confirm, bool bin
 		/* free context, call shutdown callback */
 		FreeDecodingContext(ctx);
 
-		ReplicationSlotRelease();
+		ReplicationSlotRelease(true);
 		InvalidateSystemCaches();
 	}
 	PG_CATCH();
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index e5e0cf8768..fbb02bb036 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -337,6 +337,11 @@ ReplicationSlotCreate(const char *name, bool db_specific,
 
 	/* Let everybody know we've modified this slot */
 	ConditionVariableBroadcast(&slot->active_cv);
+
+	ereport(log_replication_commands ? LOG : DEBUG1,
+			(errmsg("created replication slot \"%s\"",
+	NameStr(slot->data.name)),
+			 errhidestmt(true)));
 }
 
 /*
@@ -377,7 +382,7 @@ SearchNamedReplicationSlot(const char *name, bool need_lock)
  * nowait is false, we sleep until the slot is released by the owning process.
  */
 void
-ReplicationSlotAcquire(const char *name, bool nowait)
+ReplicationSlotAcquire(const char *name, bool nowait, bool msg_ok)
 {
 	ReplicationSlot *s;
 	int			active_pid;
@@ -457,6 +462,12 @@ retry:
 
 	/* We made this slot active, so it's ours now. */
 	MyReplicationSlot = s;
+
+	if (msg_ok)
+		ereport(log_replication_commands ? LOG : DEBUG1,
+(errmsg("acquired replication slot \"%s\"",
+		NameStr(s->data.name)),
+ errhidestmt(true)));
 }
 
 /*
@@ -466,7 +477,7 @@ retry:
  * Resources this slot requires will be preserved.
  */
 void
-ReplicationSlotRelease(void)
+ReplicationSlotRelease(bool msg_ok)
 {
 	ReplicationSlot *slot = MyReplicationSlot;
 
@@ -516,6 +527,12 @@ ReplicationSlotRe

Re: Why is src/test/modules/committs/t/002_standby.pl flaky?

[Thomas Munro]

On Mon, Jan 10, 2022 at 8:06 AM Thomas Munro  wrote:
> On Mon, Jan 10, 2022 at 12:00 AM Alexander Lakhin  wrote:
> > Going down through the call chain, I see that at the end of it
> > WaitForMultipleObjects() hangs while waiting for the primary connection
> > socket event. So it looks like the socket, that is closed by the
> > primary, can get into a state unsuitable for WaitForMultipleObjects().
>
> I wonder if FD_CLOSE is edge-triggered, and it's already told us once.

Can you reproduce it with this patch?
From d3cfff53911ce068d4a31fe7ac7933958936d1b0 Mon Sep 17 00:00:00 2001
From: Thomas Munro 
Date: Mon, 10 Jan 2022 14:45:05 +1300
Subject: [PATCH] Make Windows FD_CLOSE reporting sticky.

XXX Just testing an idea...
---
 src/backend/storage/ipc/latch.c | 16 
 src/include/storage/latch.h |  1 +
 2 files changed, 17 insertions(+)

diff --git a/src/backend/storage/ipc/latch.c b/src/backend/storage/ipc/latch.c
index 1d893cf863..4a61b31cd5 100644
--- a/src/backend/storage/ipc/latch.c
+++ b/src/backend/storage/ipc/latch.c
@@ -899,6 +899,7 @@ AddWaitEventToSet(WaitEventSet *set, uint32 events, pgsocket fd, Latch *latch,
 	event->user_data = user_data;
 #ifdef WIN32
 	event->reset = false;
+	event->closed = false;
 #endif
 
 	if (events == WL_LATCH_SET)
@@ -1882,6 +1883,20 @@ WaitEventSetWaitBlock(WaitEventSet *set, int cur_timeout,
 return 1;
 			}
 		}
+
+		/*
+		 * XXX: HYPOTHESIS TO TEST
+		 * Windows only reports FD_CLOSE once.  If we've seen that already,
+		 * continue to report it.
+		 */
+		if ((cur_event & WL_SOCKET_MASK) && cur_event->closed)
+		{
+			occurred_events->pos = cur_event->pos;
+			occurred_events->user_data = cur_event->user_data;
+			occurred_events->events = (cur_event->events & WL_SOCKET_MASK);
+			occurred_events->fd = cur_event->fd;
+			return 1;
+		}
 	}
 
 	/*
@@ -2002,6 +2017,7 @@ WaitEventSetWaitBlock(WaitEventSet *set, int cur_timeout,
 		{
 			/* EOF/error, so signal all caller-requested socket flags */
 			occurred_events->events |= (cur_event->events & WL_SOCKET_MASK);
+			cur_event->closed = true;
 		}
 
 		if (occurred_events->events != 0)
diff --git a/src/include/storage/latch.h b/src/include/storage/latch.h
index 44f9368c64..c24f46dc37 100644
--- a/src/include/storage/latch.h
+++ b/src/include/storage/latch.h
@@ -147,6 +147,7 @@ typedef struct WaitEvent
 	void	   *user_data;		/* pointer provided in AddWaitEventToSet */
 #ifdef WIN32
 	bool		reset;			/* Is reset of the event required? */
+	bool		closed;			/* Has FD_CLOSE event been reported? */
 #endif
 } WaitEvent;
 
-- 
2.33.1

Re: [PATCH] Prefer getenv("HOME") to find the UNIX home directory

[Tom Lane]

I wrote:
> Meh.  I guess there's not much point in arguing with facts on the
> ground.  Anders' proposed behavior seems like the way to go then,
> at least so far as libpq is concerned.

So I pushed that, but while working on it I grew quite annoyed
at the messy API exhibited by src/port/thread.c, particularly
at how declaring its functions in port.h requires #including
 and  there.  That means those headers are
read by every compile in our tree, though only a tiny number
of modules actually need either.  So here are a couple of
follow-on patches to improve that situation.

For pqGethostbyname, there is no consumer other than
src/port/getaddrinfo.c, which makes it even sillier because that
file isn't even compiled on a large majority of platforms, making
pqGethostbyname dead code that we nonetheless build everywhere.
So 0001 attached just moves that function into getaddrinfo.c.

For pqGetpwuid, the best solution seemed to be to add a
less platform-dependent API layer, which I did in 0002
attached.  Perhaps someone would object to the API details
I chose, but by and large this seems like an improvement
that reduces the amount of code duplication in the callers.

Thoughts?

regards, tom lane

diff --git a/src/include/port.h b/src/include/port.h
index 22ea292a6d..df81fa97c6 100644
--- a/src/include/port.h
+++ b/src/include/port.h
@@ -14,7 +14,6 @@
 #define PG_PORT_H
 
 #include 
-#include 
 #include 
 
 /*
@@ -485,12 +484,6 @@ extern int	pqGetpwuid(uid_t uid, struct passwd *resultbuf, char *buffer,
 	   size_t buflen, struct passwd **result);
 #endif
 
-extern int	pqGethostbyname(const char *name,
-			struct hostent *resultbuf,
-			char *buffer, size_t buflen,
-			struct hostent **result,
-			int *herrno);
-
 extern void pg_qsort(void *base, size_t nel, size_t elsize,
 	 int (*cmp) (const void *, const void *));
 extern int	pg_qsort_strcmp(const void *a, const void *b);
diff --git a/src/port/Makefile b/src/port/Makefile
index b3754d8940..bfe1feb0d4 100644
--- a/src/port/Makefile
+++ b/src/port/Makefile
@@ -84,6 +84,10 @@ libpgport.a: $(OBJS)
 	rm -f $@
 	$(AR) $(AROPT) $@ $^
 
+# getaddrinfo.o and getaddrinfo_shlib.o need PTHREAD_CFLAGS (but getaddrinfo_srv.o does not)
+getaddrinfo.o: CFLAGS+=$(PTHREAD_CFLAGS)
+getaddrinfo_shlib.o: CFLAGS+=$(PTHREAD_CFLAGS)
+
 # thread.o and thread_shlib.o need PTHREAD_CFLAGS (but thread_srv.o does not)
 thread.o: CFLAGS+=$(PTHREAD_CFLAGS)
 thread_shlib.o: CFLAGS+=$(PTHREAD_CFLAGS)
diff --git a/src/port/getaddrinfo.c b/src/port/getaddrinfo.c
index b0ca4c778e..3284c6eb52 100644
--- a/src/port/getaddrinfo.c
+++ b/src/port/getaddrinfo.c
@@ -34,6 +34,14 @@
 #include "port/pg_bswap.h"
 
 
+#ifdef FRONTEND
+static int	pqGethostbyname(const char *name,
+			struct hostent *resultbuf,
+			char *buffer, size_t buflen,
+			struct hostent **result,
+			int *herrno);
+#endif
+
 #ifdef WIN32
 /*
  * The native routines may or may not exist on the Windows platform we are on,
@@ -394,3 +402,39 @@ getnameinfo(const struct sockaddr *sa, int salen,
 
 	return 0;
 }
+
+/*
+ * Wrapper around gethostbyname() or gethostbyname_r() to mimic
+ * POSIX gethostbyname_r() behaviour, if it is not available or required.
+ */
+#ifdef FRONTEND
+static int
+pqGethostbyname(const char *name,
+struct hostent *resultbuf,
+char *buffer, size_t buflen,
+struct hostent **result,
+int *herrno)
+{
+#if defined(ENABLE_THREAD_SAFETY) && defined(HAVE_GETHOSTBYNAME_R)
+
+	/*
+	 * broken (well early POSIX draft) gethostbyname_r() which returns 'struct
+	 * hostent *'
+	 */
+	*result = gethostbyname_r(name, resultbuf, buffer, buflen, herrno);
+	return (*result == NULL) ? -1 : 0;
+#else
+
+	/* no gethostbyname_r(), just use gethostbyname() */
+	*result = gethostbyname(name);
+
+	if (*result != NULL)
+		*herrno = h_errno;
+
+	if (*result != NULL)
+		return 0;
+	else
+		return -1;
+#endif
+}
+#endif			/* FRONTEND */
diff --git a/src/port/thread.c b/src/port/thread.c
index c1040d4e24..a4c1672c89 100644
--- a/src/port/thread.c
+++ b/src/port/thread.c
@@ -76,41 +76,3 @@ pqGetpwuid(uid_t uid, struct passwd *resultbuf, char *buffer,
 #endif
 }
 #endif
-
-/*
- * Wrapper around gethostbyname() or gethostbyname_r() to mimic
- * POSIX gethostbyname_r() behaviour, if it is not available or required.
- * This function is called _only_ by our getaddrinfo() portability function.
- */
-#ifndef HAVE_GETADDRINFO
-int
-pqGethostbyname(const char *name,
-struct hostent *resultbuf,
-char *buffer, size_t buflen,
-struct hostent **result,
-int *herrno)
-{
-#if defined(FRONTEND) && defined(ENABLE_THREAD_SAFETY) && defined(HAVE_GETHOSTBYNAME_R)
-
-	/*
-	 * broken (well early POSIX draft) gethostbyname_r() which returns 'struct
-	 * hostent *'
-	 */
-	*result = gethostbyname_r(name, resultbuf, buffer, buflen, herrno);
-	return (*result == NULL) ? -1 : 0;
-#else
-
-	/* no gethostbyname_r(), just use gethostbyname() */
-	*result = gethostbyname(name);
-

Re: null iv parameter passed to combo_init()

[Zhihong Yu]

On Sun, Jan 9, 2022 at 1:27 PM Zhihong Yu  wrote:

>
>
> On Sun, Jan 9, 2022 at 12:38 PM Zhihong Yu  wrote:
>
>>
>>
>> On Sun, Jan 9, 2022 at 8:48 AM Noah Misch  wrote:
>>
>>> On Sun, Jan 09, 2022 at 04:37:32AM -0800, Zhihong Yu wrote:
>>> > On Sat, Jan 8, 2022 at 11:32 PM Tom Lane  wrote:
>>> > > Noah Misch  writes:
>>> > > > On further thought, I would write it this way:
>>> > >
>>> > > > - else
>>> > > > + else if (ivlen != 0)
>>> > > >   memcpy(ivbuf, iv, ivlen);
>>> > >
>>> > > FWIW, I liked the "ivlen > 0" formulation better.  They should be
>>> > > equivalent, because ivlen is unsigned, but it just seems like "> 0"
>>> > > is more natural.
>>>
>>> If I were considering the one code site in isolation, I'd pick "ivlen >
>>> 0".
>>> But of the four sites identified so far, three have signed length
>>> variables.
>>> Since we're likely to get more examples of this pattern, some signed and
>>> some
>>> unsigned, I'd rather use a style that does the optimal thing whether or
>>> not
>>> the variable is signed.  What do you think?
>>>
>>> > Patch v4 is attached.
>>>
>>> Does this pass the test procedure shown upthread?
>>>
>> Hi,
>> I installed gcc 4.9.3
>>
>> When I ran:
>> ./configure CFLAGS='-fsanitize=undefined
>> -fsanitize-undefined-trap-on-error'
>>
>> I saw:
>>
>> configure:3977: $? = 0
>> configure:3966: gcc -V >&5
>> gcc: error: unrecognized command line option '-V'
>> gcc: fatal error: no input files
>> compilation terminated.
>> configure:3977: $? = 1
>> configure:3966: gcc -qversion >&5
>> gcc: error: unrecognized command line option '-qversion'
>> gcc: fatal error: no input files
>> compilation terminated.
>> configure:3977: $? = 1
>> configure:3997: checking whether the C compiler works
>> configure:4019: gcc -fsanitize=undefined
>> -fsanitize-undefined-trap-on-error   conftest.c  >&5
>> gcc: error: unrecognized command line option
>> '-fsanitize-undefined-trap-on-error'
>> configure:4023: $? = 1
>> configure:4061: result: no
>>
>> I wonder if a higher version gcc is needed.
>>
>> FYI
>>
>
> After installing gcc-11, ./configure passed (with 0003-memcpy-null.patch).
> In the output of `make check-world`, I don't see `runtime error`.
> Though there was a crash (maybe specific to my machine):
>
> Core was generated by
> `/nfusr/dev-server/zyu/postgres/tmp_install/usr/local/pgsql/bin/postgres
> --singl'.
> Program terminated with signal SIGILL, Illegal instruction.
> #0  0x0050642d in write_item.cold ()
> Missing separate debuginfos, use: debuginfo-install
> glibc-2.17-325.el7_9.x86_64 nss-pam-ldapd-0.8.13-25.el7.x86_64
> sssd-client-1.16.5-10.el7_9.10.x86_64
> (gdb) bt
> #0  0x0050642d in write_item.cold ()
> #1  0x00ba9d1b in write_relcache_init_file ()
> #2  0x00bb58f7 in RelationCacheInitializePhase3 ()
> #3  0x00bd5cb5 in InitPostgres ()
> #4  0x00a0a9ea in PostgresMain ()
>
> FYI
>
Hi,
Earlier I was using devtoolset-11 which had an `Illegal instruction` error.

I compiled / installed gcc-11 from source (which took whole afternoon).
`make check-world` passed with patch v3.
In tmp_install/log/install.log, I saw:

gcc -Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Werror=vla -Wendif-labels
-Wmissing-format-attribute -Wimplicit-fallthrough=3 -Wcast-function-type
-Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard
-Wno-format-truncation -Wno-stringop-truncation -fsanitize=undefined
-fsanitize-undefined-trap-on-error -I../../src/port -DFRONTEND
-I../../src/include  -D_GNU_SOURCE   -c -o path.o path.c
rm -f libpgport.a

Cheers

Re: pg_dump/restore --no-tableam

[Andres Freund]

Hi,

On 2022-01-03 15:44:24 -0600, Justin Pryzby wrote:
> @cfbot: rebased

> From 69ae2ed5d00a97d351e1f6c45a9e406f33032898 Mon Sep 17 00:00:00 2001
> From: Justin Pryzby 
> Date: Sun, 7 Mar 2021 19:35:37 -0600
> Subject: [PATCH] Add pg_dump/restore --no-table-am..
> 
> This was for some reason omitted from 3b925e905.

Seems the docs changes aren't quite right?

https://cirrus-ci.com/task/5864769860141056?logs=docs_build#L344

[02:43:01.356] ref/pg_dump.sgml:1162: parser error : Opening and ending tag 
mismatch: varlistentry line 934 and variablelist
[02:43:01.356] 
[02:43:01.356]^


> + 
> + 

Yup...

Greetings,

Andres Freund

Re: TAP test to cover "EndOfLogTLI != replayTLI" case

[Andres Freund]

Hi,

On 2021-11-23 11:43:21 +0530, Amul Sul wrote:
> Attached patch covers a case where TLI in the filename for a
> record being read is different from where it belongs to. In other
> words, it covers following case noted in StartupXLOG():

> Thoughts? Suggestions?

It seems the test isn't quite reliable. It occasionally fails on freebsd,
macos, linux and always on windows (starting with the new CI stuff, before the
test wasn't run).

See https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest/36/3427

Greetings,

Andres Freund

RE: row filtering for logical replication

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

On Friday, January 7, 2022 3:40 PM Peter Smith  wrote:
> Below are some review comments for the v60 patches.
> 
> 1e.
> "Subsequent UPDATE or DELETE statements have no effect."
> 
> Why won't they have an effect? The first impression is the newly updated tuple
> now matches the filter, I think this part seems to need some more detailed
> explanation. I saw there are some slightly different details in the header
> comment of the pgoutput_row_filter_update_check function - does it help?

Thanks for the comments ! I have addressed all the comments except 1e which
I will think over it and update in next version.

Best regards,
Hou zj

Re: ICU for global collation

[Julien Rouhaud]

On Fri, Jan 07, 2022 at 03:25:28PM +0100, Peter Eisentraut wrote:
> 
> I tested this a bit.  I used the following setup:
> 
> create table t1 (a text);
> insert into t1 select md5(generate_series(1, 1000)::text);
> select count(*) from t1 where a > '';
> 
> And then I changed in varstr_cmp():
> 
> if (collid != DEFAULT_COLLATION_OID)
> mylocale = pg_newlocale_from_collation(collid);
> 
> to just
> 
> mylocale = pg_newlocale_from_collation(collid);
> 
> I find that the \timing results are indistinguishable.  (I used locale
> "en_US.UTF-8" and made sure that that code path is actually hit.)
> 
> Does anyone have other insights?

Looking at the git history, you added this comment in 414c5a2ea65.

After a bit a digging in the lists, I found that you introduced it to fix a
reported 13% slowdown in varstr_cmp():
https://www.postgresql.org/message-id/20110129075253.GA18784%40tornado.leadboat.com
https://www.postgresql.org/message-id/1296748408.6442.1.camel%40vanquo.pezone.net

RE: row filtering for logical replication

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

On Wednesday, January 5, 2022 2:01 PM vignesh C  wrote:
> On Tue, Jan 4, 2022 at 9:58 AM Peter Smith  wrote:
> >
> > Here is the v58* patch set:
> >
> > Main changes from v57* are
> > 1. Couple of review comments fixed
> >
> > ~~
> >
> > Review comments (details)
> > =
> >
> > v58-0001 (main)
> > - PG docs updated as suggested [Alvaro, Euler 26/12]
> >
> > v58-0002 (new/old tuple)
> > - pgputput_row_filter_init refactored as suggested [Wangw 30/12] #3
> > - re-ran pgindent
> >
> > v58-0003 (tab, dump)
> > - no change
> >
> > v58-0004 (refactor transformations)
> > - minor changes to commit message
> 
> Few comments:

Thanks for the comments!

> 1) We could include namespace names along with the relation to make it more
> clear to the user if the user had specified tables having same table names 
> from
> different schemas:

Since most of the error message in publicationcmd.c and pg_publication.c 
doesn't include include namespace names along with the relation,
I am not sure is it necessary to add this. So, I didn’t change this in the 
patch.

> 5) This log will be logged for each tuple, if there are millions of records 
> it will
> get logged millions of times, we could remove it:
> +   /* update requires a new tuple */
> +   Assert(newtuple);
> +
> +   elog(DEBUG3, "table \"%s.%s\" has row filter",
> +
> get_namespace_name(get_rel_namespace(RelationGetRelid(relation))),
> +get_rel_name(relation->rd_id));

Since the message is logged only in DEBUG3 and could be useful for some
debugging purpose, so I didn't remove this in the new version patch.

Best regards,
Hou zj

Re: Multiple Query IDs for a rewritten parse tree

[Andrey V. Lepikhov]

On 1/9/22 5:13 PM, Julien Rouhaud wrote:

For now the queryid mixes two different things: fingerprinting and query text
normalization.  Should each calculation method be allowed to do a different
normalization too, and if yes where should be stored the state data needed for
that?  If not, we would need some kind of primary hash for that purpose.


Do You mean JumbleState?
I think, registering queryId generator we should store also a pointer 
(void **args) to an additional data entry, as usual.



Looking at Andrey's use case for wanting multiple hashes, I don't think that
adaptive optimization needs a normalized query string.  The only use would be
to output some statistics, but this could be achieved by storing a list of
"primary queryid" for each adaptive entry.  That's probably also true for
anything that's not monitoring intended.  Also, all monitoring consumers should
probably agree on the same queryid, both fingerprint and normalized string, as
otherwise it's impossible to cross-reference metric data.


I can add one more use case.
Our extension for freezing query plan uses query tree comparison 
technique to prove, that the plan can be applied (and we don't need to 
execute planning procedure at all).
The procedure of a tree equality checking is expensive and we use 
cheaper queryId comparison to identify possible candidates. So here, for 
the better performance and queries coverage, we need to use query tree 
normalization - queryId should be stable to some modifications in a 
query text which do not change semantics.
As an example, query plan with external parameters can be used to 
execute constant query if these constants correspond by place and type 
to the parameters. So, queryId calculation technique returns also 
pointers to all constants and parameters found during the calculation.


--
regards,
Andrey Lepikhov
Postgres Professional

Fix a possible typo in rewriteheap.c code comments

[Bharath Rupireddy]

Hi,

It looks like there's a typo, attaching a tiny patch to fix it.

  *
  * When doing logical decoding - which relies on using cmin/cmax of catalog
  * tuples, via xl_heap_new_cid records - heap rewrites have to log enough
- * information to allow the decoding backend to updates its internal mapping
+ * information to allow the decoding backend to update its internal mapping

Regards,
Bharath Rupireddy.
From 7954f128efbda9e95cf0cc2e38588430c9ab4359 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy 
Date: Mon, 10 Jan 2022 04:06:58 +
Subject: [PATCH v1] Fix a typo in rewriteheap.c code comments

---
 src/backend/access/heap/rewriteheap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/backend/access/heap/rewriteheap.c b/src/backend/access/heap/rewriteheap.c
index 5bb5ebba23..aa265edf60 100644
--- a/src/backend/access/heap/rewriteheap.c
+++ b/src/backend/access/heap/rewriteheap.c
@@ -741,7 +741,7 @@ raw_heap_insert(RewriteState state, HeapTuple tup)
  *
  * When doing logical decoding - which relies on using cmin/cmax of catalog
  * tuples, via xl_heap_new_cid records - heap rewrites have to log enough
- * information to allow the decoding backend to updates its internal mapping
+ * information to allow the decoding backend to update its internal mapping
  * of (relfilenode,ctid) => (cmin, cmax) to be correct for the rewritten heap.
  *
  * For that, every time we find a tuple that's been modified in a catalog
-- 
2.25.1

Re: TAP test to cover "EndOfLogTLI != replayTLI" case

[Amul Sul]

On Mon, Jan 10, 2022 at 8:25 AM Andres Freund  wrote:
>
> Hi,
>
> On 2021-11-23 11:43:21 +0530, Amul Sul wrote:
> > Attached patch covers a case where TLI in the filename for a
> > record being read is different from where it belongs to. In other
> > words, it covers following case noted in StartupXLOG():
>
> > Thoughts? Suggestions?
>
> It seems the test isn't quite reliable. It occasionally fails on freebsd,
> macos, linux and always on windows (starting with the new CI stuff, before the
> test wasn't run).
>
> See 
> https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest/36/3427
>

Thanks for the note, I can see the same test is failing on my centos
vm too with the latest master head(376ce3e404b).  The failing reason is
the "recovery_target_inclusive = off" setting which is unnecessary for
this test, the attached patch removing the same.

Regards,
Amul
From 88ae9ea5a33c1ecc5b5493dae9c016ef19fbf88f Mon Sep 17 00:00:00 2001
From: Amul Sul 
Date: Sun, 9 Jan 2022 23:10:07 -0500
Subject: [PATCH v2] TAP test for EndOfLogTLI

---
 src/test/recovery/t/003_recovery_targets.pl | 52 -
 1 file changed, 51 insertions(+), 1 deletion(-)

diff --git a/src/test/recovery/t/003_recovery_targets.pl b/src/test/recovery/t/003_recovery_targets.pl
index 24da78c0bcd..cf72b5d9343 100644
--- a/src/test/recovery/t/003_recovery_targets.pl
+++ b/src/test/recovery/t/003_recovery_targets.pl
@@ -6,7 +6,7 @@ use strict;
 use warnings;
 use PostgreSQL::Test::Cluster;
 use PostgreSQL::Test::Utils;
-use Test::More tests => 9;
+use Test::More tests => 10;
 use Time::HiRes qw(usleep);
 
 # Create and test a standby from given backup, with a certain recovery target.
@@ -182,3 +182,53 @@ $logfile = slurp_file($node_standby->logfile());
 ok( $logfile =~
 	  qr/FATAL: .* recovery ended before configured recovery target was reached/,
 	'recovery end before target reached is a fatal error');
+
+# Test to cover a case where that we are looking for WAL record that ought to be
+# in for e.g 00010001 we don't find it; instead we find
+# 00020003 because of various reasons such as there was a
+# timeline switch in that segment, and we were reading the old WAL from a
+# segment belonging to a higher timeline or our recovery target timeline is 2,
+# or something that has 2 in its history.
+
+# Insert few more data to primary
+$node_primary->safe_psql('postgres',
+	"INSERT INTO tab_int VALUES (generate_series(6001,7000))");
+my $lsn6 = $node_primary->safe_psql('postgres',
+	"SELECT pg_current_wal_lsn()");
+
+# Setup new standby and enable WAL archiving to archive WAL files at the same
+# location as the primary.
+my $archive_cmd = $node_primary->safe_psql('postgres',
+	"SELECT current_setting('archive_command')");
+$node_standby = PostgreSQL::Test::Cluster->new('standby_9');
+$node_standby->init_from_backup(
+	$node_primary, 'my_backup',
+	has_streaming => 1);
+$node_standby->append_conf(
+'postgresql.conf', qq(
+archive_mode = on
+archive_command = '$archive_cmd'
+));
+$node_standby->start;
+# Wait until necessary replay has been done on standby
+$node_primary->wait_for_catchup($node_standby, 'replay',
+	$node_primary->lsn('write'));
+$node_standby->promote;
+$node_standby->safe_psql('postgres',
+	"INSERT INTO tab_int VALUES (generate_series(7001,8000))");
+# Force archiving of WAL file
+$node_standby->safe_psql('postgres', "SELECT pg_switch_wal()");
+$node_standby->stop;
+
+# Another standby whose recovery target lsn will be in the WAL file has
+# a different TLI than the target LSN belongs to.
+$node_standby = PostgreSQL::Test::Cluster->new('standby_10');
+$node_standby->init_from_backup(
+	$node_primary, 'my_backup',
+	has_restoring => 1);
+$node_standby->append_conf(
+'postgresql.conf', qq(recovery_target_lsn = '$lsn6'));
+$node_standby->start;
+my $result = $node_standby->safe_psql('postgres',
+	"SELECT count(*) FROM tab_int");
+is($result, '7000', "check standby content before timeline switch $lsn6");
-- 
2.18.0

pg_upgrade verbosity when redirecting output to log file

[Andres Freund]

Hi,

I've been annoyed for quite a while that pg_upgrade tests lists all copied
files - it's ~1500 lines or so in the make -C src/bin/pg_ugprade check case
(obviously for a cluster with more objects it's basically unlimited). But
whenever I was looking into something about the issue, I didn't see the log
outpu, leading me to believe somebody else had fixed it (although I thought I
complained about this before, but I couldn't find it in the archives).

Turns out that it only happens when the output is not a tty. And I notice it
whenever I redirect the log output to a file, pipe, or such.


This actually might not be intended?


While util.c:pg_log_v() [1] says

/* PG_VERBOSE and PG_STATUS are only output in verbose mode */

it actually prints out the status report unconditionally:

...
case PG_STATUS:
if (isatty(fileno(stdout)))
printf("  %s%-*.*s\r",...);
...
else
printf("  %s\n", message);
break;

this isn't bad when stdout is a tty, because the \r will hide the repeated
output. But when its not, we just dump out the progress, regardless of
verbosity.

This code appears to have been this way for a long time and I'm not quite sure
what the intent really is.

It seems not unreasonable to log progress if a tty or if verbose is specified?

I guess there's an argument to be made that outputting all that data
unrequested isn't great in the tty case either. On a cluster with a large
schema that could be quite a few MB of output - enough to slow things down
over a low throughput / high latency link probably.  On a test cluster with
20k tables I had lying around, script -c "pg_upgrade " (which simulates a
tty) results in a ~4MB typescript.


A very minor thing is that we do the isatty() thing in every PG_STATUS logging
invocation. Each time that triggers a
ioctl(1, TCGETS, {B38400 opost isig icanon echo ...}) = 0
which isn't a huge cost compared to actually copying a file, but it's not
entirely free either.

Greetings,

Andres Freund


[1] called from
  transfer_all_new_tablespaces() ->
  parallel_transfer_all_new_dbs() ->
  transfer_all_new_dbs() ->
  transfer_single_new_db() ->
  transfer_relfile() ->
/* Copying files might take some time, so give feedback. */
pg_log(PG_STATUS, "status: %s", old_file);

Re: pg_upgrade verbosity when redirecting output to log file

[Andres Freund]

Hi,

On 2022-01-09 20:28:40 -0800, Andres Freund wrote:
> Turns out that it only happens when the output is not a tty. And I notice it
> whenever I redirect the log output to a file, pipe, or such.

Ah. More precisely, it happens when doing
  make -s -Otarget -j32 check-world,
but *not* when
  make -s -Otarget -j32 -C src/bin/pg_upgrade check

because when there's only one target the subprocess ends up with the
FDs make is invoked with, but when there's concurrency -Otarget causes
stdin/out to be temporary files. Leading to the endless output "Copying user
relation files" output when doing check-world...

Greetings,

Andres Freund

Re: Multiple Query IDs for a rewritten parse tree

[Julien Rouhaud]

On Mon, Jan 10, 2022 at 09:10:59AM +0500, Andrey V. Lepikhov wrote:
> On 1/9/22 5:13 PM, Julien Rouhaud wrote:
> > For now the queryid mixes two different things: fingerprinting and query 
> > text
> > normalization.  Should each calculation method be allowed to do a different
> > normalization too, and if yes where should be stored the state data needed 
> > for
> > that?  If not, we would need some kind of primary hash for that purpose.
> > 
> Do You mean JumbleState?

Yes, or some other abstraction.  For instance with the proposed patch to handle
differently the IN clauses, you needs a to change JumbleState.

> I think, registering queryId generator we should store also a pointer (void
> **args) to an additional data entry, as usual.

Well, yes but only if you need to produce different versions of the normalized
query text, and I'm not convinced that's really necessary.

> > Looking at Andrey's use case for wanting multiple hashes, I don't think that
> > adaptive optimization needs a normalized query string.  The only use would 
> > be
> > to output some statistics, but this could be achieved by storing a list of
> > "primary queryid" for each adaptive entry.  That's probably also true for
> > anything that's not monitoring intended.  Also, all monitoring consumers 
> > should
> > probably agree on the same queryid, both fingerprint and normalized string, 
> > as
> > otherwise it's impossible to cross-reference metric data.
> > 
> I can add one more use case.
> Our extension for freezing query plan uses query tree comparison technique
> to prove, that the plan can be applied (and we don't need to execute
> planning procedure at all).
> The procedure of a tree equality checking is expensive and we use cheaper
> queryId comparison to identify possible candidates. So here, for the better
> performance and queries coverage, we need to use query tree normalization -
> queryId should be stable to some modifications in a query text which do not
> change semantics.
> As an example, query plan with external parameters can be used to execute
> constant query if these constants correspond by place and type to the
> parameters. So, queryId calculation technique returns also pointers to all
> constants and parameters found during the calculation.

I'm also working on a similar extension, and yes you can't accept any
fingerprinting approach for that.  I don't know what are the exact heuristics
of your cheaper queryid calculation are, but is it reasonable to use it with
something like pg_stat_statements?  If yes, you don't really need two queryid
approach for the sake of this single extension and therefore don't need to
store multiple jumble state or similar per statement.  Especially since
requiring another one would mean a performance drop as soon as you want to use
something as common as pg_stat_statements.

Re: Fix a possible typo in rewriteheap.c code comments

[Amit Kapila]

On Mon, Jan 10, 2022 at 9:42 AM Bharath Rupireddy
 wrote:
>
> Hi,
>
> It looks like there's a typo, attaching a tiny patch to fix it.
>
>   *
>   * When doing logical decoding - which relies on using cmin/cmax of catalog
>   * tuples, via xl_heap_new_cid records - heap rewrites have to log enough
> - * information to allow the decoding backend to updates its internal mapping
> + * information to allow the decoding backend to update its internal mapping
>

LGTM.

-- 
With Regards,
Amit Kapila.

Re: Is it correct to update db state in control file as "shutting down" during end-of-recovery checkpoint?

[Jaime Casanova]

On Thu, Dec 09, 2021 at 07:41:52AM +0530, Bharath Rupireddy wrote:
> On Wed, Dec 8, 2021 at 11:02 PM Bossart, Nathan  wrote:
> >
> > On 12/8/21, 3:29 AM, "Bharath Rupireddy" 
> >  wrote:
> > > Thanks for your thoughts. I'm fine either way, hence attaching two
> > > patches here with and I will leave it for the committer 's choice.
> > > 1) v1-0001-Add-DB_IN_END_OF_RECOVERY_CHECKPOINT-state-for-co.patch --
> > > adds new db state DB_IN_END_OF_RECOVERY_CHECKPOINT for control file.
> > > 2) v1-0001-Skip-control-file-db-state-updation-during-end-of.patch --
> > > just skips setting db state to DB_SHUTDOWNING and DB_SHUTDOWNED in
> > > case of end-of-recovery checkpoint so that the state will be
> > > DB_IN_CRASH_RECOVERY which then changes to DB_IN_PRODUCTION.
> >
> > I've bumped this one to ready-for-committer.  For the record, my
> > preference is the second patch (for the reasons discussed upthread).
> > Both patches might benefit from a small comment or two, too.
> 
> Thanks. I've added a comment to the patch
> v2-0001-Skip-control-file-db-state-updation-during-end-of.patch. The
> other patch remains the same as the new state
> DB_IN_END_OF_RECOVERY_CHECKPOINT introduced there says it all.
> 

AFAIU is one patch or the other but not both, isn't it?

This habit of putting two conflicting versions of patches on the same 
thread causes http://cfbot.cputube.org/ to fail.

Now; I do think that the secondd patch, the one that just skips update
of the state in control file, is the way to go. The other patch adds too
much complexity for a small return.

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

Re: Is it correct to update db state in control file as "shutting down" during end-of-recovery checkpoint?

[Bharath Rupireddy]

On Mon, Jan 10, 2022 at 10:58 AM Jaime Casanova
 wrote:
>
> On Thu, Dec 09, 2021 at 07:41:52AM +0530, Bharath Rupireddy wrote:
> > On Wed, Dec 8, 2021 at 11:02 PM Bossart, Nathan  wrote:
> > >
> > > On 12/8/21, 3:29 AM, "Bharath Rupireddy" 
> > >  wrote:
> > > > Thanks for your thoughts. I'm fine either way, hence attaching two
> > > > patches here with and I will leave it for the committer 's choice.
> > > > 1) v1-0001-Add-DB_IN_END_OF_RECOVERY_CHECKPOINT-state-for-co.patch --
> > > > adds new db state DB_IN_END_OF_RECOVERY_CHECKPOINT for control file.
> > > > 2) v1-0001-Skip-control-file-db-state-updation-during-end-of.patch --
> > > > just skips setting db state to DB_SHUTDOWNING and DB_SHUTDOWNED in
> > > > case of end-of-recovery checkpoint so that the state will be
> > > > DB_IN_CRASH_RECOVERY which then changes to DB_IN_PRODUCTION.
> > >
> > > I've bumped this one to ready-for-committer.  For the record, my
> > > preference is the second patch (for the reasons discussed upthread).
> > > Both patches might benefit from a small comment or two, too.
> >
> > Thanks. I've added a comment to the patch
> > v2-0001-Skip-control-file-db-state-updation-during-end-of.patch. The
> > other patch remains the same as the new state
> > DB_IN_END_OF_RECOVERY_CHECKPOINT introduced there says it all.

> Now; I do think that the secondd patch, the one that just skips update
> of the state in control file, is the way to go. The other patch adds too
> much complexity for a small return.

Thanks. Attaching the above patch.

Regards,
Bharath Rupireddy.


v2-0001-Skip-control-file-db-state-updation-during-end-of.patch
Description: Binary data

Re: ICU for global collation

[Julien Rouhaud]

On Mon, Jan 10, 2022 at 11:25:08AM +0800, Julien Rouhaud wrote:
> On Fri, Jan 07, 2022 at 03:25:28PM +0100, Peter Eisentraut wrote:
> > 
> > I tested this a bit.  I used the following setup:
> > 
> > create table t1 (a text);
> > insert into t1 select md5(generate_series(1, 1000)::text);
> > select count(*) from t1 where a > '';
> > 
> > And then I changed in varstr_cmp():
> > 
> > if (collid != DEFAULT_COLLATION_OID)
> > mylocale = pg_newlocale_from_collation(collid);
> > 
> > to just
> > 
> > mylocale = pg_newlocale_from_collation(collid);
> > 
> > I find that the \timing results are indistinguishable.  (I used locale
> > "en_US.UTF-8" and made sure that that code path is actually hit.)
> > 
> > Does anyone have other insights?
> 
> Looking at the git history, you added this comment in 414c5a2ea65.
> 
> After a bit a digging in the lists, I found that you introduced it to fix a
> reported 13% slowdown in varstr_cmp():
> https://www.postgresql.org/message-id/20110129075253.GA18784%40tornado.leadboat.com
> https://www.postgresql.org/message-id/1296748408.6442.1.camel%40vanquo.pezone.net

So I tried to run Noah's benchmark to see if I could reproduce the slowdown.
Unfortunately the results I'm getting don't really make sense as removing the
optimisation brings a 15% speedup, and with a few more runs I can see that I
have about 25% noise, so there isn't much I can do to help.

Re: pg_upgrade verbosity when redirecting output to log file

[Tom Lane]

Andres Freund  writes:
> On 2022-01-09 20:28:40 -0800, Andres Freund wrote:
>> Turns out that it only happens when the output is not a tty. And I notice it
>> whenever I redirect the log output to a file, pipe, or such.

> Ah. More precisely, it happens when doing
>   make -s -Otarget -j32 check-world,
> but *not* when
>   make -s -Otarget -j32 -C src/bin/pg_upgrade check

Fun!  That seems to me to be a strong argument for not letting
the behavior vary depending on isatty().

I think I'd vote for just nuking that output altogether.
It seems of very dubious value.

regards, tom lane

Re: row filtering for logical replication

[Amit Kapila]

On Mon, Jan 10, 2022 at 8:41 AM houzj.f...@fujitsu.com
 wrote:
>
> Attach the v61 patch set.
>

Few comments:
==
1.
pgoutput_row_filter()
{
..
+
+ oldctx = MemoryContextSwitchTo(CacheMemoryContext);
+ rfnode = n_filters > 1 ? makeBoolExpr(OR_EXPR, rfnodes[idx], -1) :
linitial(rfnodes[idx]);
+ entry->exprstate[idx] = pgoutput_row_filter_init_expr(rfnode);
+ MemoryContextSwitchTo(oldctx);
..
}

rel_sync_cache_relation_cb()
{
..
+ if (entry->exprstate[idx] != NULL)
+ {
+ pfree(entry->exprstate[idx]);
+ entry->exprstate[idx] = NULL;
+ }
..
}

I think this can leak memory as just freeing 'exprstate' is not
sufficient. It contains other allocated memory as well like for
'steps'. Apart from that we might allocate other memory as well for
generating expression state. I think it would be better if we can have
another memory context (say cache_expr_cxt) in RelationSyncEntry and
allocate it the first time we need it and then reset it instead of
doing pfree of 'exprstate'. Also, we can free this new context in
pgoutput_shutdown before destroying RelationSyncCache.

2. If we do the above, we can use this new context at all other places
in the patch where it is using CacheMemoryContext.

3.
@@ -1365,6 +1785,7 @@ rel_sync_cache_publication_cb(Datum arg, int
cacheid, uint32 hashvalue)
 {
  HASH_SEQ_STATUS status;
  RelationSyncEntry *entry;
+ MemoryContext oldctx;

  /*
  * We can get here if the plugin was used in SQL interface as the
@@ -1374,6 +1795,8 @@ rel_sync_cache_publication_cb(Datum arg, int
cacheid, uint32 hashvalue)
  if (RelationSyncCache == NULL)
  return;

+ oldctx = MemoryContextSwitchTo(CacheMemoryContext);
+
  /*
  * There is no way to find which entry in our cache the hash belongs to so
  * mark the whole cache as invalid.
@@ -1392,6 +1815,8 @@ rel_sync_cache_publication_cb(Datum arg, int
cacheid, uint32 hashvalue)
  entry->pubactions.pubdelete = false;
  entry->pubactions.pubtruncate = false;
  }
+
+ MemoryContextSwitchTo(oldctx);
 }

Is there a reason for the above change?

4.
+#define SET_NO_FILTER_FOR_CURRENT_PUBACTIONS \
+ if (pub->pubactions.pubinsert) \
+ no_filter[idx_ins] = true; \
+ if (pub->pubactions.pubupdate) \
+ no_filter[idx_upd] = true; \
+ if (pub->pubactions.pubdelete) \
+ no_filter[idx_del] = true

I don't see the need for this macro and it makes code less readable. I
think we can instead move this code to a function to avoid duplicate
code.

5.
Multiple publications might have multiple row filters for
+ * this relation. Since row filter usage depends on the DML operation,
+ * there are multiple lists (one for each operation) which row filters
+ * will be appended.

There seems to be a typo in the above sentence.
/which row filters/to which row filters

6.
+ /*
+ * Find if there are any row filters for this relation. If there are,
+ * then prepare the necessary ExprState and cache it in
+ * entry->exprstate.
+ *
+ * NOTE: All publication-table mappings must be checked.
+ *
+ * NOTE: If the relation is a partition and pubviaroot is true, use
+ * the row filter of the topmost partitioned table instead of the row
+ * filter of its own partition.
+ *
+ * NOTE: Multiple publications might have multiple row filters for
+ * this relation. Since row filter usage depends on the DML operation,
+ * there are multiple lists (one for each operation) which row filters
+ * will be appended.
+ *
+ * NOTE: FOR ALL TABLES implies "don't use row filter expression" so
+ * it takes precedence.
+ *
+ * NOTE: ALL TABLES IN SCHEMA implies "don't use row filter
+ * expression" if the schema is the same as the table schema.
+ */
+ foreach(lc, data->publications)

Let's not add NOTE for each of these points but instead expand the
first sentence as "Find if there are any row filters for this
relation. If there are, then prepare the necessary ExprState and cache
it in entry->exprstate. To build an expression state, we need to
ensure the following:"

-- 
With Regards,
Amit Kapila.

Re: pg_upgrade verbosity when redirecting output to log file

[Andres Freund]

Hi,

On 2022-01-10 01:14:32 -0500, Tom Lane wrote:
> Andres Freund  writes:
> Fun!  That seems to me to be a strong argument for not letting
> the behavior vary depending on isatty().

Yea.


> I think I'd vote for just nuking that output altogether.
> It seems of very dubious value.

It seems worthwhile in some form - on large cluster in copy mode, the "Copying
user relation files" step can take *quite* a while, and even link/clone mode
aren't fast. But perhaps what'd be really needed is something counting up
actual progress in percentage of files and/or space...

I think just coupling it to verbose mode makes the most sense, for now?

Greetings,

Andres Freund

Re: Why is src/test/modules/committs/t/002_standby.pl flaky?

[Alexander Lakhin]

10.01.2022 05:00, Thomas Munro wrote:
> On Mon, Jan 10, 2022 at 8:06 AM Thomas Munro  wrote:
>> On Mon, Jan 10, 2022 at 12:00 AM Alexander Lakhin  
>> wrote:
>>> Going down through the call chain, I see that at the end of it
>>> WaitForMultipleObjects() hangs while waiting for the primary connection
>>> socket event. So it looks like the socket, that is closed by the
>>> primary, can get into a state unsuitable for WaitForMultipleObjects().
>> I wonder if FD_CLOSE is edge-triggered, and it's already told us once.
> Can you reproduce it with this patch?
Unfortunately, this fix (with the correction "(cur_event &
WL_SOCKET_MASK)" -> "(cur_event->events & WL_SOCKET_MASK") doesn't work,
because we have two separate calls to libpqrcv_PQgetResult():
> Then we get COMMAND_OK here:
>     res = libpqrcv_PQgetResult(conn->streamConn);
>     if (PQresultStatus(res) == PGRES_COMMAND_OK)
> and finally just hang at:
>     /* Verify that there are no more results. */
>     res = libpqrcv_PQgetResult(conn->streamConn);
The libpqrcv_PQgetResult function, in turn, invokes WaitLatchOrSocket()
where WaitEvents are defined locally, and the closed flag set on the
first invocation but expected to be checked on second.
>> I've managed to reproduce this failure too.
>> Removing "shutdown(MyProcPort->sock, SD_SEND);" doesn't help here, so
>> the culprit is exactly "closesocket(MyProcPort->sock);".
>>
> Ugh.  Did you try removing the closesocket and keeping shutdown?
> I don't recall if we tried that combination before.
Even with shutdown() only I still observe WaitForMultipleObjects()
hanging (and WSAPoll() returns POLLHUP for the socket).

As to your concern regarding other clients, I suspect that this issue is
caused by libpqwalreceiver' specific call pattern and may be other
clients just don't do that. I need some more time to analyze this.

Best regards,
Alexander

Re: Multiple Query IDs for a rewritten parse tree

[Andrey V. Lepikhov]

On 1/10/22 9:51 AM, Julien Rouhaud wrote:

On Mon, Jan 10, 2022 at 09:10:59AM +0500, Andrey V. Lepikhov wrote:

I can add one more use case.
Our extension for freezing query plan uses query tree comparison technique
to prove, that the plan can be applied (and we don't need to execute
planning procedure at all).
The procedure of a tree equality checking is expensive and we use cheaper
queryId comparison to identify possible candidates. So here, for the better
performance and queries coverage, we need to use query tree normalization -
queryId should be stable to some modifications in a query text which do not
change semantics.
As an example, query plan with external parameters can be used to execute
constant query if these constants correspond by place and type to the
parameters. So, queryId calculation technique returns also pointers to all
constants and parameters found during the calculation.


I'm also working on a similar extension, and yes you can't accept any
fingerprinting approach for that.  I don't know what are the exact heuristics
of your cheaper queryid calculation are, but is it reasonable to use it with
something like pg_stat_statements?  If yes, you don't really need two queryid
approach for the sake of this single extension and therefore don't need to
store multiple jumble state or similar per statement.  Especially since
requiring another one would mean a performance drop as soon as you want to use
something as common as pg_stat_statements.

I think, pg_stat_statements can live with an queryId generator of the 
sr_plan extension. But It replaces all constants with $XXX parameter at 
the query string. In our extension user defines which plan is optimal 
and which constants can be used as parameters in the plan.
One drawback I see here - creating or dropping of my extension changes 
behavior of pg_stat_statements that leads to distortion of the DB load 
profile. Also, we haven't guarantees, that another extension will work 
correctly (or in optimal way) with such queryId.


--
regards,
Andrey Lepikhov
Postgres Professional