Re: Marking some contrib modules as trusted extensions

[Sandro Santilli]

On Thu, Feb 13, 2020 at 07:09:18PM -0500, Tom Lane wrote:
> I wrote:
> > Andres Freund  writes:
> >> It seems to me that FROM UNPACKAGED shouldn't support trusted.
> 
> > Hmm, seems like a reasonable idea, but I'm not quite sure how to mechanize
> > it given that "unpackaged" isn't magic in any way so far as extension.c
> > is concerned.  Maybe we could decide that the time for supporting easy
> > updates from pre-9.1 is past, and just remove all the unpackaged-to-XXX
> > scripts?  Maybe even remove the "FROM version" option altogether.
> 
> [ thinks some more... ]  A less invasive idea would be to insist that
> you be superuser to use the FROM option.  But I'm thinking that the
> unpackaged-to-XXX scripts are pretty much dead letters anymore.  Has
> anyone even tested them in years?  How much longer do we want to be
> on the hook to fix them?

PostGIS uses unpackaged-to-XXX pretty heavily, and has it under
automated testing (which broke since "FROM unpackaged" support was
removed, see 14514.1581638...@sss.pgh.pa.us)

We'd be ok with requiring SUPERUSER for doing that, since that's
what is currently required so nothing would change for us.

Instead, dropping UPGRADE..FROM completely puts us in trouble of
having to find another way to "package" postgis objects.

--strk;

Re: Use compiler intrinsics for bit ops in hash

[David Fetter]

On Fri, Jan 31, 2020 at 04:59:18PM +0100, David Fetter wrote:
> On Wed, Jan 15, 2020 at 03:45:12PM -0800, Jesse Zhang wrote:
> > On Tue, Jan 14, 2020 at 2:09 PM David Fetter  wrote:
> > > > The changes in hash AM and SIMPLEHASH do look like a net positive
> > > > improvement. My biggest cringe might be in pg_bitutils:
> > > >
> > > > 1. Is ceil_log2_64 dead code?
> > >
> > > Let's call it nascent code. I suspect there are places it could go, if
> > > I look for them.  Also, it seemed silly to have one without the other.
> > >
> > 
> > While not absolutely required, I'd like us to find at least one
> > place and start using it. (Clang also nags at me when we have
> > unused functions).
> 
> Done in the expanded patches attached.

These bit-rotted a little, so I've updated them.

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

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
>From 90d3575b0716839b76953023bb63242b1d0698ef Mon Sep 17 00:00:00 2001
From: David Fetter 
Date: Wed, 29 Jan 2020 02:09:59 -0800
Subject: [PATCH v5 1/3] de-long-ify
To: hackers
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="2.24.1"

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


diff --git a/src/backend/access/gist/gistbuildbuffers.c b/src/backend/access/gist/gistbuildbuffers.c
index 4b562d8d3f..482a569814 100644
--- a/src/backend/access/gist/gistbuildbuffers.c
+++ b/src/backend/access/gist/gistbuildbuffers.c
@@ -34,11 +34,11 @@ static void gistPlaceItupToPage(GISTNodeBufferPage *pageBuffer,
 IndexTuple item);
 static void gistGetItupFromPage(GISTNodeBufferPage *pageBuffer,
 IndexTuple *item);
-static long gistBuffersGetFreeBlock(GISTBuildBuffers *gfbb);
-static void gistBuffersReleaseBlock(GISTBuildBuffers *gfbb, long blocknum);
+static uint64 gistBuffersGetFreeBlock(GISTBuildBuffers *gfbb);
+static void gistBuffersReleaseBlock(GISTBuildBuffers *gfbb, uint64 blocknum);
 
-static void ReadTempFileBlock(BufFile *file, long blknum, void *ptr);
-static void WriteTempFileBlock(BufFile *file, long blknum, void *ptr);
+static void ReadTempFileBlock(BufFile *file, uint64 blknum, void *ptr);
+static void WriteTempFileBlock(BufFile *file, uint64 blknum, void *ptr);
 
 
 /*
@@ -64,7 +64,7 @@ gistInitBuildBuffers(int pagesPerBuffer, int levelStep, int maxLevel)
 	/* Initialize free page management. */
 	gfbb->nFreeBlocks = 0;
 	gfbb->freeBlocksLen = 32;
-	gfbb->freeBlocks = (long *) palloc(gfbb->freeBlocksLen * sizeof(long));
+	gfbb->freeBlocks = (int64 *) palloc(gfbb->freeBlocksLen * sizeof(int64));
 
 	/*
 	 * Current memory context will be used for all in-memory data structures
@@ -469,7 +469,7 @@ gistPopItupFromNodeBuffer(GISTBuildBuffers *gfbb, GISTNodeBuffer *nodeBuffer,
 /*
  * Select a currently unused block for writing to.
  */
-static long
+static uint64
 gistBuffersGetFreeBlock(GISTBuildBuffers *gfbb)
 {
 	/*
@@ -487,7 +487,7 @@ gistBuffersGetFreeBlock(GISTBuildBuffers *gfbb)
  * Return a block# to the freelist.
  */
 static void
-gistBuffersReleaseBlock(GISTBuildBuffers *gfbb, long blocknum)
+gistBuffersReleaseBlock(GISTBuildBuffers *gfbb, uint64 blocknum)
 {
 	int			ndx;
 
@@ -495,9 +495,9 @@ gistBuffersReleaseBlock(GISTBuildBuffers *gfbb, long blocknum)
 	if (gfbb->nFreeBlocks >= gfbb->freeBlocksLen)
 	{
 		gfbb->freeBlocksLen *= 2;
-		gfbb->freeBlocks = (long *) repalloc(gfbb->freeBlocks,
+		gfbb->freeBlocks = (int64 *) repalloc(gfbb->freeBlocks,
 			 gfbb->freeBlocksLen *
-			 sizeof(long));
+			 sizeof(uint64));
 	}
 
 	/* Add blocknum to array */
@@ -755,7 +755,7 @@ gistRelocateBuildBuffersOnSplit(GISTBuildBuffers *gfbb, GISTSTATE *giststate,
  */
 
 static void
-ReadTempFileBlock(BufFile *file, long blknum, void *ptr)
+ReadTempFileBlock(BufFile *file, uint64 blknum, void *ptr)
 {
 	if (BufFileSeekBlock(file, blknum) != 0)
 		elog(ERROR, "could not seek temporary file: %m");
@@ -764,7 +764,7 @@ ReadTempFileBlock(BufFile *file, long blknum, void *ptr)
 }
 
 static void
-WriteTempFileBlock(BufFile *file, long blknum, void *ptr)
+WriteTempFileBlock(BufFile *file, uint64 blknum, void *ptr)
 {
 	if (BufFileSeekBlock(file, blknum) != 0)
 		elog(ERROR, "could not seek temporary file: %m");
diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index c46764bf42..4fc478640a 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -70,7 +70,7 @@ static int	_SPI_pquery(QueryDesc *queryDesc, bool fire_triggers, uint64 tcount);
 static void _SPI_error_callback(void *arg);
 
 static void _SPI_cursor_operation(Portal portal,
-  FetchDirection direction, long count,
+  FetchDirection direction, uint64 count,
   DestReceiver *dest);
 
 static SPIPlanPtr _SPI_make_plan_non_temp(SPIPlanPtr plan);
@@ -493,7 +493,7 @@ SPI_inside_nonatomic_context(void)
 
 /* Pars

Re: Commit fest manager for 2020-03

[Michael Banck]

Hi,

On Wed, Feb 26, 2020 at 04:41:12PM +0900, Michael Paquier wrote:
> The next commit fets is going to begin in a couple of days, and there
> is a total of 207 patches registered as of today.  We don't have any
> manager yet, so is there any volunteer for taking the lead this time?

This is the last one for v13 I think? So probably somebody quite
experienced should handle this one. Or (if there is one already for
v13?) maybe even the Release Team themselves?


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer

Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz

Re: Resolving the python 2 -> python 3 mess

[Andrew Dunstan]

On 2/26/20 2:47 AM, Andrew Dunstan wrote:
> On 2/25/20 8:24 PM, Andrew Dunstan wrote:
>> On 2/25/20 7:08 PM, Tom Lane wrote:
>>> Andrew Dunstan  writes:
 On 2/25/20 5:06 PM, Tom Lane wrote:
> No joy there --- now that I look closer, it seems the cfbot doesn't
> build any of the external-language PLs on Windows.  I'll have to
> wait for some reviewer to try it.
 What are the requirements for testing? bowerbird builds with python 2.7,
 although I guess I should really try to upgrade it  3.x.
>>> Has to be python 3, unfortunately; the patch has no effect on a
>>> python 2 build.
>>>
>>> 
>>
>> Yeah, I have python3 working on drongo, I'll test there.
>>
> It's almost there, you need to add something like this to Mkvcbuild.pm:
>
>
>     if ($solution->{options}->{python2_stub})
>     {
>         my $plpython2_stub =
>           $solution->AddProject('plpython2', 'dll', 'PLs',
> 'src/pl/stub_plpython2');
>         $plpython2_stub->AddReference($postgres);
>     }





However, when it get to testing contrib it complains like this:



Checking hstore_plpython
C:/prog/bf/root/HEAD/pgsql/Release/pg_regress/pg_regress
--bindir=C:/prog/bf/root/HEAD/pgsql/Release/psql
--dbname=contrib_regression --load-ex
tension=hstore --load-extension=plpythonu
--load-extension=hstore_plpythonu hstore_plpython
(using postmaster on localhost, default port)
== dropping database "contrib_regression" ==
DROP DATABASE
== creating database "contrib_regression" ==
CREATE DATABASE
ALTER DATABASE
== installing hstore  ==
CREATE EXTENSION
== installing plpythonu   ==
CREATE EXTENSION
== installing hstore_plpythonu    ==
ERROR:  could not access file "$libdir/hstore_plpython2": No such file
or directory
command failed: "C:/prog/bf/root/HEAD/pgsql/Release/psql/psql" -X -c
"CREATE EXTENSION IF NOT EXISTS \"hstore_plpythonu\"" "contrib_regression"


So there's a bit more work to do.


cheers


andrew



-- 

Andrew Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Marking some contrib modules as trusted extensions

[Andres Freund]

Hi,

On 2020-02-26 09:11:21 +0100, Sandro Santilli wrote:
> PostGIS uses unpackaged-to-XXX pretty heavily, and has it under
> automated testing (which broke since "FROM unpackaged" support was
> removed, see 14514.1581638...@sss.pgh.pa.us)
> 
> We'd be ok with requiring SUPERUSER for doing that, since that's
> what is currently required so nothing would change for us.
> 
> Instead, dropping UPGRADE..FROM completely puts us in trouble of
> having to find another way to "package" postgis objects.

Coul you explain what postgis is trying to achieve with FROM unpackaged?

Greetings,

Andres Freund

Re: Marking some contrib modules as trusted extensions

[Sandro Santilli]

On Wed, Feb 26, 2020 at 12:17:37AM -0800, Andres Freund wrote:
> Hi,
> 
> On 2020-02-26 09:11:21 +0100, Sandro Santilli wrote:
> > PostGIS uses unpackaged-to-XXX pretty heavily, and has it under
> > automated testing (which broke since "FROM unpackaged" support was
> > removed, see 14514.1581638...@sss.pgh.pa.us)
> > 
> > We'd be ok with requiring SUPERUSER for doing that, since that's
> > what is currently required so nothing would change for us.
> > 
> > Instead, dropping UPGRADE..FROM completely puts us in trouble of
> > having to find another way to "package" postgis objects.
> 
> Coul you explain what postgis is trying to achieve with FROM unpackaged?

We're turning a non-extension based install into an extension-based
one. Common need for those who came to PostGIS way before EXTENSION
was even invented and for those who remained there for the bigger
flexibility (for example to avoid the raster component, which was
unavoidable with EXTENSION mechanism until PostGIS 3.0).

For the upgrades to 3.0.0 when coming from a previous version we're
using that `FROM unpackaged` SYNTAX for re-packaging the raster
component for those who still want it (raster objects are unpackaged
from 'postgis' extension on EXTENSION UPDATE because there was no other
way to move them from an extension to another).

I guess it would be ok for us to do the packaging directly from the
scripts that would run on `CREATE EXTENSION postgis`, but would that
mean we'd take the security risk you're trying to avoid by dropping
the `FROM unpackaged` syntax ?

--strk;

Re: [PATCH] pg_upgrade: report the reason for failing to open the cluster version file

[Daniel Gustafsson]

> On 26 Feb 2020, at 02:48, Michael Paquier  wrote:
> 
> On Tue, Feb 25, 2020 at 11:55:06PM +, Dagfinn Ilmari Mannsåker wrote:
>> @@ -164,11 +164,11 @@ get_major_server_version(ClusterInfo *cluster)
>>  snprintf(ver_filename, sizeof(ver_filename), "%s/PG_VERSION",
>>   cluster->pgdata);
>>  if ((version_fd = fopen(ver_filename, "r")) == NULL)
>> -pg_fatal("could not open version file: %s\n", ver_filename);
>> +pg_fatal("could not open version file \"%s\": %m\n", 
>> ver_filename);
> 
> Here I think that it would be better to just use "could not open
> file" as we know that we are dealing with a version file already
> thanks to ver_filename.

Isn't that a removal of detail with very little benefit?  Not everyone running
pg_upgrade will know internal filenames, and the ver_filename contains the
pgdata path as well which might provide additional clues in case this goes
wrong.

cheers ./daniel

Re: [PATCH] pg_upgrade: report the reason for failing to open the cluster version file

[Magnus Hagander]

On Wed, Feb 26, 2020 at 9:56 AM Daniel Gustafsson  wrote:
>
> > On 26 Feb 2020, at 02:48, Michael Paquier  wrote:
> >
> > On Tue, Feb 25, 2020 at 11:55:06PM +, Dagfinn Ilmari Mannsåker wrote:
> >> @@ -164,11 +164,11 @@ get_major_server_version(ClusterInfo *cluster)
> >>  snprintf(ver_filename, sizeof(ver_filename), "%s/PG_VERSION",
> >>   cluster->pgdata);
> >>  if ((version_fd = fopen(ver_filename, "r")) == NULL)
> >> -pg_fatal("could not open version file: %s\n", ver_filename);
> >> +pg_fatal("could not open version file \"%s\": %m\n", 
> >> ver_filename);
> >
> > Here I think that it would be better to just use "could not open
> > file" as we know that we are dealing with a version file already
> > thanks to ver_filename.
>
> Isn't that a removal of detail with very little benefit?  Not everyone running
> pg_upgrade will know internal filenames, and the ver_filename contains the
> pgdata path as well which might provide additional clues in case this goes
> wrong.

+1, seems like that would be a regression in value.

Committed as per Dagfinn's v2.

-- 
 Magnus Hagander
 Me: https://www.hagander.net/
 Work: https://www.redpill-linpro.com/

Re: [PATCH] pg_upgrade: report the reason for failing to open the cluster version file

[Michael Paquier]

On Wed, Feb 26, 2020 at 10:06:38AM +0100, Magnus Hagander wrote:
> +1, seems like that would be a regression in value.

Having more generic messages is less work for translators, we have
PG_VERSION in the file name, and that's more complicated to translate
in both French and Japanese.  No idea about other languages.

> Committed as per Dagfinn's v2.

Anyway, too late :)
--
Michael


signature.asc
Description: PGP signature

Re: Yet another vectorized engine

[Konstantin Knizhnik]

On 25.02.2020 19:40, Konstantin Knizhnik wrote:

I have ported vectorize_engine  for zedstore (vertical table AM).
Results of TPCH-10G/Q1 are the following:

par.workers
PG9_6
vectorize=off
PG9_6
vectorize=on
master
vectorize=off
jit=on
master
vectorize=off
jit=off master
vectorize=on
jit=on  master
vectorize=on
jit=off zedstore vectorize=off
jit=on
zedstore vectorize=off
jit=off zedstore vectorize=on
jit=on  zedstore vectorize=on
jit=off
0
36
20
16
25.5
15
17.5
18
26
17
19
4
10
-
5   7
-
-   5
7
-
-




After correct calculation of used columns bitmapset and passing it to 
table_beginscan_with_column_projection function zedstore+vectorize_engine

show the best result (without parallel execution):


par.workers
PG9_6
vectorize=off
PG9_6
vectorize=on
master
vectorize=off
jit=on
master
vectorize=off
jit=off master
vectorize=on
jit=on  master
vectorize=on
jit=off zedstore vectorize=off
jit=on
zedstore vectorize=off
jit=off zedstore vectorize=on
jit=on  zedstore vectorize=on
jit=off
0
36
20
16
25.5
15
17.5
18
26
14
16
4
10
-
5   7
-
-   5
7
-
-



but still the difference with vanilla is minimal.


Profiler top is the following:

  16.30%  postgres  postgres [.] zedstoream_getnexttile
   6.98%  postgres  postgres [.] decode_chunk
   6.68%  postgres  liblz4.so.1.7.1  [.] LZ4_decompress_safe
   5.37%  postgres  vectorize_engine.so  [.] vfloat8_accum
   5.23%  postgres  postgres [.] bpchareq

--

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

Re: Yet another vectorized engine

[Hubert Zhang]

Hi Konstantin,

On Tue, Feb 25, 2020 at 6:44 PM Konstantin Knizhnik <
k.knizh...@postgrespro.ru> wrote:

>
>
> On 25.02.2020 11:06, Hubert Zhang wrote:
>
> Hi Konstantin,
>
> I checkout your branch pg13 in repo
> https://github.com/zhangh43/vectorize_engine
> After I fixed some compile error, I tested Q1 on TPCH-10G
> The result is different from yours and vectorize version is too slow. Note
> that I disable parallel worker by default.
> no JIT no Vectorize:  36 secs
> with JIT only: 23 secs
> with Vectorize only:   33 secs
> JIT + Vectorize: 29 secs
>
> My config option is `CFLAGS='-O3 -g -march=native'
> --prefix=/usr/local/pgsql/ --disable-cassert --enable-debug --with-llvm`
> I will do some spike on why vectorized is so slow. Could you please
> provide your compile option and the TPCH dataset size and your
> queries(standard Q1?) to help me to debug on it.
>
>
>
> Hi, Hubert
>
> Sorry, looks like I have used slightly deteriorated snapshot of master so
> I have not noticed some problems.
> Fixes are committed.
>
> Most of the time is spent in unpacking heap tuple
> (tts_buffer_heap_getsomeattrs):
>
>   24.66%  postgres  postgres [.] tts_buffer_heap_getsomeattrs
>8.28%  postgres  vectorize_engine.so  [.] VExecStoreColumns
>5.94%  postgres  postgres [.] HeapTupleSatisfiesVisibility
>4.21%  postgres  postgres [.] bpchareq
>4.12%  postgres  vectorize_engine.so  [.] vfloat8_accum
>
>
> In my version of nodeSeqscan I do not keep all fetched 1024 heap tuples
> but stored there attribute values in vector columns immediately.
> But to avoid extraction of useless data it is necessary to know list of
> used columns.
> The same problem is solved in zedstore, but unfortunately there is no
> existed method in Postgres to get list
> of used attributes. I have done it but my last implementation contains
> error which cause loading of all columns.
> Fixed version is committed.
>
> Now profile without JIT is:
>
>  15.52%  postgres  postgres [.] tts_buffer_heap_getsomeattrs
>   10.25%  postgres  postgres [.] ExecInterpExpr
>6.54%  postgres  postgres [.] HeapTupleSatisfiesVisibility
>5.12%  postgres  vectorize_engine.so  [.] VExecStoreColumns
>4.86%  postgres  postgres [.] bpchareq
>4.80%  postgres  vectorize_engine.so  [.] vfloat8_accum
>3.78%  postgres  postgres [.] tts_minimal_getsomeattrs
>3.66%  postgres  vectorize_engine.so  [.] VExecAgg
>3.38%  postgres  postgres [.] hashbpchar
>
> and with JIT:
>
>  13.88%  postgres  postgres [.] tts_buffer_heap_getsomeattrs
>7.15%  postgres  vectorize_engine.so  [.] vfloat8_accum
>6.03%  postgres  postgres [.] HeapTupleSatisfiesVisibility
>5.55%  postgres  postgres [.] bpchareq
>4.42%  postgres  vectorize_engine.so  [.] VExecStoreColumns
>4.19%  postgres  postgres [.] hashbpchar
>4.09%  postgres  vectorize_engine.so  [.] vfloat8pl
>
>
I also tested Q1 with your latest code. Result of vectorized is still slow.
PG13 native: 38 secs
PG13 Vec: 30 secs
PG13 JIT: 23 secs
PG13 JIT+Vec: 27 secs

My perf result is as belows. There are three parts:
1. lookup_hash_entry(43.5%) this part is not vectorized yet.
2. scan part: fetch_input_tuple(36%)
3. vadvance_aggregates part(20%)
I also perfed on PG96 vectorized version and got similar perf results and
running time of vectorized PG96 and PG13 are also similar. But PG13 is much
faster than PG96. So I just wonder whether we merge all the latest executor
code of PG13 into the vectorized PG13 branch?

- agg_fill_hash_table ◆ - 43.50% lookup_hash_entry (inlined) ▒ + 39.07%
LookupTupleHashEntry ▒ 0.56% ExecClearTuple (inlined) ▒ - 36.06%
fetch_input_tuple ▒ - ExecProcNode (inlined) ▒ - 36.03% VExecScan ▒ -
34.60% ExecScanFetch (inlined) ▒ - ExecScanFetch (inlined) ▒ - VSeqNext ▒ +
16.64% table_scan_getnextslot (inlined) ▒ - 10.29% slot_getsomeattrs
(inlined) ▒ - 10.17% slot_getsomeattrs_int ▒ + tts_buffer_heap_getsomeattrs
▒ 7.14% VExecStoreColumns ▒ + 1.38% ExecQual (inlined) ▒ - 20.30%
Vadvance_aggregates (inlined) ▒ - 17.46% Vadvance_transition_function
(inlined) ▒ + 11.95% vfloat8_accum ▒ + 4.74% vfloat8pl ▒ 0.75% vint8inc_any
▒ + 2.77% ExecProject (inlined)

-- 
Thanks

Hubert Zhang

Re: truncating timestamps on arbitrary intervals

[John Naylor]

On Wed, Feb 26, 2020 at 3:51 PM David Fetter  wrote:
>
> I believe the following should error out, but doesn't.
>
> # SELECT date_trunc_interval('1 year 1 ms', TIMESTAMP '2001-02-16 20:38:40');
>  date_trunc_interval
> ═
>  2001-01-01 00:00:00
> (1 row)

You're quite right. I forgot to add error checking for
second-and-below units. I've added your example to the tests. (I
neglected to mention in my first email that because I chose to convert
the interval to the pg_tm struct (seemed easiest), it's not
straightforward how to allow multiple unit types, and I imagine the
use case is small, so I had it throw an error.)

> Please find attached an update that I believe fixes the bug I found in
> a principled way.

Thanks for that! I made a couple adjustments and incorporated your fix
into v3: While working on v1, I noticed the DTK_FOO macros already had
an idiom for bitmasking (see utils/datetime.h), so I used that instead
of a bespoke enum. Also, since the bitmask is checked once, I removed
the individual member checks, allowing me to remove all the gotos.

There's another small wrinkle: Since we store microseconds internally,
it's neither convenient nor useful to try to error out for things like
'2 ms 500 us', since that is just as well written as '2500 us', and
stored exactly the same. I'm inclined to just skip the millisecond
check and just use microseconds, but I haven't done that yet.

Also, I noticed this bug in v1:

SELECT date_trunc_interval('17 days', TIMESTAMP '2001-02-16 20:38:40.123456');
 date_trunc_interval
-
 2001-01-31 00:00:00
(1 row)

This is another consequence of month and day being 1-based. Fixed,
with new tests.

-- 
John Naylorhttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


v3-datetrunc_interval.patch
Description: Binary data

Re: BUG #16108: Colorization to the output of command-line has unproperly behaviors at Windows platform

[Michail Nikolaev]

Hello.

Looks totally fine to me now.

So, I need to mark it as "ready to commiter", right?

Re: Parallel copy

[Amit Kapila]

On Tue, Feb 25, 2020 at 9:30 PM Tomas Vondra
 wrote:
>
> On Sun, Feb 23, 2020 at 05:09:51PM -0800, Andres Freund wrote:
> >Hi,
> >
> >> The one piece of information I'm missing here is at least a very rough
> >> quantification of the individual steps of CSV processing - for example
> >> if parsing takes only 10% of the time, it's pretty pointless to start by
> >> parallelising this part and we should focus on the rest. If it's 50% it
> >> might be a different story. Has anyone done any measurements?
> >
> >Not recently, but I'm pretty sure that I've observed CSV parsing to be
> >way more than 10%.
> >
>
> Perhaps. I guess it'll depend on the CSV file (number of fields, ...),
> so I still think we need to do some measurements first.
>

Agreed.

> I'm willing to
> do that, but (a) I doubt I'll have time for that until after 2020-03,
> and (b) it'd be good to agree on some set of typical CSV files.
>

Right, I don't know what is the best way to define that.  I can think
of the below tests.

1. A table with 10 columns (with datatypes as integers, date, text).
It has one index (unique/primary). Load with 1 million rows (basically
the data should be probably 5-10 GB).
2. A table with 10 columns (with datatypes as integers, date, text).
It has three indexes, one index can be (unique/primary). Load with 1
million rows (basically the data should be probably 5-10 GB).
3. A table with 10 columns (with datatypes as integers, date, text).
It has three indexes, one index can be (unique/primary). It has before
and after trigeers. Load with 1 million rows (basically the data
should be probably 5-10 GB).
4. A table with 10 columns (with datatypes as integers, date, text).
It has five or six indexes, one index can be (unique/primary). Load
with 1 million rows (basically the data should be probably 5-10 GB).

Among all these tests, we can check how much time did we spend in
reading, parsing the csv files vs. rest of execution?

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

Re: BUG #16108: Colorization to the output of command-line has unproperly behaviors at Windows platform

[Juan José Santamaría Flecha]

On Wed, Feb 26, 2020 at 11:48 AM Michail Nikolaev <
michail.nikol...@gmail.com> wrote:

>
> Looks totally fine to me now.
>
> So, I need to mark it as "ready to commiter", right?
>

Yes, that's right. Thanks for reviewing it.

Regards

Re: Resolving the python 2 -> python 3 mess

[Andrew Dunstan]

On 2/26/20 3:17 AM, Andrew Dunstan wrote:
> On 2/26/20 2:47 AM, Andrew Dunstan wrote:
>> On 2/25/20 8:24 PM, Andrew Dunstan wrote:
>>> On 2/25/20 7:08 PM, Tom Lane wrote:
 Andrew Dunstan  writes:
> On 2/25/20 5:06 PM, Tom Lane wrote:
>> No joy there --- now that I look closer, it seems the cfbot doesn't
>> build any of the external-language PLs on Windows.  I'll have to
>> wait for some reviewer to try it.
> What are the requirements for testing? bowerbird builds with python 2.7,
> although I guess I should really try to upgrade it  3.x.
 Has to be python 3, unfortunately; the patch has no effect on a
 python 2 build.


>>> Yeah, I have python3 working on drongo, I'll test there.
>>>
>> It's almost there, you need to add something like this to Mkvcbuild.pm:
>>
>>
>>     if ($solution->{options}->{python2_stub})
>>     {
>>         my $plpython2_stub =
>>           $solution->AddProject('plpython2', 'dll', 'PLs',
>> 'src/pl/stub_plpython2');
>>         $plpython2_stub->AddReference($postgres);
>>     }
>
>
>
>
> However, when it get to testing contrib it complains like this:
>
>
> 
> Checking hstore_plpython
> C:/prog/bf/root/HEAD/pgsql/Release/pg_regress/pg_regress
> --bindir=C:/prog/bf/root/HEAD/pgsql/Release/psql
> --dbname=contrib_regression --load-ex
> tension=hstore --load-extension=plpythonu
> --load-extension=hstore_plpythonu hstore_plpython
> (using postmaster on localhost, default port)
> == dropping database "contrib_regression" ==
> DROP DATABASE
> == creating database "contrib_regression" ==
> CREATE DATABASE
> ALTER DATABASE
> == installing hstore  ==
> CREATE EXTENSION
> == installing plpythonu   ==
> CREATE EXTENSION
> == installing hstore_plpythonu    ==
> ERROR:  could not access file "$libdir/hstore_plpython2": No such file
> or directory
> command failed: "C:/prog/bf/root/HEAD/pgsql/Release/psql/psql" -X -c
> "CREATE EXTENSION IF NOT EXISTS \"hstore_plpythonu\"" "contrib_regression"
>
>
> So there's a bit more work to do.
>
>


This seems to fix it.


cheers


andrew



-- 
Andrew Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

diff --git a/src/tools/msvc/vcregress.pl b/src/tools/msvc/vcregress.pl
index 82dca29a61..d3a07f84bb 100644
--- a/src/tools/msvc/vcregress.pl
+++ b/src/tools/msvc/vcregress.pl
@@ -423,15 +423,15 @@ sub subdircheck
 
 		@opts = grep { $_ !~ /plpythonu/ } @opts;
 
-		if (-d "$topdir/$Config/plpython2")
+		if (-d "$topdir/$Config/plpython3")
 		{
-			push @opts, "--load-extension=plpythonu";
-			push @opts, '--load-extension=' . $module . 'u';
+			@tests = mangle_plpython3(\@tests);
 		}
-		else
+		elsif (-d "$topdir/$Config/plpython2")
 		{
-			# must be python 3
-			@tests = mangle_plpython3(\@tests);
+			# if python3 doesn't exist this isn't the stub module
+			push @opts, "--load-extension=plpythonu";
+			push @opts, '--load-extension=' . $module . 'u';
 		}
 	}
 

Re: Yet another vectorized engine

[Konstantin Knizhnik]

On 26.02.2020 13:11, Hubert Zhang wrote:



and with JIT:

 13.88%  postgres  postgres [.]
tts_buffer_heap_getsomeattrs
   7.15%  postgres  vectorize_engine.so  [.] vfloat8_accum
   6.03%  postgres  postgres [.]
HeapTupleSatisfiesVisibility
   5.55%  postgres  postgres [.] bpchareq
   4.42%  postgres  vectorize_engine.so  [.] VExecStoreColumns
   4.19%  postgres  postgres [.] hashbpchar
   4.09%  postgres  vectorize_engine.so  [.] vfloat8pl


I also tested Q1 with your latest code. Result of vectorized is still 
slow.

PG13 native: 38 secs
PG13 Vec: 30 secs
PG13 JIT: 23 secs
PG13 JIT+Vec: 27 secs



It is strange that your results are much slower than my and profile is 
very different.

Which postgres configuration you are using?



My perf result is as belows. There are three parts:
1. lookup_hash_entry(43.5%) this part is not vectorized yet.
It is vectorized in some sense: lookup_hash_entry performs bulk of hash 
lookups and pass array with results of such lookups to aggregate 
transmit functions.
It will be possible to significantly increase speed of HashAgg if we 
store data in order of grouping attributes and use RLE (run length 
encoding) to peform just one
hash lookup for group of values. But it requires creation of special 
partitions (like it is done in Vertica and VOPS).



2. scan part: fetch_input_tuple(36%)
3. vadvance_aggregates part(20%)
I also perfed on PG96 vectorized version and got similar perf results 
and running time of vectorized PG96 and PG13 are also similar. But 
PG13 is much faster than PG96. So I just wonder whether we merge all 
the latest executor code of PG13 into the vectorized PG13 branch?


Sorry, I do not understand the question. vectorize_executor contains 
patched versions of nodeSeqscan  and nodeAgg from standard executor.
When performing porting to PG13, I took the latest version of nodeAgg 
and tried to apply your patches to it. Certainly not always it was 
possible and I have to rewrite a lt of places. Concerning nodeSeqscan - 
I took old version from vectorize_executor and port it to PG13.




- agg_fill_hash_table ◆ - 43.50% lookup_hash_entry (inlined) ▒ + 
39.07% LookupTupleHashEntry ▒ 0.56% ExecClearTuple (inlined) ▒ - 
36.06% fetch_input_tuple ▒ - ExecProcNode (inlined) ▒ - 36.03% 
VExecScan ▒ - 34.60% ExecScanFetch (inlined) ▒ - ExecScanFetch 
(inlined) ▒ - VSeqNext ▒ + 16.64% table_scan_getnextslot (inlined) ▒ - 
10.29% slot_getsomeattrs (inlined) ▒ - 10.17% slot_getsomeattrs_int ▒ 
+ tts_buffer_heap_getsomeattrs ▒ 7.14% VExecStoreColumns ▒ + 1.38% 
ExecQual (inlined) ▒ - 20.30% Vadvance_aggregates (inlined) ▒ - 17.46% 
Vadvance_transition_function (inlined) ▒ + 11.95% vfloat8_accum ▒ + 
4.74% vfloat8pl ▒ 0.75% vint8inc_any ▒ + 2.77% ExecProject (inlined)




It is strange that I am not seeing lookup_hash_entry in profile in my case.

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

Re: Wait event that should be reported while waiting for WAL archiving to finish

[Fujii Masao]

On 2020/02/18 12:39, Michael Paquier wrote:

On Mon, Feb 17, 2020 at 10:21:23PM +0900, Fujii Masao wrote:

On 2020/02/17 18:48, Michael Paquier wrote:

Actually, I have some questions:
1) Should a new wait event be added in recoveryPausesHere()?  That
would be IMO useful.


Yes, it's useful, I think. But it's better to implement that
as a separate patch.


No problem for me.


On second thought, it's OK to add that event into the patch.
Attached is the updated version of the patch. This patch adds
two wait events for WAL archiving and recovery pause.



2) Perhaps those two points should be replaced with WaitLatch(), where
we would use the new wait events introduced?


For what? Maybe it should, but I'm not sure it's worth the trouble.


I don't have more to offer than signal handling consistency for both
without relying on pg_usleep()'s behavior depending on the platform,
and power consumption.  For the recovery pause, the second argument
may not be worth carrying, but we never had this argument for the
archiving wait, did we?


I have no idea about this. But I wonder how much that change
is helpful to reduce the power consumption because waiting
for WAL archive during the backup basically not so frequently
happens.

Regards,


--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 87586a7b06..6c0c93ffdc 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -1335,7 +1335,11 @@ postgres   27093  0.0  0.0  30096  2752 ?Ss   
11:34   0:00 postgres: ser
  Waiting in an extension.
 
 
- IPC
+ IPC
+ BackupWaitWalArchive
+ Waiting for WAL files required for the backup to be 
successfully archived.
+
+
  BgWorkerShutdown
  Waiting for background worker to shut down.
 
@@ -1467,6 +1471,10 @@ postgres   27093  0.0  0.0  30096  2752 ?Ss   
11:34   0:00 postgres: ser
  Promote
  Waiting for standby promotion.
 
+
+ RecoveryPause
+ Waiting for recovery to be resumed.
+
 
  ReplicationOriginDrop
  Waiting for a replication origin to become inactive to be 
dropped.
diff --git a/src/backend/access/transam/xlog.c 
b/src/backend/access/transam/xlog.c
index d19408b3be..5569606183 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -5945,7 +5945,9 @@ recoveryPausesHere(void)
 
while (RecoveryIsPaused())
{
+   pgstat_report_wait_start(WAIT_EVENT_RECOVERY_PAUSE);
pg_usleep(100L);/* 1000 ms */
+   pgstat_report_wait_end();
HandleStartupProcInterrupts();
}
 }
@@ -11129,7 +11131,9 @@ do_pg_stop_backup(char *labelfile, bool waitforarchive, 
TimeLineID *stoptli_p)
reported_waiting = true;
}
 
+   
pgstat_report_wait_start(WAIT_EVENT_BACKUP_WAIT_WAL_ARCHIVE);
pg_usleep(100L);
+   pgstat_report_wait_end();
 
if (++waits >= seconds_before_warning)
{
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index 462b4d7e06..2400bf31ee 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -3740,6 +3740,9 @@ pgstat_get_wait_ipc(WaitEventIPC w)
 
switch (w)
{
+   case WAIT_EVENT_BACKUP_WAIT_WAL_ARCHIVE:
+   event_name = "BackupWaitWalArchive";
+   break;
case WAIT_EVENT_BGWORKER_SHUTDOWN:
event_name = "BgWorkerShutdown";
break;
@@ -3839,6 +3842,9 @@ pgstat_get_wait_ipc(WaitEventIPC w)
case WAIT_EVENT_PROMOTE:
event_name = "Promote";
break;
+   case WAIT_EVENT_RECOVERY_PAUSE:
+   event_name = "RecoveryPause";
+   break;
case WAIT_EVENT_REPLICATION_ORIGIN_DROP:
event_name = "ReplicationOriginDrop";
break;
diff --git a/src/include/pgstat.h b/src/include/pgstat.h
index 3a65a51696..6d8332163c 100644
--- a/src/include/pgstat.h
+++ b/src/include/pgstat.h
@@ -817,7 +817,8 @@ typedef enum
  */
 typedef enum
 {
-   WAIT_EVENT_BGWORKER_SHUTDOWN = PG_WAIT_IPC,
+   WAIT_EVENT_BACKUP_WAIT_WAL_ARCHIVE = PG_WAIT_IPC,
+   WAIT_EVENT_BGWORKER_SHUTDOWN,
WAIT_EVENT_BGWORKER_STARTUP,
WAIT_EVENT_BTREE_PAGE,
WAIT_EVENT_CLOG_GROUP_UPDATE,
@@ -850,6 +851,7 @@ typedef enum
WAIT_EVENT_PARALLEL_FINISH,
WAIT_EVENT_PROCARRAY_GROUP_UPDATE,
WAIT_EVENT_PROMOTE,
+   WAIT_EVENT_RECOVERY_PAUSE,
WAIT_EVENT

Re: Commit fest manager for 2020-03

[David Steele]

On 2/26/20 3:15 AM, Michael Banck wrote:


On Wed, Feb 26, 2020 at 04:41:12PM +0900, Michael Paquier wrote:

The next commit fets is going to begin in a couple of days, and there
is a total of 207 patches registered as of today.  We don't have any
manager yet, so is there any volunteer for taking the lead this time?


This is the last one for v13 I think? So probably somebody quite
experienced should handle this one. Or (if there is one already for
v13?) maybe even the Release Team themselves?


I'm happy to be CFM for this commitfest.

I'm not sure it would be a good use of resources to have the release 
team performing CFM duties directly.  They will have enough on their 
plate and in any case I don't think the team has been announced yet.


Regards,
--
-David
da...@pgmasters.net

Re: [postgis-devel] About EXTENSION from UNPACKAGED on PostgreSQL 13

[Stephen Frost]

Greetings,

* Darafei "Komяpa" Praliaskouski (m...@komzpa.net) wrote:
> PostGIS 2.5 had raster and vector blended together in single extension.
> In PostGIS 3, they were split out into postgis and postgis_raster extensions.

For my 2c, at least, I still don't really get why that split was done.

> To upgrade, there is now postgis_extensions_upgrade() function, that
> unpackages the raster part out of postgis extensions, upgrades it, and
> packages raster functions back into postgis_raster by utilizing FROM
> UNPACKAGED.
> Removal of FROM UNPACKAGED breaks PostGIS 2.5 -> 3.0 upgrade path, and
> we haven't yet found a proper replacement since such removal wasn't
> something we were expecting.

I agree that there probably isn't a very good path to allow an extension
to be split up like that without having to drop some things.  An
alternative would have been to *not* split up postgis, but rather to
have a postgis_raster and a postgis_vector.  Adding in support for other
ways to migrate a function from one extension to another would make
sense too.

Thanks,

Stephen


signature.asc
Description: PGP signature

Re: [postgis-devel] About EXTENSION from UNPACKAGED on PostgreSQL 13

[Sandro Santilli]

On Wed, Feb 26, 2020 at 08:55:03AM -0500, Stephen Frost wrote:
> Greetings,
> 
> * Darafei "Komяpa" Praliaskouski (m...@komzpa.net) wrote:
> > PostGIS 2.5 had raster and vector blended together in single extension.
> > In PostGIS 3, they were split out into postgis and postgis_raster 
> > extensions.
> 
> For my 2c, at least, I still don't really get why that split was done.

It's pretty easy to understand: to let user decide what he needs and
what not.

> > Removal of FROM UNPACKAGED breaks PostGIS 2.5 -> 3.0 upgrade path, and
> > we haven't yet found a proper replacement since such removal wasn't
> > something we were expecting.
> 
> I agree that there probably isn't a very good path to allow an extension
> to be split up like that without having to drop some things.  An
> alternative would have been to *not* split up postgis, but rather to
> have a postgis_raster and a postgis_vector.  Adding in support for other
> ways to migrate a function from one extension to another would make
> sense too.

I think support for migrating an object between extensions DOES exist,
it's just that you cannot use it from extension upgrade scripts.

Anyway pgsql-hackers is not the right place for discussion.
On pgsql-hackers we only want to find a future-proof way to "package
existing objects into an extension".  If the syntax
`CREATE EXTENSION  FROM UNPACKAGED` 
has gone, would it be ok for just:
`CREATE EXTENSION `
to intercept unpackaged objects and package them ?

--strk;

Re: pg_stat_progress_basebackup - progress reporting for pg_basebackup, in the server side

[Fujii Masao]

On 2020/02/18 21:31, Fujii Masao wrote:



On 2020/02/18 16:53, Amit Langote wrote:

On Tue, Feb 18, 2020 at 4:42 PM Fujii Masao  wrote:

On 2020/02/18 16:02, Amit Langote wrote:

I noticed that there is missing  tag in the documentation changes:


Could you tell me where I should add  tag?


+ 
+  waiting for checkpoint to finish
+  
+   The WAL sender process is currently performing
+   pg_start_backup to set up for
+   taking a base backup, and waiting for backup start
+   checkpoint to finish.
+  
+ 

There should be a  between  and  at the end of the
hunk shown above.


Will fix. Thanks!


Just to clarify, that's the missing  tag I am talking about above.


OK, so I added  tag just after the above .


+  
+   Whenever pg_basebackup is taking a base
+   backup, the pg_stat_progress_basebackup
+   view will contain a row for each WAL sender process that is currently
+   running BASE_BACKUP replication command
+   and streaming the backup.

I understand that you wrote "Whenever pg_basebackup is taking a
backup...", because description of other views contains a similar
starting line.  But, it may not only be pg_basebackup that would be
served by this view, no?  It could be any tool that speaks Postgres'
replication protocol and thus be able to send a BASE_BACKUP command.
If that is correct, I would write something like "When an application
is taking a backup" or some such without specific reference to
pg_basebackup.  Thoughts?


Yeah, there may be some such applications. But most users would
use pg_basebackup, so getting rid of the reference to pg_basebackup
would make the description a bit difficult-to-read. Also I can imagine
that an user of those backup applications would get to know
the progress reporting view from their documents. So I prefer
the existing one or something like "Whenever an application like
   pg_basebackup ...". Thought?


Sure, "an application like pg_basebackup" sounds fine to me.


OK, I changed the doc that way. Attached the updated version of the patch.


Attached is the updated version of the patch.

The previous patch used only pgstat_progress_update_param()
even when updating multiple values. Since those updates are
not atomic, this can cause readers of the values to see
the intermediate states. To avoid this issue, the latest patch
uses pgstat_progress_update_multi_param(), instead.

Regards,

--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 87586a7b06..dd4a668eea 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -376,6 +376,14 @@ postgres   27093  0.0  0.0  30096  2752 ?Ss   
11:34   0:00 postgres: ser
   
  
 
+ 
+  
pg_stat_progress_basebackuppg_stat_progress_basebackup
+  One row for each WAL sender process streaming a base backup,
+   showing current progress.
+   See .
+  
+ 
+
 

   
@@ -3535,7 +3543,10 @@ SELECT pg_stat_get_backend_pid(s.backendid) AS pid,
certain commands during command execution.  Currently, the only commands
which support progress reporting are ANALYZE,
CLUSTER,
-   CREATE INDEX, and VACUUM.
+   CREATE INDEX, VACUUM,
+   and  (i.e., replication
+   command that  issues to take
+   a base backup).
This may be expanded in the future.
   
 
@@ -4336,6 +4347,156 @@ SELECT pg_stat_get_backend_pid(s.backendid) AS pid,


   
+ 
+
+ 
+  Base Backup Progress Reporting
+
+  
+   Whenever an application like pg_basebackup
+   is taking a base backup, the
+   pg_stat_progress_basebackup
+   view will contain a row for each WAL sender process that is currently
+   running BASE_BACKUP replication command
+   and streaming the backup. The tables below describe the information
+   that will be reported and provide information about how to interpret it.
+  
+
+  
+   pg_stat_progress_basebackup View
+   
+
+
+  Column
+  Type
+  Description
+ 
+
+
+   
+
+ pid
+ integer
+ Process ID of a WAL sender process.
+
+
+ phase
+ text
+ Current processing phase. See .
+
+
+ backup_total
+ bigint
+ 
+  Total amount of data that will be streamed. If progress reporting
+  is not enabled in pg_basebackup
+  (i.e., --progress option is not specified),
+  this is 0. Otherwise, this is estimated and
+  reported as of the beginning of
+  streaming database files phase. Note that
+  this is only an approximation since the database
+  may change during streaming database files phase
+  and WAL log may be included in the backup later. This is always
+  the same value as backup_streamed
+  once the amount of data streamed exceeds the estimated
+  total size.
+ 
+
+
+ backup_streamed
+ bigint
+ 
+  Amount of data streamed. This counter only advances

Re: Commit fest manager for 2020-03

[Daniel Gustafsson]

> On 26 Feb 2020, at 14:39, David Steele  wrote:
> 
> On 2/26/20 3:15 AM, Michael Banck wrote:
>> On Wed, Feb 26, 2020 at 04:41:12PM +0900, Michael Paquier wrote:
>>> The next commit fets is going to begin in a couple of days, and there
>>> is a total of 207 patches registered as of today.  We don't have any
>>> manager yet, so is there any volunteer for taking the lead this time?
>> This is the last one for v13 I think? So probably somebody quite
>> experienced should handle this one. Or (if there is one already for
>> v13?) maybe even the Release Team themselves?
> 
> I'm happy to be CFM for this commitfest.

Thanks! 

> I'm not sure it would be a good use of resources to have the release team 
> performing CFM duties directly.  They will have enough on their plate

Absolutely, combining CFM and RM duties will make one of them (or both) suffer
a lack of attention.

cheers ./daniel

Re: [postgis-devel] About EXTENSION from UNPACKAGED on PostgreSQL 13

[Daniel Gustafsson]

> On 26 Feb 2020, at 15:13, Sandro Santilli  wrote:

> On pgsql-hackers we only want to find a future-proof way to "package
> existing objects into an extension".

What is the longterm goal of PostGIS, to use this as a stepping stone to reach
a point where no unpackaged extensions exist; or find a way to continue with
the current setup except with syntax that isn't going away?

> If the syntax
> `CREATE EXTENSION  FROM UNPACKAGED` 
> has gone, would it be ok for just:
> `CREATE EXTENSION `
> to intercept unpackaged objects and package them ?

Overloading the same syntax for creating packaged as well as unpackaged
extensions sounds like the wrong path to go down.

cheers ./daniel

Re: proposal: schema variables

[remi duval]

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

Hello Pavel

First thanks for working on this patch cause it might be really helpful for 
those of us trying to migrate PL code between RDBMs.

I tried your patch for migrating an Oracle package body to PL/pgSQL after also 
testing a solution using set_config and current_setting (which works but I'm 
not really satisfied by this workaround solution).

So I compiled latest postgres sources from github on Linux (redhat 7.7) using 
only your patch number 1 (I did not try the second part of the patch).

For my use-case it's working great, performances are excellent (compared to 
other solution for porting "package variables").
I did not test all the features involved by the patch (especially ALTER 
variable).

I have some feedback however :

1) Failure when using pg_dump 13 on a 12.1 database

When exporting a 12.1 database using pg_dump from the latest development 
sources I have an error regarding variables export 

[pg12@TST-LINUX-PG-03 ~]$ /opt/postgres12/pg12/bin/pg_dump -h localhost -p 5432 
-U postgres -f dump_pg12.sql database1
pg_dump: error: query failed: ERROR:  relation "pg_variable" does not exist
LINE 1: ...og.pg_get_expr(v.vardefexpr,0) as vardefexpr FROM pg_variabl...
 ^
pg_dump: error: query was: SELECT v.tableoid, v.oid, v.varname, v.vareoxaction, 
v.varnamespace, 
(SELECT rolname FROM pg_catalog.pg_roles WHERE oid = varowner) AS rolname
, (SELECT pg_catalog.array_agg(acl ORDER BY row_n) FROM (SELECT acl, row_n 
FROM pg_catalog.unnest(coalesce(v.varacl,pg_catalog.acldefault('V',v.varowner)))
 WITH ORDINALITY AS perm(acl,row_n) 
 WHERE NOT EXISTS ( SELECT 1 FROM 
pg_catalog.unnest(coalesce(pip.initprivs,pg_catalog.acldefault('V',v.varowner)))
 AS init(init_acl) 
WHERE acl = init_acl)) as foo) as varacl, ...:

I think that it should have worked anyway cause the documentation states :
https://www.postgresql.org/docs/current/upgrading.html
"It is recommended that you use the pg_dump and pg_dumpall programs from the 
newer version of PostgreSQL, to take advantage of enhancements that might have 
been made in these programs." (that's what I did here)

I think there should be a way to avoid dumping the variable if they don't 
exist, should'nt it ?

2) Displaying the variables + completion
I created 2 variables using :
CREATE VARIABLE my_pkg.g_dat_deb varchar(11);
CREATE VARIABLE my_pkg.g_dat_fin varchar(11);
When I try to display them, I can only see them when prefixing by the schema :
bdd13=> \dV
"Did not find any schema variables."
bdd13=> \dV my_pkg.*
   List of variables
   Schema   |  Name  | Type  | Is nullable | Default | 
Owner | Transactional end action
++---+-+-+---+--
 my_pkg| g_dat_deb   | character varying(11) | t   | | myowner  
 |
 my_pkg| g_dat_fin | character varying(11) | t   | | 
myowner   |
(3 rows)

bdd13=> \dV my_pkg
Did not find any schema variable named "my_pck".
NB : Using this template, functions are returned, maybe variables should also 
be listed ? (here by querying on "my_pkg%")
cts_get13=> \dV my_p [TAB]
=> completion using [TAB] key is not working

Is this normal that I cannot see all the variables when not specifying any 
schema ?
Also the completion works for functions, but not for variable.
That's just some bonus but it might be good to have it.

I think the way variables are listed using \dV should match with \df for 
querying functions

3) Any way to define CONSTANTs ?
We already talked a bit about this subject and also Gilles Darold introduces it 
in this mailing-list topic but I'd like to insist on it.
I think it would be nice to have a way to say that a variable should not be 
changed once defined.
Maybe it's hard to implement and can be implemented later, but I just want to 
know if this concern is open.


Otherwise the documentation looks good to me.

Regards

Rémi

Re: Resolving the python 2 -> python 3 mess

[Tom Lane]

Andrew Dunstan  writes:
> This seems to fix it.

OK, so we need that *and* the AddProject addition you mentioned?

regards, tom lane

Re: [postgis-devel] About EXTENSION from UNPACKAGED on PostgreSQL 13

[Sandro Santilli]

On Wed, Feb 26, 2020 at 03:35:46PM +0100, Daniel Gustafsson wrote:
> > On 26 Feb 2020, at 15:13, Sandro Santilli  wrote:
> 
> > On pgsql-hackers we only want to find a future-proof way to "package
> > existing objects into an extension".
> 
> What is the longterm goal of PostGIS, to use this as a stepping stone to reach
> a point where no unpackaged extensions exist; or find a way to continue with
> the current setup except with syntax that isn't going away?

No unpackaged extension seems like a good goal in the long term.

> Overloading the same syntax for creating packaged as well as unpackaged
> extensions sounds like the wrong path to go down.

So what other options would we have to let people upgrade a running
postgis or postgis_raster outside of the EXTENSION mechanism ?

--strk;

Re: Parallel copy

[Alastair Turner]

On Wed, 26 Feb 2020 at 10:54, Amit Kapila  wrote:
>
> On Tue, Feb 25, 2020 at 9:30 PM Tomas Vondra
>  wrote:
> >
...
> >
> > Perhaps. I guess it'll depend on the CSV file (number of fields, ...),
> > so I still think we need to do some measurements first.
> >
>
> Agreed.
>
> > I'm willing to
> > do that, but (a) I doubt I'll have time for that until after 2020-03,
> > and (b) it'd be good to agree on some set of typical CSV files.
> >
>
> Right, I don't know what is the best way to define that.  I can think
> of the below tests.
>
> 1. A table with 10 columns (with datatypes as integers, date, text).
> It has one index (unique/primary). Load with 1 million rows (basically
> the data should be probably 5-10 GB).
> 2. A table with 10 columns (with datatypes as integers, date, text).
> It has three indexes, one index can be (unique/primary). Load with 1
> million rows (basically the data should be probably 5-10 GB).
> 3. A table with 10 columns (with datatypes as integers, date, text).
> It has three indexes, one index can be (unique/primary). It has before
> and after trigeers. Load with 1 million rows (basically the data
> should be probably 5-10 GB).
> 4. A table with 10 columns (with datatypes as integers, date, text).
> It has five or six indexes, one index can be (unique/primary). Load
> with 1 million rows (basically the data should be probably 5-10 GB).
>
> Among all these tests, we can check how much time did we spend in
> reading, parsing the csv files vs. rest of execution?

That's a good set of tests of what happens after the parse. Two
simpler test runs may provide useful baselines - no
constraints/indexes with all columns varchar and no
constraints/indexes with columns correctly typed.

For testing the impact of various parts of the parse process, my idea would be:
 - A base dataset with 10 columns including int, date and text. One
text field quoted and containing both delimiters and line terminators
 - A derivative to measure just line parsing - strip the quotes around
the text field and quote the whole row as one text field
 - A derivative to measure the impact of quoted fields - clean up the
text field so it doesn't require quoting
 - A derivative to measure the impact of row length - run ten rows
together to make 100 column rows, but only a tenth as many rows

If that sounds reasonable, I'll try to knock up a generator.

--
Alastair

Re: Parallel copy

[Ants Aasma]

On Tue, 25 Feb 2020 at 18:00, Tomas Vondra  wrote:
> Perhaps. I guess it'll depend on the CSV file (number of fields, ...),
> so I still think we need to do some measurements first. I'm willing to
> do that, but (a) I doubt I'll have time for that until after 2020-03,
> and (b) it'd be good to agree on some set of typical CSV files.

I agree that getting a nice varied dataset would be nice. Including
things like narrow integer only tables, strings with newlines and
escapes in them, extremely wide rows.

I tried to capture a quick profile just to see what it looks like.
Grabbed a random open data set from the web, about 800MB of narrow
rows CSV [1].

Script:
CREATE TABLE census (year int,age int,ethnic int,sex int,area text,count text);
COPY census FROM '.../Data8277.csv' WITH (FORMAT 'csv', HEADER true);

Profile:
# Samples: 59K of event 'cycles:u'
# Event count (approx.): 57644269486
#
# Overhead  Command   Shared Object   Symbol
#     ..
...
#
18.24%  postgres  postgres[.] CopyReadLine
 9.23%  postgres  postgres[.] NextCopyFrom
 8.87%  postgres  postgres[.] NextCopyFromRawFields
 5.82%  postgres  postgres[.] pg_verify_mbstr_len
 5.45%  postgres  postgres[.] pg_strtoint32
 4.16%  postgres  postgres[.] heap_fill_tuple
 4.03%  postgres  postgres[.] heap_compute_data_size
 3.83%  postgres  postgres[.] CopyFrom
 3.78%  postgres  postgres[.] AllocSetAlloc
 3.53%  postgres  postgres[.] heap_form_tuple
 2.96%  postgres  postgres[.] InputFunctionCall
 2.89%  postgres  libc-2.30.so[.] __memmove_avx_unaligned_erms
 1.82%  postgres  libc-2.30.so[.] __strlen_avx2
 1.72%  postgres  postgres[.] AllocSetReset
 1.72%  postgres  postgres[.] RelationPutHeapTuple
 1.47%  postgres  postgres[.] heap_prepare_insert
 1.31%  postgres  postgres[.] heap_multi_insert
 1.25%  postgres  postgres[.] textin
 1.24%  postgres  postgres[.] int4in
 1.05%  postgres  postgres[.] tts_buffer_heap_clear
 0.85%  postgres  postgres[.] pg_any_to_server
 0.80%  postgres  postgres[.] pg_comp_crc32c_sse42
 0.77%  postgres  postgres[.] cstring_to_text_with_len
 0.69%  postgres  postgres[.] AllocSetFree
 0.60%  postgres  postgres[.] appendBinaryStringInfo
 0.55%  postgres  postgres[.] tts_buffer_heap_materialize.part.0
 0.54%  postgres  postgres[.] palloc
 0.54%  postgres  libc-2.30.so[.] __memmove_avx_unaligned
 0.51%  postgres  postgres[.] palloc0
 0.51%  postgres  postgres[.] pg_encoding_max_length
 0.48%  postgres  postgres[.] enlargeStringInfo
 0.47%  postgres  postgres[.] ExecStoreVirtualTuple
 0.45%  postgres  postgres[.] PageAddItemExtended

So that confirms that the parsing is a huge chunk of overhead with
current splitting into lines being the largest portion. Amdahl's law
says that splitting into tuples needs to be made fast before
parallelizing makes any sense.

Regards,
Ants Aasma

[1] 
https://www3.stats.govt.nz/2018census/Age-sex-by-ethnic-group-grouped-total-responses-census-usually-resident-population-counts-2006-2013-2018-Censuses-RC-TA-SA2-DHB.zip

Re: bad wal on replica / incorrect resource manager data checksum in record / zfs

[Alex Malek]

On Thu, Feb 20, 2020 at 12:01 PM Alex Malek  wrote:

> On Thu, Feb 20, 2020, 6:16 AM Amit Kapila  wrote:
>
>> On Thu, Feb 20, 2020 at 3:06 AM Alex Malek  wrote:
>> >
>> >
>> > Hello Postgres Hackers -
>> >
>> > We are having a reoccurring issue on 2 of our replicas where
>> replication stops due to this message:
>> > "incorrect resource manager data checksum in record at ..."
>> > This has been occurring on average once every 1 to 2 weeks during large
>> data imports (100s of GBs being written)
>> > on one of two replicas.
>> > Fixing the issue has been relatively straight forward: shutdown
>> replica, remove the bad wal file, restart replica and
>> > the good wal file is retrieved from the master.
>> > We are doing streaming replication using replication slots.
>> > However twice now, the master had already removed the WAL file so the
>> file had to retrieved from the wal archive.
>> >
>> > The WAL log directories on the master and the replicas are on ZFS file
>> systems.
>> > All servers are running RHEL 7.7 (Maipo)
>> > PostgreSQL 10.11
>> > ZFS v0.7.13-1
>> >
>> > The issue seems similar to
>> https://www.postgresql.org/message-id/CANQ55Tsoa6%3Dvk2YkeVUN7qO-2YdqJf_AMVQxqsVTYJm0qqQQuw%40mail.gmail.com
>> and to https://github.com/timescale/timescaledb/issues/1443
>> >
>> > One quirk in our ZFS setup is ZFS is not handling our RAID array, so
>> ZFS sees our array as a single device.
>> >
>> > Right before the issue started we did some upgrades and altered some
>> postgres configs and ZFS settings.
>> > We have been slowly rolling back changes but so far the the issue
>> continues.
>> >
>> > Some interesting data points while debugging:
>> > We had lowered the ZFS recordsize from 128K to 32K and for that week
>> the issue started happening every other day.
>> > Using xxd and diff we compared "good" and "bad" wal files and the
>> differences were not random bad bytes.
>> >
>> > The bad file either had a block of zeros that were not in the good file
>> at that position or other data.  Occasionally the bad data has contained
>> legible strings not in the good file at that position. At least one of
>> those exact strings has existed elsewhere in the files.
>> > However I am not sure if that is the case for all of them.
>> >
>> > This made me think that maybe there was an issue w/ wal file recycling
>> and ZFS under heavy load, so we tried lowering
>> > min_wal_size in order to "discourage" wal file recycling but my
>> understanding is a low value discourages recycling but it will still
>> > happen (unless setting wal_recycle in psql 12).
>> >
>>
>> We do print a message "recycled write-ahead log file .." in DEBUG2
>> mode.  You either want to run the server with DEBUG2 or maybe change
>> the code to make it LOG and see if that is printed.  If you do that,
>> you can verify if the corrupted WAL is the same as a recycled one.
>>
>
> Are you suggesting having the master, the replicas or all in debug mode?
> How much extra logging would this generate?
> A replica typically consumes over 1 TB of WAL files before a bad wal file
> is encountered.
>
>
>
>> > There is a third replica where this bug has not (yet?) surfaced.
>> > This leads me to guess the bad data does not originate on the master.
>> > This replica is older than the other replicas, slower CPUs, less RAM,
>> and the WAL disk array is spinning disks.
>> > The OS, version of Postgres, and version of ZFS are the same as the
>> other replicas.
>> > This replica is not using a replication slot.
>> > This replica does not serve users so load/contention is much lower than
>> the others.
>> > The other replicas often have 100% utilization of the disk array that
>> houses the (non-wal) data.
>> >
>> > Any insight into the source of this bug or how to address it?
>> >
>> > Since the master has a good copy of the WAL file, can the replica
>> re-request  the file from the master? Or from the archive?
>> >
>>
>> I think we do check in the archive if we get the error during
>> streaming, but archive might also have the same data due to which this
>> problem happens.  Have you checked that the archive WAL file, is it
>> different from the bad WAL?  See the
>
>
> Typically the master, the archive and the other replicas all have a good
> copy of the WAL file.
>
> relevant bits of code in
>> WaitForWALToBecomeAvailable especially the code near below comment:
>>
>> "Failure while streaming. Most likely, we got here because streaming
>> replication was terminated, or promotion was triggered. But we also
>> get here if we find an invalid record in the WAL streamed from master,
>> in which case something is seriously wrong. There's little chance that
>> the problem will just go away, but PANIC is not good for availability
>> either, especially in hot standby mode. So, we treat that the same as
>> disconnection, and retry from archive/pg_wal again. The WAL in the
>> archive should be identical to what was streamed, so it's unlikely
>> that it helps, but one can hope..."
>>
>>

Re: Commit fest manager for 2020-03

[Tomas Vondra]

On Wed, Feb 26, 2020 at 03:29:18PM +0100, Daniel Gustafsson wrote:

On 26 Feb 2020, at 14:39, David Steele  wrote:

On 2/26/20 3:15 AM, Michael Banck wrote:

On Wed, Feb 26, 2020 at 04:41:12PM +0900, Michael Paquier wrote:

The next commit fets is going to begin in a couple of days, and
there is a total of 207 patches registered as of today.  We don't
have any manager yet, so is there any volunteer for taking the lead
this time?

This is the last one for v13 I think? So probably somebody quite
experienced should handle this one. Or (if there is one already for
v13?) maybe even the Release Team themselves?


I'm happy to be CFM for this commitfest.


Thanks!


I'm not sure it would be a good use of resources to have the release
team performing CFM duties directly.  They will have enough on their
plate


Absolutely, combining CFM and RM duties will make one of them (or both)
suffer a lack of attention.



Did we actually decide who's going to be on RMT this year? I don't think
anyone particular was mentioned / proposed at the FOSDEM dev meeting. It
might be a good idea to decide that before the last CF too.

regards

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

Re: Commit fest manager for 2020-03

[Daniel Gustafsson]

> On 26 Feb 2020, at 16:29, Tomas Vondra  wrote:

> Did we actually decide who's going to be on RMT this year? I don't think
> anyone particular was mentioned / proposed at the FOSDEM dev meeting. It
> might be a good idea to decide that before the last CF too.

We didn't, we only discussed (based on feedback from previous RMTs) that having
some level of timezone overlap between the members makes for shorter roundtrips
in communication.

cheers ./daniel

Re: truncating timestamps on arbitrary intervals

[Tom Lane]

John Naylor  writes:
> [ v3-datetrunc_interval.patch ]

A few thoughts:

* In general, binning involves both an origin and a stride.  When
working with plain numbers it's almost always OK to set the origin
to zero, but it's less clear to me whether that's all right for
timestamps.  Do we need another optional argument?  Even if we
don't, "zero" for tm_year is 1900, which is going to give results
that surprise somebody.

* I'm still not convinced that the code does the right thing for
1-based months or days.  Shouldn't you need to subtract 1, then
do the modulus, then add back 1?

* Speaking of modulus, would it be clearer to express the
calculations like
timestamp -= timestamp % interval;
(That's just a question, I'm not sure.)

* Code doesn't look to have thought carefully about what to do with
negative intervals, or BC timestamps.

* The comment 
 * Justify all lower timestamp units and throw an error if any
 * of the lower interval units are non-zero.
doesn't seem to have a lot to do with what the code after it actually
does.  Also, you need explicit /* FALLTHRU */-type comments in that
switch, or pickier buildfarm members will complain.

* Seems like you could jam all the unit-related error checking into
that switch's default: case, where it will cost nothing if the
call is valid:

switch (unit)
{
...
default:
if (unit == 0)
// complain about zero interval
else
// complain about interval with multiple components
}

* I'd use ERRCODE_INVALID_PARAMETER_VALUE for any case of disallowed
contents of the interval.

regards, tom lane

Re: [postgis-devel] About EXTENSION from UNPACKAGED on PostgreSQL 13

[Stephen Frost]

Greetings,

* Sandro Santilli (s...@kbt.io) wrote:
> On pgsql-hackers we only want to find a future-proof way to "package
> existing objects into an extension".  If the syntax
> `CREATE EXTENSION  FROM UNPACKAGED` 
> has gone, would it be ok for just:
> `CREATE EXTENSION `
> to intercept unpackaged objects and package them ?

No.  The reason it was removed is because it's not going to be safe to
do when we have trusted extensions.  Perhaps it would be possible to
figure out a way to make it safe, but the reason FROM UNPACKAGED was
created and existed doesn't apply any more.  That PostGIS has been using
it for something else entirely is unfortunate, but the way to address
what PostGIS needs is to talk about that, not talk about how this ugly
hack used to work and doesn't any more.

Thanks,

Stephen


signature.asc
Description: PGP signature

Re: Commit fest manager for 2020-03

[Tom Lane]

David Steele  writes:
> On 2/26/20 3:15 AM, Michael Banck wrote:
>> This is the last one for v13 I think? So probably somebody quite
>> experienced should handle this one. Or (if there is one already for
>> v13?) maybe even the Release Team themselves?

> I'm not sure it would be a good use of resources to have the release 
> team performing CFM duties directly.  They will have enough on their 
> plate and in any case I don't think the team has been announced yet.

The Release Team hasn't been picked yet.  In past years I think
we've chosen them at the PGCon dev meeting.  So really, there's no
overlap --- the CF will be done before we need the RT.

Having said that, if you want to do it, that's great.

regards, tom lane

Re: [postgis-devel] About EXTENSION from UNPACKAGED on PostgreSQL 13

[Sandro Santilli]

On Wed, Feb 26, 2020 at 10:37:41AM -0500, Stephen Frost wrote:
> Greetings,
> 
> * Sandro Santilli (s...@kbt.io) wrote:
> > On pgsql-hackers we only want to find a future-proof way to "package
> > existing objects into an extension".  If the syntax
> > `CREATE EXTENSION  FROM UNPACKAGED` 
> > has gone, would it be ok for just:
> > `CREATE EXTENSION `
> > to intercept unpackaged objects and package them ?
> 
> No.  The reason it was removed is because it's not going to be safe to
> do when we have trusted extensions.

This part is not clear to me. You're _assuming_ that the unpackaged--xxx
will not make checks, so you _drop_ support for it ? Can't the normal
extension script also be unsafe for some reason ? Or can't the
unpackaged-xxx script be made safe by the publishers ? Or, as a last
resort.. can't you just mark postgis as UNSAFE and still require
superuser, which would give us the same experience as before ?


> Perhaps it would be possible to
> figure out a way to make it safe, but the reason FROM UNPACKAGED was
> created and existed doesn't apply any more.

Wasn't the reason of existance the ability for people to switch from
non-extension to extension based installs ?

>  That PostGIS has been using
> it for something else entirely is unfortunate, but the way to address
> what PostGIS needs is to talk about that, not talk about how this ugly
> hack used to work and doesn't any more.

Seriously, what was FROM UNPACKAGED meant to be used for ?

--strk;

Re: [postgis-devel] About EXTENSION from UNPACKAGED on PostgreSQL 13

[Paul Ramsey]

OK, well, what PostGIS needs is the ability for 'ALTER EXTENSION …. UPDATE foo’ 
to end up with two extensions in the end, ‘foo’ and ‘foo_new’. That’s what’s 
happening in the 2.x -> 3 upgrade process, as ‘postgis’ becomes ‘postgis’ and 
‘postgis_raster’. 

Presumably 15 years out from the 1.x -> 2.x we can stop worrying about bundling 
unpackaged postgis into an extension, and just recommend a hard upgrade 
dump/restore to the hardy souls still running 1.x.

P.

> On Feb 26, 2020, at 7:37 AM, Stephen Frost  wrote:
> 
> Greetings,
> 
> * Sandro Santilli (s...@kbt.io) wrote:
>> On pgsql-hackers we only want to find a future-proof way to "package
>> existing objects into an extension".  If the syntax
>> `CREATE EXTENSION  FROM UNPACKAGED` 
>> has gone, would it be ok for just:
>> `CREATE EXTENSION `
>> to intercept unpackaged objects and package them ?
> 
> No.  The reason it was removed is because it's not going to be safe to
> do when we have trusted extensions.  Perhaps it would be possible to
> figure out a way to make it safe, but the reason FROM UNPACKAGED was
> created and existed doesn't apply any more.  That PostGIS has been using
> it for something else entirely is unfortunate, but the way to address
> what PostGIS needs is to talk about that, not talk about how this ugly
> hack used to work and doesn't any more.
> 
> Thanks,
> 
> Stephen
> ___
> postgis-devel mailing list
> postgis-de...@lists.osgeo.org
> https://lists.osgeo.org/mailman/listinfo/postgis-devel

Re: [PATCH] pg_upgrade: report the reason for failing to open the cluster version file

[Tom Lane]

Michael Paquier  writes:
> On Wed, Feb 26, 2020 at 10:06:38AM +0100, Magnus Hagander wrote:
>> +1, seems like that would be a regression in value.

> Having more generic messages is less work for translators, we have
> PG_VERSION in the file name, and that's more complicated to translate
> in both French and Japanese.  No idea about other languages.

Just looking at the committed diff, it seems painfully obvious that these
two messages were written by different people who weren't talking to each
other.  Why aren't they more alike?  Given

   pg_fatal("could not open version file \"%s\": %m\n", ver_filename);

(which seems fine to me), I think the second ought to be

   pg_fatal("could not parse version file \"%s\"\n", ver_filename);

The wording as it stands:

   pg_fatal("could not parse PG_VERSION file from \"%s\"\n", 
cluster->pgdata);

could be criticized on more grounds than just that it's pointlessly
different from the adjacent message: it doesn't follow the style guideline
about saying what each mentioned object is.  You could fix that maybe with
s/from/from directory/, but I think this construction is unfortunate and
overly verbose already.

regards, tom lane

Re: [Proposal] Global temporary tables

[曾文旌(义从)]

> 2020年2月25日 下午11:31，tushar  写道：
> 
> Hi,
> 
> I have created two  global temporary tables like this -
> 
> Case 1- 
> postgres=# create global  temp table foo(n int) with 
> (on_commit_delete_rows='true');
> CREATE TABLE
> 
> Case 2- 
> postgres=# create global  temp table bar1(n int) on commit delete rows;
> CREATE TABLE
> 
> 
> but   if i try to do the same having only 'temp' keyword , Case 2 is working 
> fine but getting this error  for case 1 -
> 
> postgres=# create   temp table foo1(n int) with 
> (on_commit_delete_rows='true');
> ERROR:  regular table cannot specifie on_commit_delete_rows
> postgres=# 
> 
> postgres=#  create   temp table bar1(n int) on commit delete rows;
> CREATE TABLE
> 
> i think this error message need to be more clear .
Also fixed in global_temporary_table_v14-pg13.patch

Wenjing



> 
> regards,
> tushar 
> 
> On 2/25/20 7:19 PM, Pavel Stehule wrote/:
>> 
>> 
>> út 25. 2. 2020 v 14:36 odesílatel Prabhat Sahu 
>> mailto:prabhat.s...@enterprisedb.com>> 
>> napsal:
>> Hi All,
>> 
>> Please check the below findings on GTT.
>> -- Scenario 1:
>> Under "information_schema", We are not allowed to create "temporary table", 
>> whereas we can CREATE/DROP "Global Temporary Table", is it expected ?
>> 
>> It is ok for me. temporary tables should be created only in proprietary 
>> schema. For GTT there is not risk of collision, so it can be created in any 
>> schema where are necessary access rights.
>> 
>> Pavel
>> 
>> 
>> postgres=# create temporary table information_schema.temp1(c1 int);
>> ERROR:  cannot create temporary relation in non-temporary schema
>> LINE 1: create temporary table information_schema.temp1(c1 int);
>>^
>> 
>> postgres=# create global temporary table information_schema.temp1(c1 int);
>> CREATE TABLE
>> 
>> postgres=# drop table information_schema.temp1 ;
>> DROP TABLE
>> 
>> -- Scenario 2:
>> Here I am getting the same error message in both the below cases.
>> We may add a "global" keyword with GTT related error message.
>> 
>> postgres=# create global temporary table gtt1 (c1 int unique);
>> CREATE TABLE
>> postgres=# create temporary table tmp1 (c1 int unique);
>> CREATE TABLE
>> 
>> postgres=# create temporary table tmp2 (c1 int references gtt1(c1) );
>> ERROR:  constraints on temporary tables may reference only temporary tables
>> 
>> postgres=# create global temporary table gtt2 (c1 int references tmp1(c1) );
>> ERROR:  constraints on temporary tables may reference only temporary tables
>> 
>> Thanks,
>> Prabhat Sahu
>> 
>> On Tue, Feb 25, 2020 at 2:25 PM 曾文旌(义从) > > wrote:
>> 
>> 
>>> 2020年2月24日 下午5:44，Prabhat Sahu >> > 写道：
>>> 
>>> On Fri, Feb 21, 2020 at 9:10 PM 曾文旌(义从) >> > wrote:
>>> Hi,
>>> I have started testing the "Global temporary table" feature,
>>> That's great, I see hope.
>>> from "gtt_v11-pg13.patch". Below is my findings:
>>> 
>>> -- session 1:
>>> postgres=# create global temporary table gtt1(a int);
>>> CREATE TABLE
>>> 
>>> -- seeeion 2:
>>> postgres=# truncate gtt1 ;
>>> ERROR:  could not open file "base/13585/t3_16384": No such file or directory
>>> 
>>> is it expected?
>>> 
>>> Oh ,this is a bug, I fixed it.
>>> Thanks for the patch.
>>> I have verified the same, Now the issue is resolved with v12 patch.
>>> 
>>> Kindly confirm the below scenario:
>>> 
>>> postgres=# create global temporary table gtt1 (c1 int unique);
>>> CREATE TABLE
>>> 
>>> postgres=# create global temporary table gtt2 (c1 int references gtt1(c1) );
>>> ERROR:  referenced relation "gtt1" is not a global temp table
>>> 
>>> postgres=# create table tab2 (c1 int references gtt1(c1) );
>>> ERROR:  referenced relation "gtt1" is not a global temp table
>>> 
>>> Thanks, 
>>> Prabhat Sahu
>> 
>> GTT supports foreign key constraints in global_temporary_table_v13-pg13.patch
>> 
>> 
>> Wenjing
>> 
>> 
>> 
>> 
>> 
>> -- 
>> With Regards,
>> Prabhat Kumar Sahu
>> EnterpriseDB: http://www.enterprisedb.com 
> 
> -- 
> regards,tushar
> EnterpriseDB  https://www.enterprisedb.com/ 
> The Enterprise PostgreSQL Company

Re: [postgis-devel] About EXTENSION from UNPACKAGED on PostgreSQL 13

[Chapman Flack]

On 2/26/20 10:52 AM, Sandro Santilli wrote:

> This part is not clear to me. You're _assuming_ that the unpackaged--xxx
> will not make checks, so you _drop_ support for it ? Can't the normal
> extension script also be unsafe for some reason ? Or can't the
> unpackaged-xxx script be made safe by the publishers ? Or, as a last
> resort.. can't you just mark postgis as UNSAFE and still require
> superuser, which would give us the same experience as before ?

I am wondering: does anything in the PG 13 change preclude writing
a postgis_raster--unpackaged.sql script that could be applied with
CREATE EXTENSION postgis_raster VERSION unpackaged;
and would do perhaps nothing at all, or merely confirm that the
right unpackaged things are present and are the right things...

... from which an ALTER EXTENSION postgis_raster UPDATE TO 3.0;
would naturally run the existing postgis_raster--unpackaged--3.0.sql
and execute all of its existing ALTER EXTENSION ... ADD operations?

Has the disadvantage of being goofy, but possibly the advantage of
being implementable in the current state of affairs.

Regards,
-Chap

Re: [Patch] Make pg_checksums skip foreign tablespace directories

[Bernd Helmle]

Am Dienstag, den 25.02.2020, 11:33 +0900 schrieb Michael Paquier:
> I really think that
> we should avoid duplicating the same logic around, and that we should
> remain consistent with non-directory entries in those paths,
> complaining with a proper failure if extra, unwanted files are
> present.

Okay, please find an updated patch attached.

My feeling is that in the case we cannot successfully resolve a
tablespace location from pg_tblspc, we should error out, but i could
imagine that people would like to have just a warning instead.

I've updated the TAP test for pg_checksums by adding a dummy
subdirectory into the tablespace directory already created for the
corrupted relfilenode test, containing a file to process in case an
unpatched pg_checksums is run. With the patch attached, these
directories simply won't be considered to check.

Thanks,

Bernd
From 15bd3a6457f59f3ba4bf72c645499132e90127cd Mon Sep 17 00:00:00 2001
From: Bernd Helmle 
Date: Wed, 26 Feb 2020 16:49:03 +0100
Subject: [PATCH 1/2] Formerly pg_checksums recursively dived into pg_tblspc,
 scanning any tablespace location found there without taking into account,
 that tablespace locations might be used by other PostgreSQL instances at the
 same time. This could likely happen for example by using pg_upgrade to
 upgrade clusters with tablespaces.

Fix this by resolving tablespace locations explicitly by looking
up their TABLESPACE_VERSION_DIRECTORY subdirectory if called on
the pg_tblspc directory, and, if found, recursively diving into
them directly. If we can't resolve the tablespace location link, we
throw an error to indicate a probably orphaned or missing tablespace
location.
---
 src/bin/pg_checksums/pg_checksums.c | 43 -
 1 file changed, 42 insertions(+), 1 deletion(-)

diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c
index 9bd0bf947f..1367f53bbf 100644
--- a/src/bin/pg_checksums/pg_checksums.c
+++ b/src/bin/pg_checksums/pg_checksums.c
@@ -385,7 +385,48 @@ scan_directory(const char *basedir, const char *subdir, bool sizeonly)
 #else
 		else if (S_ISDIR(st.st_mode) || pgwin32_is_junction(fn))
 #endif
-			dirsize += scan_directory(path, de->d_name, sizeonly);
+		{
+			/*
+			 * If we are called with subdir pg_tblspc, we assume to
+			 * operate on tablespace locations. We're just interested
+			 * in TABLESPACE_VERSION_DIRECTORY only, so we're resolving
+			 * the linked locations here explicitely and dive into them
+			 * directly.
+			 */
+			if (strncmp("pg_tblspc", subdir, strlen("pg_tblspc")) == 0)
+			{
+chartblspc_path[MAXPGPATH];
+struct stat tblspc_st;
+
+/*
+ * Resolve tablespace location path and check
+ * whether a TABLESPACE_VERSION_DIRECTORY exists. Not finding
+ * a valid location is an unexpected condition, since there
+ * shouldn't be orphaned links. Tell the user something
+ * is wrong.
+ */
+snprintf(tblspc_path, sizeof(tblspc_path), "%s/%s/%s",
+		 path, de->d_name, TABLESPACE_VERSION_DIRECTORY);
+
+if (lstat(tblspc_path, &tblspc_st) < 0)
+{
+	pg_log_error("invalid tablespace location \"%s/%s\"",
+ subdir, de->d_name);
+	exit(1);
+}
+
+snprintf(tblspc_path, sizeof(tblspc_path), "%s/%s",
+		 path, de->d_name);
+
+/* Looks like a valid tablespace location */
+dirsize += scan_directory(tblspc_path, TABLESPACE_VERSION_DIRECTORY,
+		  sizeonly);
+			}
+			else
+			{
+dirsize += scan_directory(path, de->d_name, sizeonly);
+			}
+		}
 	}
 	closedir(dir);
 	return dirsize;
-- 
2.24.1

From 77393902b2b9f92c51ad525eacde5ab87e55faac Mon Sep 17 00:00:00 2001
From: Bernd Helmle 
Date: Wed, 26 Feb 2020 17:48:11 +0100
Subject: [PATCH 2/2] Update TAP tests for pg_checksums.

Add an additional test with a dummy foreign tablespace
location to check the new behavior of pg_checksums to properly
ignore any other versioned tablespace subdirectory from e.g.
pg_upgrade'd clusters.
---
 src/bin/pg_checksums/t/002_actions.pl | 10 +-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/src/bin/pg_checksums/t/002_actions.pl b/src/bin/pg_checksums/t/002_actions.pl
index 83a730ea94..88e186d873 100644
--- a/src/bin/pg_checksums/t/002_actions.pl
+++ b/src/bin/pg_checksums/t/002_actions.pl
@@ -5,7 +5,7 @@ use strict;
 use warnings;
 use PostgresNode;
 use TestLib;
-use Test::More tests => 62;
+use Test::More tests => 63;
 
 
 # Utility routine to create and check a table with corrupted checksums
@@ -217,6 +217,14 @@ sub fail_corrupt
 # Stop instance for the follow-up checks.
 $node->stop;
 
+# Create fake directories in the tablespace location, claiming
+# to be foreign objects we shouldn't touch.
+mkdir "$tablespace_dir/PG_99_1/";
+append_to_file "$tablespace_dir/PG_99_1/foo", "123";
+# Should not fail
+command_ok([ 'pg_checksums', '--check', '-D', $pgdata ],
+	"succeeds with foreign tablespace");
+
 # Authorized relation files filled wi

Re: truncating timestamps on arbitrary intervals

[David Fetter]

On Wed, Feb 26, 2020 at 06:38:57PM +0800, John Naylor wrote:
> On Wed, Feb 26, 2020 at 3:51 PM David Fetter  wrote:
> >
> > I believe the following should error out, but doesn't.
> >
> > # SELECT date_trunc_interval('1 year 1 ms', TIMESTAMP '2001-02-16 
> > 20:38:40');
> >  date_trunc_interval
> > ═
> >  2001-01-01 00:00:00
> > (1 row)
> 
> You're quite right. I forgot to add error checking for
> second-and-below units. I've added your example to the tests. (I
> neglected to mention in my first email that because I chose to convert
> the interval to the pg_tm struct (seemed easiest), it's not
> straightforward how to allow multiple unit types, and I imagine the
> use case is small, so I had it throw an error.)

I suspect that this could be sanely expanded to span some sets of
adjacent types in a future patch, e.g. year + month or hour + minute.

> > Please find attached an update that I believe fixes the bug I found in
> > a principled way.
> 
> Thanks for that! I made a couple adjustments and incorporated your fix
> into v3: While working on v1, I noticed the DTK_FOO macros already had
> an idiom for bitmasking (see utils/datetime.h),

Oops!  Sorry I missed that.

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

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

Allow auto_explain to log plans before queries are executed

[Yugo NAGATA]

Hi,

Attached is a patch for allowing auto_explain to log plans before
queries are executed.

Currently, auto_explain logs plans only after query executions,
so if a query gets stuck its plan could not be logged. If we can
know plans of stuck queries, we may get some hints to resolve the
stuck. This is useful when you are testing and debugging your
application whose queries get stuck in some situations.

This patch adds  new option log_before_query to auto_explain.
Setting auto_explain.log_before_query option logs all plans before
queries are executed regardless of auto_explain.log_min_duration
unless this is set -1 to disable logging.  If log_before_query is
enabled, only duration time is logged after query execution as in
the case of when both log_statement and log_min_duration_statement
are enabled.

Regards,
Yugo Nagata

-- 
Yugo NAGATA 
>From 02f3bd5763fb0719827112afff809267fd778989 Mon Sep 17 00:00:00 2001
From: Yugo Nagata 
Date: Fri, 14 Feb 2020 16:32:32 +0900
Subject: [PATCH] Allow to log plans before queries are executed

Previously, auto_explain logs plans only after query executions,
so if a query gets stuck its plan could not be logged. Setting
auto_explain.log_before_query option logs all plans before queries
are executed regardless of auto_explain.log_min_duration unless
this is set -1 to disable logging.
---
 contrib/auto_explain/auto_explain.c | 123 
 doc/src/sgml/auto-explain.sgml  |  21 +
 2 files changed, 109 insertions(+), 35 deletions(-)

diff --git a/contrib/auto_explain/auto_explain.c b/contrib/auto_explain/auto_explain.c
index f69dde876c..1c90ed4469 100644
--- a/contrib/auto_explain/auto_explain.c
+++ b/contrib/auto_explain/auto_explain.c
@@ -34,6 +34,7 @@ static int	auto_explain_log_format = EXPLAIN_FORMAT_TEXT;
 static int	auto_explain_log_level = LOG;
 static bool auto_explain_log_nested_statements = false;
 static double auto_explain_sample_rate = 1;
+static bool auto_explain_log_before_query = false;
 
 static const struct config_enum_entry format_options[] = {
 	{"text", EXPLAIN_FORMAT_TEXT, false},
@@ -63,6 +64,9 @@ static int	nesting_level = 0;
 /* Is the current top-level query to be sampled? */
 static bool current_query_sampled = false;
 
+/* Was the plan logged before query execution? */
+static bool was_logged = false;
+
 #define auto_explain_enabled() \
 	(auto_explain_log_min_duration >= 0 && \
 	 (nesting_level == 0 || auto_explain_log_nested_statements) && \
@@ -84,6 +88,7 @@ static void explain_ExecutorRun(QueryDesc *queryDesc,
 static void explain_ExecutorFinish(QueryDesc *queryDesc);
 static void explain_ExecutorEnd(QueryDesc *queryDesc);
 
+static ExplainState *do_explain(QueryDesc *queryDesc);
 
 /*
  * Module load callback
@@ -218,6 +223,17 @@ _PG_init(void)
 			 NULL,
 			 NULL);
 
+	DefineCustomBoolVariable("auto_explain.log_before_query",
+			 "Log before query is executed.",
+			 NULL,
+			 &auto_explain_log_before_query,
+			 false,
+			 PGC_SUSET,
+			 0,
+			 NULL,
+			 NULL,
+			 NULL);
+
 	EmitWarningsOnPlaceholders("auto_explain");
 
 	/* Install hooks. */
@@ -288,6 +304,7 @@ explain_ExecutorStart(QueryDesc *queryDesc, int eflags)
 	else
 		standard_ExecutorStart(queryDesc, eflags);
 
+	was_logged = false;
 	if (auto_explain_enabled())
 	{
 		/*
@@ -303,6 +320,25 @@ explain_ExecutorStart(QueryDesc *queryDesc, int eflags)
 			queryDesc->totaltime = InstrAlloc(1, INSTRUMENT_ALL);
 			MemoryContextSwitchTo(oldcxt);
 		}
+
+		/* Log plan before the query is executed */
+		if (!auto_explain_log_analyze && auto_explain_log_before_query)
+		{
+			ExplainState *es = do_explain(queryDesc);
+
+			/*
+			 * Note: we rely on the existing logging of context or
+			 * debug_query_string to identify just which statement is being
+			 * reported.  This isn't ideal but trying to do it here would
+			 * often result in duplication.
+			 */
+			ereport(auto_explain_log_level,
+	(errmsg("plan:\n%s", es->str->data),
+	 errhidestmt(true)));
+			pfree(es->str->data);
+
+			was_logged = true;
+		}
 	}
 }
 
@@ -369,48 +405,26 @@ explain_ExecutorEnd(QueryDesc *queryDesc)
 		msec = queryDesc->totaltime->total * 1000.0;
 		if (msec >= auto_explain_log_min_duration)
 		{
-			ExplainState *es = NewExplainState();
-
-			es->analyze = (queryDesc->instrument_options && auto_explain_log_analyze);
-			es->verbose = auto_explain_log_verbose;
-			es->buffers = (es->analyze && auto_explain_log_buffers);
-			es->timing = (es->analyze && auto_explain_log_timing);
-			es->summary = es->analyze;
-			es->format = auto_explain_log_format;
-			es->settings = auto_explain_log_settings;
-
-			ExplainBeginOutput(es);
-			ExplainQueryText(es, queryDesc);
-			ExplainPrintPlan(es, queryDesc);
-			if (es->analyze && auto_explain_log_triggers)
-ExplainPrintTriggers(es, queryDesc);
-			if (es->costs)
-ExplainPrintJITSummary(es, queryDesc);
-			ExplainEndOutput(es);
-
-			/* Remove last line break */
-			if (

Re: Allow auto_explain to log plans before queries are executed

[Julien Rouhaud]

On Thu, Feb 27, 2020 at 02:35:18AM +0900, Yugo NAGATA wrote:
> Hi,
>
> Attached is a patch for allowing auto_explain to log plans before
> queries are executed.
>
> Currently, auto_explain logs plans only after query executions,
> so if a query gets stuck its plan could not be logged. If we can
> know plans of stuck queries, we may get some hints to resolve the
> stuck. This is useful when you are testing and debugging your
> application whose queries get stuck in some situations.

Indeed that could be useful.

> This patch adds  new option log_before_query to auto_explain.

Maybe "log_before_execution" would be better?

> Setting auto_explain.log_before_query option logs all plans before
> queries are executed regardless of auto_explain.log_min_duration
> unless this is set -1 to disable logging.  If log_before_query is
> enabled, only duration time is logged after query execution as in
> the case of when both log_statement and log_min_duration_statement
> are enabled.

I'm not sure about this behavior.  The final explain plan is needed at least if
log_analyze, log_buffers or log_timing are enabled.

Re: Error on failed COMMIT

[Vladimir Sitnikov]

Just one more data point: drivers do allow users to execute queries in a
free form.
Shat is the user might execute /*comment*/commit/*comment*/ as a free-form
SQL, and they would expect that the resulting
 behaviour should be exactly the same as .commit() API call (==silent
rollback is converted to an exception).

That is drivers can add extra logic into .commit() API implementation,
however, turning free-form SQL into exceptions
is hard to do consistently from the driver side.
It is not like "check the response from .commit() result".
It is more like "don't forget to parse user-provided SQL and verify if it
is semantically equivalent to commit"

Pushing full SQL parser to the driver is not the best idea taking into the
account the extensibility the core has.

Vladimir

Re: [PATCH] pg_upgrade: report the reason for failing to open the cluster version file

[Dagfinn Ilmari Mannsåker]

Tom Lane  writes:

> Michael Paquier  writes:
>> On Wed, Feb 26, 2020 at 10:06:38AM +0100, Magnus Hagander wrote:
>>> +1, seems like that would be a regression in value.
>
>> Having more generic messages is less work for translators, we have
>> PG_VERSION in the file name, and that's more complicated to translate
>> in both French and Japanese.  No idea about other languages.
>
> Just looking at the committed diff, it seems painfully obvious that these
> two messages were written by different people who weren't talking to each
> other.  Why aren't they more alike?  Given
>
>pg_fatal("could not open version file \"%s\": %m\n", ver_filename);
>
> (which seems fine to me), I think the second ought to be
>
>pg_fatal("could not parse version file \"%s\"\n", ver_filename);

Good point.  Patch attached.

- ilmari
-- 
- Twitter seems more influential [than blogs] in the 'gets reported in
  the mainstream press' sense at least.   - Matt McLeod
- That'd be because the content of a tweet is easier to condense down
  to a mainstream media article.  - Calle Dybedahl

>From a761148357420fdf81589e28df701cc1fde4cb87 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= 
Date: Wed, 26 Feb 2020 18:29:03 +
Subject: [PATCH] Make pg_upgrade's version file parsing error message
 consistent

---
 src/bin/pg_upgrade/server.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/bin/pg_upgrade/server.c b/src/bin/pg_upgrade/server.c
index be604d3351..f669bb4e8a 100644
--- a/src/bin/pg_upgrade/server.c
+++ b/src/bin/pg_upgrade/server.c
@@ -168,7 +168,7 @@ get_major_server_version(ClusterInfo *cluster)
 
 	if (fscanf(version_fd, "%63s", cluster->major_version_str) == 0 ||
 		sscanf(cluster->major_version_str, "%d.%d", &v1, &v2) < 1)
-		pg_fatal("could not parse PG_VERSION file from \"%s\"\n", cluster->pgdata);
+		pg_fatal("could not parse version file \"%s\"\n", ver_filename);
 
 	fclose(version_fd);
 
-- 
2.20.1

Re: Error on failed COMMIT

[Haumacher, Bernhard]

Am 24.02.2020 um 13:34 schrieb Robert Haas:

As I said upthread, I think one of the things that would be pretty
badly broken by this is psql -f something.sql, where something.sql
contains a series of blocks of the form "begin; something; something;
something; commit;". Right now whichever transactions succeed get
committed. With the proposed change, if one transaction block fails,
it'll merge with all of the following blocks.



No, that's *not* true.

The only difference with the proposed change would be another error in 
the logs for the commit following the block with the failed insert. 
Note: Nobody has suggested that the commit that returns with an error 
should not end the transaction. Do just the same as with any other 
commit error in response to a constraint violation!



Am 24.02.2020 um 18:53 schrieb David Fetter:

On Mon, Feb 24, 2020 at 06:40:16PM +0100, Vik Fearing wrote:

On 24/02/2020 18:37, David Fetter wrote:

If we'd done this from a clean sheet of paper, it would have been the
right decision. We're not there, and haven't been for decades.

OTOH, it's never too late to do the right thing.

Some right things take a lot of prep work in order to actually be
right things. This is one of them.  Defaulting to SERIALIZABLE
isolation is another.



Here the proposed changes is really much much less noticable - please 
report the error (again) instead of giving an incomprehensible status 
code. Nothing else must be changed - the failing commit should do the 
rollback and end the transaction - but it should report this situation 
as an error!


Regards Bernhard

Re: Error on failed COMMIT

[Vik Fearing]

On 25/02/2020 12:11, Laurenz Albe wrote:
> On Tue, 2020-02-25 at 13:25 +0530, Robert Haas wrote:
>> On Tue, Feb 25, 2020 at 12:47 PM Vladimir Sitnikov
>>  wrote:
>>> Noone suggested that "commit leaves the session in a transaction state".
>>> Of course, every commit should terminate the transaction.
>>> However, if a commit fails (for any reason), it should produce the relevant 
>>> ERROR that explains what went wrong rather than silently doing a rollback.
>>
>> OK, I guess I misinterpreted the proposal. That would be much less
>> problematic -- any driver or application that can't handle ERROR in
>> response to an attempted COMMIT would be broken already.
> 
> I agree with that.
> 
> There is always some chance that someone relies on COMMIT not
> throwing an error when it rolls back, but I think that throwing an
> error is actually less astonishing than *not* throwing one.
> 
> So, +1 for the proposal from me.

I started this thread for some discussion and hopefully a documentation
patch.  But now I have moved firmly into the +1 camp.  COMMIT should
error if it can't commit, and then terminate the (aborted) transaction.
-- 
Vik Fearing

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

[Alexander Korotkov]

On Tue, Feb 25, 2020 at 1:48 PM Alexander Korotkov
 wrote:
>
> I think usage of chmod() deserves comment.  As I get default
> permissions are sufficient for work, but we need to set them to
> satisfy 'check PGDATA permissions' test.


I've added this comment myself.  I've also fixes some indentation.
Patch now looks good to me.  I'm going to push it if no objections.

--
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company


v16-0001-pg_rewind-Add-options-to-restore-WAL-files.patch
Description: Binary data

Re: Commit fest manager for 2020-03

[Tomas Vondra]

On Wed, Feb 26, 2020 at 10:45:16AM -0500, Tom Lane wrote:

David Steele  writes:

On 2/26/20 3:15 AM, Michael Banck wrote:

This is the last one for v13 I think? So probably somebody quite
experienced should handle this one. Or (if there is one already for
v13?) maybe even the Release Team themselves?



I'm not sure it would be a good use of resources to have the release
team performing CFM duties directly.  They will have enough on their
plate and in any case I don't think the team has been announced yet.


The Release Team hasn't been picked yet.  In past years I think
we've chosen them at the PGCon dev meeting.  So really, there's no
overlap --- the CF will be done before we need the RT.



Nope, the RMT for PG12 was announced on 2019/03/30 [1], i.e. shortly
before the end of the last CF (and before pgcon). I think there was some
discussion about the members at/after the FOSDEM dev meeting. The
overlap with CFM duties is still fairly minimal, and there's not much
for RMT to do before the end of the last CF anyway ...

[1] https://www.postgresql.org/message-id/20190330094043.ga28...@paquier.xyz

Maybe we shouldn't wait with assembling RMT until pgcon, though.


regards

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

Re: Resolving the python 2 -> python 3 mess

[Andrew Dunstan]

On Thu, Feb 27, 2020 at 1:33 AM Tom Lane  wrote:
>
> Andrew Dunstan  writes:
> > This seems to fix it.
>
> OK, so we need that *and* the AddProject addition you mentioned?
>
>

Yes, the first one builds it, the second one fixes the tests to run correctly.

cheers

andrew

-- 
Andrew Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

RE: [postgis-devel] About EXTENSION from UNPACKAGED on PostgreSQL 13

[Regina Obe]

> Presumably 15 years out from the 1.x -> 2.x we can stop worrying about
> bundling unpackaged postgis into an extension, and just recommend a hard
> upgrade dump/restore to the hardy souls still running 1.x.
> 
> P.
> 

We don't need to worry about 1.x cause 1.x can only do a hard upgrade to 2 or 
3.  We never supported soft upgrade from 1.x
Easy solution there is just to install postgis extension and do 
pg_restore/postgis_restore of your data.

So it's really just the 2.1 -> 3 that are of concern.
I think now is a fine time to encourage everyone to upgrade to 3 if they can so 
they don't need to suffer any crazy solutions we come up with  :)

Turn this into a convenient emergency.

Thanks,
Regina

Re: proposal: schema variables

[Pavel Stehule]

er, but I just want
> to know if this concern is open.
>

This topic is open. I tried to play with it. The problem is syntax. When I
try to reproduce syntax from PLpgSQL, then I need to introduce new reserved
keyword. So my initial idea was wrong.
We need to open discussion about implementable syntax. For this moment you
can use a workaround - any schema variable without WRITE right is constant.
Implementation is easy. Design of syntax is harder.

please see attached patch

Regards

Pavel


>
> Otherwise the documentation looks good to me.
>
> Regards
>
> Rémi


schema-variables-20200226.patch.gz
Description: application/gzip

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

[Alexey Kondratov]

On 2020-02-26 22:03, Alexander Korotkov wrote:

On Tue, Feb 25, 2020 at 1:48 PM Alexander Korotkov
 wrote:


I think usage of chmod() deserves comment.  As I get default
permissions are sufficient for work, but we need to set them to
satisfy 'check PGDATA permissions' test.



I've added this comment myself.



Thanks for doing it yourself, I was going to answer tonight, but it 
would be obviously too late.




I've also fixes some indentation.
Patch now looks good to me.  I'm going to push it if no objections.



I think that docs should be corrected. Previously Michael was against 
the phrase 'restore_command defined in the postgresql.conf', since it 
also could be defined in any config file included there. We corrected it 
in the pg_rewind --help output, but now docs say:


+Use the restore_command defined in
+postgresql.conf to retrieve WAL files from
+the WAL archive if these files are no longer available in the
+pg_wal directory of the target cluster.

Probably it should be something like:

+Use the restore_command defined in
+the target cluster configuration to retrieve WAL files from
+the WAL archive if these files are no longer available in the
+pg_wal directory.

Here the only text split changed:

-* Ignore restore_command when not in archive recovery (meaning
-* we are in crash recovery).
+	 * Ignore restore_command when not in archive recovery (meaning we are 
in

+* crash recovery).

Should we do so in this patch?

I think that this extra dot at the end is not necessary here:

+		pg_log_debug("using config variable restore_command=\'%s\'.", 
restore_command);


If you agree then attached is a patch with all the corrections above. It 
is made with default git format-patch format, but yours were in a 
slightly different format, so I only was able to apply them with git am 
--patch-format=stgit.



--
Alexey Kondratov

Postgres Professional https://www.postgrespro.com
The Russian Postgres Company

From fa2fc359dd9852afc608663fa32733e800652ffa Mon Sep 17 00:00:00 2001
From: Alexander Korotkov 
Date: Tue, 25 Feb 2020 02:22:45 +0300
Subject: [PATCH v17] pg_rewind: Add options to restore WAL files from archive

Currently, pg_rewind fails when it could not find required WAL files in the
target data directory.  One have to manually figure out which WAL files are
required and copy them back from archive.

This commit implements new pg_rewind options, which allow pg_rewind to
automatically retrieve missing WAL files from archival storage. The
restore_command option is read from postgresql.conf.

Discussion: https://postgr.es/m/a3acff50-5a0d-9a2c-b3b2-ee36168955c1%40postgrespro.ru
Author: Alexey Kondratov
Reviewed-by: Michael Paquier, Andrey Borodin, Alvaro Herrera
Reviewed-by: Andres Freund, Alexander Korotkov
---
 doc/src/sgml/ref/pg_rewind.sgml  |  28 --
 src/backend/access/transam/xlogarchive.c |  58 +
 src/bin/pg_rewind/parsexlog.c|  33 ++-
 src/bin/pg_rewind/pg_rewind.c|  77 ++--
 src/bin/pg_rewind/pg_rewind.h|   6 +-
 src/bin/pg_rewind/t/001_basic.pl |   3 +-
 src/bin/pg_rewind/t/RewindTest.pm|  67 +-
 src/common/Makefile  |   2 +
 src/common/archive.c |  97 +
 src/common/fe_archive.c  | 106 +++
 src/include/common/archive.h |  21 +
 src/include/common/fe_archive.h  |  18 
 src/tools/msvc/Mkvcbuild.pm  |   8 +-
 13 files changed, 443 insertions(+), 81 deletions(-)
 create mode 100644 src/common/archive.c
 create mode 100644 src/common/fe_archive.c
 create mode 100644 src/include/common/archive.h
 create mode 100644 src/include/common/fe_archive.h

diff --git a/doc/src/sgml/ref/pg_rewind.sgml b/doc/src/sgml/ref/pg_rewind.sgml
index 42d29edd4e..64a6942031 100644
--- a/doc/src/sgml/ref/pg_rewind.sgml
+++ b/doc/src/sgml/ref/pg_rewind.sgml
@@ -66,11 +66,11 @@ PostgreSQL documentation
can be found either on the target timeline, the source timeline, or their common
ancestor. In the typical failover scenario where the target cluster was
shut down soon after the divergence, this is not a problem, but if the
-   target cluster ran for a long time after the divergence, the old WAL
-   files might no longer be present. In that case, they can be manually
-   copied from the WAL archive to the pg_wal directory, or
-   fetched on startup by configuring  or
-   .  The use of
+   target cluster ran for a long time after the divergence, its old WAL
+   files might no longer be present. In this case, you can manually copy them
+   from the WAL archive to the pg_wal directory, or run
+   pg_rewind with the -c option to
+   automatically retrieve them from the WAL archive. The use of
pg_rewind is not limited to failover, e.g.  a standby

Re: Error on failed COMMIT

[Dave Cramer]

On Wed, 26 Feb 2020 at 13:46, Vik Fearing  wrote:

> On 25/02/2020 12:11, Laurenz Albe wrote:
> > On Tue, 2020-02-25 at 13:25 +0530, Robert Haas wrote:
> >> On Tue, Feb 25, 2020 at 12:47 PM Vladimir Sitnikov
> >>  wrote:
> >>> Noone suggested that "commit leaves the session in a transaction
> state".
> >>> Of course, every commit should terminate the transaction.
> >>> However, if a commit fails (for any reason), it should produce the
> relevant ERROR that explains what went wrong rather than silently doing a
> rollback.
> >>
> >> OK, I guess I misinterpreted the proposal. That would be much less
> >> problematic -- any driver or application that can't handle ERROR in
> >> response to an attempted COMMIT would be broken already.
> >
> > I agree with that.
> >
> > There is always some chance that someone relies on COMMIT not
> > throwing an error when it rolls back, but I think that throwing an
> > error is actually less astonishing than *not* throwing one.
> >
> > So, +1 for the proposal from me.
>
> I started this thread for some discussion and hopefully a documentation
> patch.  But now I have moved firmly into the +1 camp.  COMMIT should
> error if it can't commit, and then terminate the (aborted) transaction.
> --
> Vik Fearing
>

OK, here is a patch that actually doesn't leave the transaction in a failed
state but emits the error and rolls back the transaction.

This is far from complete as it fails a number of tests  and does not cover
all of the possible paths.
But I'd like to know if this is strategy will be acceptable ?
What it does is create another server error level that will emit the error
and return as opposed to not returning.
I honestly haven't given much thought to the error message. At this point I
just want the nod as to how to do it.


0001-change-commit-semantics-to-throw-an-error-and-then-r.patch
Description: Binary data

Re: WIP: Aggregation push-down

[legrand legrand]

Antonin Houska-2 wrote
> Alvaro Herrera <

> alvherre@

> > wrote:
> 
>> This stuff seems very useful.  How come it sits unreviewed for so long?
> 
> I think the review is hard for people who are not interested in the
> planner
> very much. And as for further development, there are a few design
> decisions
> that can hardly be resolved without Tom Lane's comments. Right now I
> recall
> two problems: 1) is the way I currently store RelOptInfo for the grouped
> relations correct?, 2) how should we handle types for which logical
> equality
> does not imply physical (byte-wise) equality?
> 
> Fortunately it seems now that I'm not the only one who cares about 2), so
> this
> problem might be resolved soon:
> 
> https://www.postgresql.org/message-id/CAH2-Wzn3Ee49Gmxb7V1VJ3-AC8fWn-Fr8pfWQebHe8rYRxt5OQ%40mail.gmail.com
> 
> But 1) still remains.
> 
> -- 
> Antonin Houska
> Web: https://www.cybertec-postgresql.com

Hello
would "pgsql: Add equalimage B-Tree support functions." 
https://www.postgresql.org/message-id/e1j72ny-0002gi...@gemulon.postgresql.org

help for 2) ?

Regards
PAscal




--
Sent from: https://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html

Re: Error on failed COMMIT

[Tom Lane]

Dave Cramer  writes:
> OK, here is a patch that actually doesn't leave the transaction in a failed
> state but emits the error and rolls back the transaction.

> This is far from complete as it fails a number of tests  and does not cover
> all of the possible paths.
> But I'd like to know if this is strategy will be acceptable ?

I really don't think that changing the server's behavior here is going to
fly.  The people who are unhappy that we changed it are going to vastly
outnumber the people who are happy.  Even the people who are happy are not
going to find that their lives are improved all that much, because they'll
still have to deal with old servers with the old behavior for the
foreseeable future.

Even granting that a behavioral incompatibility is acceptable, I'm not
sure how a client is supposed to be sure that this "error" means that a
rollback happened, as opposed to real errors that prevented any state
change from occurring.  (A trivial example of that is misspelling the
COMMIT command; which I'll grant is unlikely in practice.  But there are
less-trivial examples involving internal server malfunctions.)  The only
way to be sure you're out of the transaction is to check the transaction
state that's sent along with ReadyForQuery ... but if you need to do
that, it's not clear why we should change the server behavior at all.

I also don't think that this scales to the case of subtransaction
commit/rollback.  That should surely act the same, but your patch doesn't
change it.

Lastly, introducing a new client-visible message level seems right out.
That's a very fundamental protocol break, independently of all else.
And if it's "not really an error", then how is this any more standards
compliant than before?

regards, tom lane

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

[Alexander Korotkov]

On Wed, Feb 26, 2020 at 11:45 PM Alexey Kondratov
 wrote:
> On 2020-02-26 22:03, Alexander Korotkov wrote:
> > On Tue, Feb 25, 2020 at 1:48 PM Alexander Korotkov
> >  wrote:
> >>
> >> I think usage of chmod() deserves comment.  As I get default
> >> permissions are sufficient for work, but we need to set them to
> >> satisfy 'check PGDATA permissions' test.
> >
> >
> > I've added this comment myself.
> >
>
> Thanks for doing it yourself, I was going to answer tonight, but it
> would be obviously too late.
>
> >
> > I've also fixes some indentation.
> > Patch now looks good to me.  I'm going to push it if no objections.
> >
>
> I think that docs should be corrected. Previously Michael was against
> the phrase 'restore_command defined in the postgresql.conf', since it
> also could be defined in any config file included there. We corrected it
> in the pg_rewind --help output, but now docs say:
>
> +Use the restore_command defined in
> +postgresql.conf to retrieve WAL files from
> +the WAL archive if these files are no longer available in the
> +pg_wal directory of the target cluster.
>
> Probably it should be something like:
>
> +Use the restore_command defined in
> +the target cluster configuration to retrieve WAL files from
> +the WAL archive if these files are no longer available in the
> +pg_wal directory.
>
> Here the only text split changed:
>
> -* Ignore restore_command when not in archive recovery (meaning
> -* we are in crash recovery).
> +* Ignore restore_command when not in archive recovery (meaning we are
> in
> +* crash recovery).
>
> Should we do so in this patch?
>
> I think that this extra dot at the end is not necessary here:
>
> +   pg_log_debug("using config variable restore_command=\'%s\'.",
> restore_command);
>
> If you agree then attached is a patch with all the corrections above.

Thank you!  I'll push the v17 version of patch.

Regarding text split change, it was made by pgindent.  I didn't notice
it belongs to unchanged part of code.  Sure, we shouldn't include this
into the patch.

> It is made with default git format-patch format, but yours were in a
> slightly different format, so I only was able to apply them with git am
> --patch-format=stgit.

I made this patch using 'git show'. Yes, it would be better if I use
'git format-patch' instead.  Thank you for noticing.

--
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: Error on failed COMMIT

[Vik Fearing]

On 26/02/2020 22:22, Tom Lane wrote:
> Dave Cramer  writes:
>> OK, here is a patch that actually doesn't leave the transaction in a failed
>> state but emits the error and rolls back the transaction.
> 
>> This is far from complete as it fails a number of tests  and does not cover
>> all of the possible paths.
>> But I'd like to know if this is strategy will be acceptable ?
> 
> I really don't think that changing the server's behavior here is going to
> fly.  The people who are unhappy that we changed it are going to vastly
> outnumber the people who are happy.  Even the people who are happy are not
> going to find that their lives are improved all that much, because they'll
> still have to deal with old servers with the old behavior for the
> foreseeable future.

Dealing with old servers for a while is something that everyone is used to.

> Even granting that a behavioral incompatibility is acceptable, I'm not
> sure how a client is supposed to be sure that this "error" means that a
> rollback happened, as opposed to real errors that prevented any state
> change from occurring.

Because the error is a Class 40 — Transaction Rollback.

My original example was:

postgres=!# commit;
ERROR:  40P00: transaction cannot be committed
DETAIL:  First error was "42601: syntax error at or near "error"".


> (A trivial example of that is misspelling the
> COMMIT command; which I'll grant is unlikely in practice.  But there are
> less-trivial examples involving internal server malfunctions.)

Misspelling the COMMIT command is likely a syntax error, which is Class
42.  Can you give one of those less-trivial examples please?

> The only
> way to be sure you're out of the transaction is to check the transaction
> state that's sent along with ReadyForQuery ... but if you need to do
> that, it's not clear why we should change the server behavior at all.

How does this differ from the deferred constraint violation example you
provided early on in the thread?  That gave the error 23505 and
terminated the transaction.  If you run the same scenario with the
primary key immediate, you get the *exact same error* but the
transaction is *not* terminated!

I won't go so far as to suggest we change all COMMIT errors to Class 40
(as the spec says), but I'm thinking it very loudly.

> I also don't think that this scales to the case of subtransaction
> commit/rollback.  That should surely act the same, but your patch doesn't
> change it.

How does one commit a subtransaction?

> Lastly, introducing a new client-visible message level seems right out.
> That's a very fundamental protocol break, independently of all else.

Yeah, this seemed like a bad idea to me, too.
-- 
Vik Fearing

Re: Error on failed COMMIT

[Dave Cramer]

On Wed, 26 Feb 2020 at 16:57, Vik Fearing  wrote:

> On 26/02/2020 22:22, Tom Lane wrote:
> > Dave Cramer  writes:
> >> OK, here is a patch that actually doesn't leave the transaction in a
> failed
> >> state but emits the error and rolls back the transaction.
> >
> >> This is far from complete as it fails a number of tests  and does not
> cover
> >> all of the possible paths.
> >> But I'd like to know if this is strategy will be acceptable ?
> >
> > I really don't think that changing the server's behavior here is going to
> > fly.  The people who are unhappy that we changed it are going to vastly
> > outnumber the people who are happy.


I'm not convinced of this. I doubt we actually have any real numbers?

Even the people who are happy are not
> > going to find that their lives are improved all that much, because
> they'll
> > still have to deal with old servers with the old behavior for the
> > foreseeable future.
>
> Dealing with old servers for a while is something that everyone is used to.
>
Clients can code around this as well for old servers. This is something
that is more palatable
if the server defines this behaviour.

>
> > Even granting that a behavioral incompatibility is acceptable, I'm not
> > sure how a client is supposed to be sure that this "error" means that a
> > rollback happened, as opposed to real errors that prevented any state
> > change from occurring.
>
> Because the error is a Class 40 — Transaction Rollback.
>

I think his point is that the error is emitted before we actually do the
rollback and it could fail.


>
> My original example was:
>
> postgres=!# commit;
> ERROR:  40P00: transaction cannot be committed
> DETAIL:  First error was "42601: syntax error at or near "error"".
>
>
> > (A trivial example of that is misspelling the
> > COMMIT command; which I'll grant is unlikely in practice.  But there are
> > less-trivial examples involving internal server malfunctions.)
>
> Misspelling the COMMIT command is likely a syntax error, which is Class
> 42.  Can you give one of those less-trivial examples please?
>
> > The only
> > way to be sure you're out of the transaction is to check the transaction
> > state that's sent along with ReadyForQuery ... but if you need to do
> > that, it's not clear why we should change the server behavior at all.
>

I guess the error has to be sent after the rollback completes.

>
> How does this differ from the deferred constraint violation example you
> provided early on in the thread?  That gave the error 23505 and
> terminated the transaction.  If you run the same scenario with the
> primary key immediate, you get the *exact same error* but the
> transaction is *not* terminated!
>
> I won't go so far as to suggest we change all COMMIT errors to Class 40
> (as the spec says), but I'm thinking it very loudly.
>
> > I also don't think that this scales to the case of subtransaction
> > commit/rollback.  That should surely act the same, but your patch doesn't
> > change it.
>
> How does one commit a subtransaction?
>
> > Lastly, introducing a new client-visible message level seems right out.
> > That's a very fundamental protocol break, independently of all else.
>
> Yeah, this seemed like a bad idea to me, too.
>

Pretty sure I can code around this.

-- 
> Vik Fearing
>

Re: PoC: custom signal handler for extensions

[legrand legrand]

Hello Maksim,

reading about
https://www.postgresql-archive.org/Allow-auto-explain-to-log-plans-before-queries-are-executed-td6124646.html
makes me think (again) about your work and pg_query_state ...

Is there a chance to see you restarting working on this patch ?
Regards
PAscal



--
Sent from: https://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html

Re: [HACKERS] [WIP] Effective storage of duplicates in B-tree index.

[Peter Geoghegan]

On Mon, Feb 24, 2020 at 4:54 PM Peter Geoghegan  wrote:
> Attached is v34, which has this change. My plan is to commit something
> very close to this on Wednesday morning (barring any objections).

Pushed.

I'm going to delay committing the pageinspect patch until tomorrow,
since I haven't thought about that aspect of the project in a while.
Seems like a good idea to go through it one more time, once it's clear
that the buildfarm is stable. The buildfarm appears to be stable now,
though there was an issue with a compiler warning earlier. I quickly
pushed a fix for that, and can see that longfin is green/passing now.

Thanks for sticking with this project, Anastasia.
-- 
Peter Geoghegan

Re: Quoting issues with createdb

[Daniel Gustafsson]

> On 14 Feb 2020, at 05:10, Michael Paquier  wrote:

> createdb has a couple of issues with its quoting.  For example take
> that, which can be confusing:
> $ createdb --lc-ctype="en_US.UTF-8';create table aa();select '1" popo
> createdb: error: database creation failed: ERROR:  CREATE DATABASE
> cannot run inside a transaction block

Nice catch!

> The root of the issue is that any values added by the command caller
> with --lc-collate, --lc-ctype or --encoding are not quoted properly,
> and in all three cases it means that the quoting needs to be
> encoding-sensitive (Tom mentioned me directly that part).  This proper
> quoting can be achieved using appendStringLiteralConn() from
> string_utils.c, at the condition of taking the connection to the
> server before building the CREATE DATABASE query.

Makes sense, it aligns it with other utils and passes all the tests.  +1 on the
fix.

> Any opinions?

I would've liked a negative test basically along the lines of your example
above.  If we left a hole the size of this, it would be nice to catch it from
accidentally happening again.

diff --git a/src/bin/scripts/t/020_createdb.pl 
b/src/bin/scripts/t/020_createdb.pl
index c0f6067a92..afd128deba 100644
--- a/src/bin/scripts/t/020_createdb.pl
+++ b/src/bin/scripts/t/020_createdb.pl
@@ -3,7 +3,7 @@ use warnings;

 use PostgresNode;
 use TestLib;
-use Test::More tests => 13;
+use Test::More tests => 14;

 program_help_ok('createdb');
 program_version_ok('createdb');
@@ -24,3 +24,6 @@ $node->issues_sql_like(

 $node->command_fails([ 'createdb', 'foobar1' ],
'fails if database already exists');
+
+$node->command_fails(['createdb', '-l', 'C\';SELECT 1;' ],
+   'fails on incorrect locale');

cheers ./daniel

Re: Reducing WaitEventSet syscall churn

[Thomas Munro]

On Sat, Feb 8, 2020 at 10:15 AM Thomas Munro  wrote:
> > > Here are some patches to get rid of frequent system calls.

Here's a new version of this patch set.  It gets rid of all temporary
WaitEventSets except a couple I mentioned in another thread[1].
WaitLatch() uses CommonWaitSet, and calls to WaitLatchOrSocket() are
replaced by either the existing FeBeWaitSet (walsender, gssapi/openssl
auth are also candidates) or a special purpose long lived WaitEventSet
(replication, postgres_fdw, pgstats).  It passes make check-world with
WAIT_USE_POLL, WAIT_USE_KQUEUE, WAIT_USE_EPOLL, all with and without
-DEXEC_BACKEND, and make check with WAIT_USE_WIN32 (Appveyor).

0001: "Don't use EV_CLEAR for kqueue events."

This fixes a problem in the kqueue implementation that only shows up
once you switch to long lived WaitEventSets.  It needs to be
level-triggered like the other implementations, for example because
there's a place in the recovery tests where we wait twice in a row
without trying to do I/O in between.  (This is a bit like commit
3b790d256f8 that fixed a similar problem on Windows.)

0002: "Use a long lived WaitEventSet for WaitLatch()."

In the last version, I had a new function WaitMyLatch(), but that
didn't help with recoveryWakeupLatch.  Switching between latches
doesn't require a syscall, so I didn't want to have a separate WES and
function just for that.  So I went back to using plain old
WaitLatch(), and made it "modify" the latch every time before waiting
on CommonWaitSet.  An alternative would be to get rid of the concept
of latches other than MyLatch, and change the function to
WaitMyLatch().  A similar thing happens for exit_on_postmaster_death,
for which I didn't want to have a separate WES, so I just set that
flag every time.  Thoughts?

0003: "Use regular WaitLatch() for condition variables."

That mechanism doesn't need its own WES anymore.

0004: "Introduce RemoveWaitEvent()."

We'll need that to be able to handle connections that are closed and
reopened under the covers by libpq (replication, postgres_fdw).  We
also wanted this on a couple of other threads for multiplexing FDWs,
to be able to adjust the wait set dynamically for a proposed async
Append feature.

The implementation is a little naive, and leaves holes in the
"pollfds" and "handles" arrays (for poll() and win32 implementations).
That could be improved with a bit more footwork in a later patch.

XXX The Windows version is based on reading documentation.  I'd be
very interested to know if check-world passes (especially
contrib/postgres_fdw and src/test/recovery).  Unfortunately my
appveyor.yml fu is not yet strong enough.

0005: "libpq: Add PQsocketChangeCount to advertise socket changes."

To support a long lived WES, libpq needs a way tell us when the socket
changes underneath our feet.  This is the simplest thing I could think
of; better ideas welcome.

0006: "Reuse a WaitEventSet in libpqwalreceiver.c."

Rather than having all users of libpqwalreceiver.c deal with the
complicated details of wait set management, have libpqwalreceiver
expose a waiting interface that understands socket changes.

Unfortunately, I couldn't figure out how to use CommonWaitSet for this
(ie adding and removing sockets to that as required), due to
complications with the bookkeeping required to provide the fd_closed
flag to RemoveWaitEvent().  So it creates its own internal long lived
WaitEventSet.

0007: "Use a WaitEventSet for postgres_fdw."

Create a single WaitEventSet and use it for all FDW connections.  By
having our own dedicated WES, we can do the bookkeeping required to
know when sockets have been closed or need to removed from kernel wait
sets explicitly (which would be much harder to achieve if
CommonWaitSet were to be used like that; you need to know when sockets
are closed by other code, so you can provide fd_closed to
RemoveWaitEvent()).

Concretely, if you use just one postgres_fdw connection, you'll see
just epoll_wait()/kevent() calls for waits, but whever you switch
between different connections, you'll see eg EPOLL_DEL/EV_DELETE
followed by EPOLL_ADD/EV_ADD when the set is adjusted (in the kqueue
implementation these could be collapse into the following wait, but I
haven't done the work for that).  An alternative would be to have one
WES per FDW connection, but that seemed wasteful of file descriptors.

0008: "Use WL_EXIT_ON_PM_DEATH in FeBeWaitSet."

The FATAL message you get if you happen to be waiting for IO rather
than waiting somewhere else seems arbitrarily different.  By switching
to a standard automatic exit, it opens the possibility of using
FeBeWaitSet in a couple more places that would otherwise need to
create their own WES (see also [1]).  Thoughts?

0009: "Use FeBeWaitSet for walsender.c."

Enabled by 0008.

0010: "Introduce a WaitEventSet for the stats collector."

[1] 
https://www.postgresql.org/message-id/flat/CA%2BhUKGK%3Dm9dLrq42oWQ4XfK9iDjGiZVwpQ1HkHrAPfG7Kh681g%40mail.gmail.com
From 6787ffeb57043b2b19201a49e656d56

Re: Shouldn't GSSAPI and SSL code use FeBeWaitSet?

[Thomas Munro]

On Mon, Feb 24, 2020 at 4:55 PM Thomas Munro  wrote:
> On Mon, Feb 24, 2020 at 4:49 PM Thomas Munro  wrote:
> > While working on a patch to reuse a common WaitEventSet for latch
> > waits, I noticed that be-secure-gssapi.c and be-secure-openssl.c don't
> > use FeBeWaitSet.  They probably should, for consistency with
> > be-secure.c, because that surely holds the socket they want, no?
>
> Hmm.  Perhaps it's like that because they're ignoring their latch
> (though they pass it in just because that interface requires it).  So
> then why not reset it and process read interrupts, like be-secure.c?

Perhaps the theory is that it doesn't matter if they ignore eg
SIGQUIT, because authentication_timeout will come along in (say) 60
seconds and exit anyway?  That makes me wonder whether it's OK that
StartupPacketTimeoutHandler() does proc_exit() from a signal handler.

Re: Allow auto_explain to log plans before queries are executed

[Yugo NAGATA]

On Wed, 26 Feb 2020 18:51:21 +0100
Julien Rouhaud  wrote:

> On Thu, Feb 27, 2020 at 02:35:18AM +0900, Yugo NAGATA wrote:
> > Hi,
> >
> > Attached is a patch for allowing auto_explain to log plans before
> > queries are executed.
> >
> > Currently, auto_explain logs plans only after query executions,
> > so if a query gets stuck its plan could not be logged. If we can
> > know plans of stuck queries, we may get some hints to resolve the
> > stuck. This is useful when you are testing and debugging your
> > application whose queries get stuck in some situations.
> 
> Indeed that could be useful.
> 
> > This patch adds  new option log_before_query to auto_explain.
> 
> Maybe "log_before_execution" would be better?

Thanks!  This seems better also to me.

> 
> > Setting auto_explain.log_before_query option logs all plans before
> > queries are executed regardless of auto_explain.log_min_duration
> > unless this is set -1 to disable logging.  If log_before_query is
> > enabled, only duration time is logged after query execution as in
> > the case of when both log_statement and log_min_duration_statement
> > are enabled.
> 
> I'm not sure about this behavior.  The final explain plan is needed at least 
> if
> log_analyze, log_buffers or log_timing are enabled.

In the current patch, log_before_query (will be log_before_execution)
has no effect if log_analyze is enabled in order to avoid to log the
same plans twice.  Instead, is it better to log the plan always twice,
before and after the execution, if  log_before_query is enabled
regardless of log_min_duration or log_analyze?

Regards,
Yugo Nagata

-- 
Yugo NAGATA 

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

[Michael Paquier]

On Thu, Feb 27, 2020 at 12:43:55AM +0300, Alexander Korotkov wrote:
> Regarding text split change, it was made by pgindent.  I didn't notice
> it belongs to unchanged part of code.  Sure, we shouldn't include this
> into the patch.

I have read through v17 (not tested, sorry), and spotted a couple of
issues that need to be addressed.

+   "--source-pgdata=$standby_pgdata",
+   "--target-pgdata=$master_pgdata",
+   "--no-sync", "--no-ensure-shutdown",
FWIW, I think that perl indenting would reshape this part.  I would
recommend to run src/tools/pgindent/pgperltidy and
./src/tools/perlcheck/pgperlcritic before commit.

+ * Copyright (c) 2020, PostgreSQL Global Development Group
Wouldn't it be better to just use the full copyright here?  I mean the
following:
Portions Copyright (c) 1996-2020, PostgreSQL Global Development Group
Portions Copyright (c) 1994, The Regents of the University of California

+++ b/src/common/archive.c
[...]
+#include "postgres.h"
+
+#include "common/archive.h"
This is incorrect.  All files shared between the backend and the
frontend in src/common/ have to include the following set of headers:
#ifndef FRONTEND
#include "postgres.h"
#else
#include "postgres_fe.h"
#endif

+++ b/src/common/fe_archive.c
[...]
+#include "postgres_fe.h"
This is incomplete.  The following piece should be added:
#ifndef FRONTEND
#error "This file is not expected to be compiled for backend code"
#endif

+   snprintf(postgres_cmd, sizeof(postgres_cmd), "%s -D %s
-C restore_command",
+postgres_exec_path, datadir_target);
+
I think that this is missing proper quoting.

I would rename ConstructRestoreCommand() to BuildRestoreCommand()
while on it..

I think that it would be saner to check the return status of
ConstructRestoreCommand() in xlogarchive.c as a sanity check, with an
elog(ERROR) if not 0, as that should never happen.
--
Michael


signature.asc
Description: PGP signature

Re: Quoting issues with createdb

[Michael Paquier]

On Thu, Feb 27, 2020 at 12:00:11AM +0100, Daniel Gustafsson wrote:
> Makes sense, it aligns it with other utils and passes all the tests.  +1 on 
> the
> fix.

Thanks for the review.

> I would've liked a negative test basically along the lines of your example
> above.  If we left a hole the size of this, it would be nice to catch it from
> accidentally happening again.

No arguments against that.

>  program_help_ok('createdb');
>  program_version_ok('createdb');
> @@ -24,3 +24,6 @@ $node->issues_sql_like(
> 
>  $node->command_fails([ 'createdb', 'foobar1' ],
> 'fails if database already exists');
> +
> +$node->command_fails(['createdb', '-l', 'C\';SELECT 1;' ],
> +   'fails on incorrect locale');

One problem with this way of testing things is that you don't check
the exact error message triggered, and this command fails with or
without the patch, so you don't actually know if things are correctly
patched up or not.  What should be used instead is command_checks_all,
available down to 11 where we check for a failure and a match with the
error string generated.  I have used that, and applied the patch down
to 9.5.
--
Michael


signature.asc
Description: PGP signature

Re: Commit fest manager for 2020-03

[Michael Paquier]

On Wed, Feb 26, 2020 at 03:29:18PM +0100, Daniel Gustafsson wrote:
> On 26 Feb 2020, at 14:39, David Steele  wrote:
>> I'm happy to be CFM for this commitfest.
> 
> Thanks! 

Thanks David!
--
Michael


signature.asc
Description: PGP signature

Re: Commit fest manager for 2020-03

[Michael Paquier]

On Wed, Feb 26, 2020 at 08:34:26PM +0100, Tomas Vondra wrote:
> Nope, the RMT for PG12 was announced on 2019/03/30 [1], i.e. shortly
> before the end of the last CF (and before pgcon). I think there was some
> discussion about the members at/after the FOSDEM dev meeting. The
> overlap with CFM duties is still fairly minimal, and there's not much
> for RMT to do before the end of the last CF anyway ...
> 
> [1] https://www.postgresql.org/message-id/20190330094043.ga28...@paquier.xyz
> 
> Maybe we shouldn't wait with assembling RMT until pgcon, though.

Waiting until PGCon if a bad idea, because we need to decide the
feature freeze deadline after the last CF, and this decision is taken
mainly by the RMT.  I think that it is also good to begin categorizing
open items and handle them when reported.
--
Michael


signature.asc
Description: PGP signature

Re: Yet another vectorized engine

[Hubert Zhang]

On Wed, Feb 26, 2020 at 7:59 PM Konstantin Knizhnik <
k.knizh...@postgrespro.ru> wrote:

>
>
> On 26.02.2020 13:11, Hubert Zhang wrote:
>
>
>
>> and with JIT:
>>
>>  13.88%  postgres  postgres [.] tts_buffer_heap_getsomeattrs
>>7.15%  postgres  vectorize_engine.so  [.] vfloat8_accum
>>6.03%  postgres  postgres [.] HeapTupleSatisfiesVisibility
>>5.55%  postgres  postgres [.] bpchareq
>>4.42%  postgres  vectorize_engine.so  [.] VExecStoreColumns
>>4.19%  postgres  postgres [.] hashbpchar
>>4.09%  postgres  vectorize_engine.so  [.] vfloat8pl
>>
>>
> I also tested Q1 with your latest code. Result of vectorized is still slow.
> PG13 native: 38 secs
> PG13 Vec: 30 secs
> PG13 JIT: 23 secs
> PG13 JIT+Vec: 27 secs
>
>
> It is strange that your results are much slower than my and profile is
> very different.
> Which postgres configuration you are using?
>
>
./configure CFLAGS="-O3 -g -march=native" --prefix=/usr/local/pgsql/
--disable-cassert --enable-debug --with-llvm
 I also use `PGXS := $(shell $(PG_CONFIG) --pgxs)` to compile
vectorized_engine. So it will share the same compile configuration.

My perf result is as belows. There are three parts:
> 1. lookup_hash_entry(43.5%) this part is not vectorized yet.
>
> It is vectorized in some sense: lookup_hash_entry performs bulk of hash
> lookups and pass array with results of such lookups to aggregate transmit
> functions.
> It will be possible to significantly increase speed of HashAgg if we store
> data in order of grouping attributes and use RLE (run length encoding) to
> peform just one
> hash lookup for group of values. But it requires creation of special
> partitions (like it is done in Vertica and VOPS).
>
>
Yes, Vertica's partition needed to be pre-sorted on user defined columns.
So for TPCH Q1 on Postgres, we could not have that assumption. And my Q1
plan uses HashAgg instead of GroupAgg based on cost.


> 2. scan part: fetch_input_tuple(36%)
> 3. vadvance_aggregates part(20%)
> I also perfed on PG96 vectorized version and got similar perf results and
> running time of vectorized PG96 and PG13 are also similar. But PG13 is much
> faster than PG96. So I just wonder whether we merge all the latest executor
> code of PG13 into the vectorized PG13 branch?
>
>
> Sorry, I do not understand the question. vectorize_executor contains
> patched versions of nodeSeqscan  and nodeAgg from standard executor.
> When performing porting to PG13, I took the latest version of nodeAgg and
> tried to apply your patches to it. Certainly not always it was possible and
> I have to rewrite a lt of places. Concerning nodeSeqscan - I took old
> version from vectorize_executor and port it to PG13.
>

> It is strange that I am not seeing lookup_hash_entry in profile in my
> case.
>
>
So you already have the PG13 nodeAgg, that is good.
Yes, it is strange. Hash table probing is always the costly part.
My perf command `perf record --call-graph dwarf -p pid`
Could you share your lineitem schema and Q1 query?
My schema and Q1 query are:
CREATE TABLE lineitem (
l_orderkey BIGINT NOT NULL,
l_partkey INTEGER NOT NULL,
l_suppkey INTEGER NOT NULL,
l_linenumber INTEGER NOT NULL,
l_quantity double precision NOT NULL,
l_extendedprice double precision NOT NULL,
l_discount double precision NOT NULL,
l_tax double precision NOT NULL,
l_returnflag CHAR(1) NOT NULL,
l_linestatus CHAR(1) NOT NULL,
l_shipdate DATE NOT NULL,
l_commitdate DATE NOT NULL,
l_receiptdate DATE NOT NULL,
l_shipinstruct CHAR(25) NOT NULL,
l_shipmode CHAR(10) NOT NULL,
l_comment VARCHAR(44) NOT NULL
);
select
l_returnflag,
l_linestatus,
sum(l_quantity) as sum_qty,
sum(l_extendedprice) as sum_base_price,
sum(l_extendedprice * (1 - l_discount)) as sum_disc_price,
sum(l_extendedprice * (1 - l_discount) * (1 + l_tax)) as sum_charge,
avg(l_quantity) as avg_qty,
avg(l_extendedprice) as avg_price,
avg(l_discount) as avg_disc,
count(l_discount) as count_order
from
lineitem
where
l_shipdate <= date '1998-12-01' - interval '106 day'
group by
l_returnflag,
l_linestatus
;


-- 
Thanks

Hubert Zhang

Re: Collation versioning

[Thomas Munro]

On Thu, Feb 27, 2020 at 3:29 AM Julien Rouhaud  wrote:
> [v10]

Thanks.  I'll do some more testing and review soon.  It'd be really
cool to get this into PG13.

FYI cfbot said:

+++ 
/home/travis/build/postgresql-cfbot/postgresql/src/test/regress/results/collate.icu.utf8.out
2020-02-26 14:45:52.114401999 +
...
- icuidx06_d_en_fr_ga | "default" | up to date
+ icuidx06_d_en_fr_ga   | "default"  | out of date

Re: Memory-Bounded Hash Aggregation

[Jeff Davis]

On Mon, 2020-02-24 at 15:29 -0800, Andres Freund wrote:
> On 2020-02-22 11:02:16 -0800, Jeff Davis wrote:
> > On Sat, 2020-02-22 at 10:00 -0800, Andres Freund wrote:
> > > Both patches, or just 0013? Seems the earlier one might make the
> > > addition of the opcodes you add less verbose?
> > 
> > Just 0013, thank you. 0008 looks like it will simplify things.
> 
> Pushed 0008.

Rebased on your change. This simplified the JIT and interpretation code
quite a bit.

Also:
* caching the compiled expressions so I can switch between the variants
cheaply
* added "Planned Partitions" to explain output
* included tape buffers in the "Memory Used" output
* Simplified the way I try to track memory usage and trigger spilling. 
* Reset hash tables always rather than rebuilding them from scratch.

I will do another round of performance tests and see if anything
changed from last time.

Regards,
Jeff Davis

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index c1128f89ec7..edfec0362e1 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -4476,6 +4476,24 @@ ANY num_sync ( 
+  enable_groupingsets_hash_disk (boolean)
+  
+   enable_groupingsets_hash_disk configuration parameter
+  
+  
+  
+   
+Enables or disables the query planner's use of hashed aggregation for
+grouping sets when the size of the hash tables is expected to exceed
+work_mem.  See .  Note that this setting only
+affects the chosen plan; execution time may still require using
+disk-based hash aggregation.  The default is off.
+   
+  
+ 
+
  
   enable_hashjoin (boolean)
   
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index d901dc4a50e..70196ea48d0 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -104,6 +104,7 @@ static void show_tablesample(TableSampleClause *tsc, PlanState *planstate,
 			 List *ancestors, ExplainState *es);
 static void show_sort_info(SortState *sortstate, ExplainState *es);
 static void show_hash_info(HashState *hashstate, ExplainState *es);
+static void show_hashagg_info(AggState *hashstate, ExplainState *es);
 static void show_tidbitmap_info(BitmapHeapScanState *planstate,
 ExplainState *es);
 static void show_instrumentation_count(const char *qlabel, int which,
@@ -1882,6 +1883,7 @@ ExplainNode(PlanState *planstate, List *ancestors,
 		case T_Agg:
 			show_agg_keys(castNode(AggState, planstate), ancestors, es);
 			show_upper_qual(plan->qual, "Filter", planstate, ancestors, es);
+			show_hashagg_info((AggState *) planstate, es);
 			if (plan->qual)
 show_instrumentation_count("Rows Removed by Filter", 1,
 		   planstate, es);
@@ -2769,6 +2771,67 @@ show_hash_info(HashState *hashstate, ExplainState *es)
 	}
 }
 
+/*
+ * If EXPLAIN ANALYZE, show information on hash aggregate memory usage and
+ * batches.
+ */
+static void
+show_hashagg_info(AggState *aggstate, ExplainState *es)
+{
+	Agg		*agg	   = (Agg *)aggstate->ss.ps.plan;
+	long	 memPeakKb = (aggstate->hash_mem_peak + 1023) / 1024;
+
+	Assert(IsA(aggstate, AggState));
+
+	if (agg->aggstrategy != AGG_HASHED &&
+		agg->aggstrategy != AGG_MIXED)
+		return;
+
+	if (es->costs)
+	{
+		appendStringInfoSpaces(es->str, es->indent * 2);
+		appendStringInfo(
+			es->str,
+			"Planned Partitions: %d\n",
+			aggstate->hash_planned_partitions);
+	}
+
+	if (!es->analyze)
+		return;
+
+	if (es->format == EXPLAIN_FORMAT_TEXT)
+	{
+		appendStringInfoSpaces(es->str, es->indent * 2);
+		appendStringInfo(
+			es->str,
+			"Memory Usage: %ldkB",
+			memPeakKb);
+
+		if (aggstate->hash_batches_used > 0)
+		{
+			appendStringInfo(
+es->str,
+"  Batches: %d  Disk: %ldkB",
+aggstate->hash_batches_used, aggstate->hash_disk_used);
+		}
+
+		appendStringInfo(
+			es->str,
+			"\n");
+	}
+	else
+	{
+		ExplainPropertyInteger("Peak Memory Usage", "kB", memPeakKb, es);
+		if (aggstate->hash_batches_used > 0)
+		{
+			ExplainPropertyInteger("HashAgg Batches", NULL,
+   aggstate->hash_batches_used, es);
+			ExplainPropertyInteger("Disk Usage", "kB",
+   aggstate->hash_disk_used, es);
+		}
+	}
+}
+
 /*
  * If it's EXPLAIN ANALYZE, show exact/lossy pages for a BitmapHeapScan node
  */
diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c
index 91aa386fa61..8c5ead93d68 100644
--- a/src/backend/executor/execExpr.c
+++ b/src/backend/executor/execExpr.c
@@ -79,7 +79,8 @@ static void ExecInitCoerceToDomain(ExprEvalStep *scratch, CoerceToDomain *ctest,
 static void ExecBuildAggTransCall(ExprState *state, AggState *aggstate,
   ExprEvalStep *scratch,
   FunctionCallInfo fcinfo, AggStatePerTrans pertrans,
-  int transno, int setno, int setoff, bool ishash);
+  int transno, int setno, int setoff, bool ishash,
+  bool nullcheck);
 
 
 /*
@@ -2924,10 +2925,13 @@ ExecInitCoerceToDomain(ExprEvalStep *scratch, CoerceTo

Re: allow frontend use of the backend's core hashing functions

[Robert Haas]

On Mon, Feb 24, 2020 at 5:32 PM Robert Haas  wrote:
> I've committed 0001 through 0003 as revised by Mark in accordance with
> the comments from Suraj. Here's the last patch again with a tweak to
> try not to break the Windows build, per some off-list advice I
> received on how not to break the Windows build. Barring complaints
> from the buildfarm or otherwise, I'll commit this one too.

Done.

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

Re: Error on failed COMMIT

[Robert Haas]

On Wed, Feb 26, 2020 at 11:53 PM Vladimir Sitnikov
 wrote:
> Pushing full SQL parser to the driver is not the best idea taking into the 
> account the extensibility the core has.

That wouldn't be necessary. You could just do strcmp() on the command tag.

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

Re: Error on failed COMMIT

[Vladimir Sitnikov]

But if the SQL is /*commit*/rollback, then the driver should not raise an
exception. The exception should be only for the case when the client asks
to commit and the database can't do that.

The resulting command tag alone is not enough.

Vladimir

Re: [Proposal] Global temporary tables

[曾文旌(义从)]

> 2020年2月25日 下午9:36，Prabhat Sahu  写道：
> 
> Hi All,
> 
> Please check the below findings on GTT.
> -- Scenario 1:
> Under "information_schema", We are not allowed to create "temporary table", 
> whereas we can CREATE/DROP "Global Temporary Table", is it expected ?
> 
> postgres=# create temporary table information_schema.temp1(c1 int);
> ERROR:  cannot create temporary relation in non-temporary schema
> LINE 1: create temporary table information_schema.temp1(c1 int);
>^
> 
> postgres=# create global temporary table information_schema.temp1(c1 int);
> CREATE TABLE
> 
> postgres=# drop table information_schema.temp1 ;
> DROP TABLE
> 
> -- Scenario 2:
> Here I am getting the same error message in both the below cases.
> We may add a "global" keyword with GTT related error message.
> 
> postgres=# create global temporary table gtt1 (c1 int unique);
> CREATE TABLE
> postgres=# create temporary table tmp1 (c1 int unique);
> CREATE TABLE
> 
> postgres=# create temporary table tmp2 (c1 int references gtt1(c1) );
> ERROR:  constraints on temporary tables may reference only temporary tables
> 
> postgres=# create global temporary table gtt2 (c1 int references tmp1(c1) );
> ERROR:  constraints on temporary tables may reference only temporary tables
Fixed in global_temporary_table_v15-pg13.patch


Wenjing


> 
> Thanks,
> Prabhat Sahu
> 
> On Tue, Feb 25, 2020 at 2:25 PM 曾文旌(义从)  > wrote:
> 
> 
>> 2020年2月24日 下午5:44，Prabhat Sahu > > 写道：
>> 
>> On Fri, Feb 21, 2020 at 9:10 PM 曾文旌(义从) > > wrote:
>> Hi,
>> I have started testing the "Global temporary table" feature,
>> That's great, I see hope.
>> from "gtt_v11-pg13.patch". Below is my findings:
>> 
>> -- session 1:
>> postgres=# create global temporary table gtt1(a int);
>> CREATE TABLE
>> 
>> -- seeeion 2:
>> postgres=# truncate gtt1 ;
>> ERROR:  could not open file "base/13585/t3_16384": No such file or directory
>> 
>> is it expected?
>> 
>> Oh ,this is a bug, I fixed it.
>> Thanks for the patch.
>> I have verified the same, Now the issue is resolved with v12 patch.
>> 
>> Kindly confirm the below scenario:
>> 
>> postgres=# create global temporary table gtt1 (c1 int unique);
>> CREATE TABLE
>> 
>> postgres=# create global temporary table gtt2 (c1 int references gtt1(c1) );
>> ERROR:  referenced relation "gtt1" is not a global temp table
>> 
>> postgres=# create table tab2 (c1 int references gtt1(c1) );
>> ERROR:  referenced relation "gtt1" is not a global temp table
>> 
>> Thanks, 
>> Prabhat Sahu
> 
> GTT supports foreign key constraints in global_temporary_table_v13-pg13.patch
> 
> 
> Wenjing
> 
> 
> 
> 
> 
> -- 
> With Regards,
> Prabhat Kumar Sahu
> EnterpriseDB: http://www.enterprisedb.com 

Crash by targetted recovery

[Kyotaro Horiguchi]

Hello.

We found that targetted promotion can cause an assertion failure.  The
attached TAP test causes that.

> TRAP: FailedAssertion("StandbyMode", File: "xlog.c", Line: 12078)

After recovery target is reached, StartupXLOG turns off standby mode
then refetches the last record. If the last record starts from the
previous WAL segment, the assertion failure is triggered.

The wrong point is that StartupXLOG does random access fetching while
WaitForWALToBecomeAvailable is thinking it is still in streaming.  I
think if it is called with random access mode,
WaitForWALToBecomeAvailable should move to XLOG_FROM_ARCHIVE even
though it is thinking that it is still reading from stream.

regards.

-- Kyotaro Horiguchi
NTT Open Source Software Center
>From 8cb817a43226a0d60dd62c6205219a2f0c807b9e Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi 
Date: Wed, 26 Feb 2020 20:41:11 +0900
Subject: [PATCH 1/2] TAP test for a crash bug

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

diff --git a/src/test/recovery/t/003_recovery_targets.pl b/src/test/recovery/t/003_recovery_targets.pl
index fd14bab208..8e71788981 100644
--- a/src/test/recovery/t/003_recovery_targets.pl
+++ b/src/test/recovery/t/003_recovery_targets.pl
@@ -167,3 +167,35 @@ foreach my $i (0..100)
 $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');
+
+
+# Edge case where targetted promotion happens on segment boundary
+$node_standby = get_new_node('standby_9');
+$node_standby->init_from_backup($node_master, 'my_backup',
+has_restoring => 1, has_streaming => 1);
+$node_standby->start;
+## read wal_segment_size
+my $result =
+  $node_standby->safe_psql('postgres', "SHOW wal_segment_size");
+die "unknown format of wal_segment_size: $result\n"
+  if ($result !~ /^([0-9]+)MB$/);
+my $segsize = $1 * 1024 * 1024;
+## stop just before the next segment boundary
+$result =
+  $node_standby->safe_psql('postgres', "SELECT pg_last_wal_replay_lsn()");
+my ($seg, $off) = split('/', $result);
+my $target = sprintf("$seg/%08X", (hex($off) / $segsize + 1) * $segsize);
+
+$node_standby->stop;
+$node_standby->append_conf('postgresql.conf', qq(
+recovery_target_inclusive=no
+recovery_target_lsn='$target'
+recovery_target_action='promote'
+));
+$node_standby->start;
+## do targetted promote
+$node_master->safe_psql('postgres', "CREATE TABLE t(); DROP TABLE t;");
+$node_master->safe_psql('postgres', "SELECT pg_switch_wal(); CHECKPOINT;");
+my $caughtup_query = "SELECT NOT pg_is_in_recovery()";
+$node_standby->poll_query_until('postgres', $caughtup_query)
+  or die "Timed out while waiting for standby to promote";
-- 
2.18.2

>From 424729eafeeab8c96074f4c8d5b87d3a23cbf73a Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi 
Date: Wed, 26 Feb 2020 20:41:37 +0900
Subject: [PATCH 2/2] Fix a crash bug of targetted promotion.

After recovery target is reached, StartupXLOG turns off standby mode
then refetches the last record. If the last record starts from the
previous segment at the time, WaitForWALToBecomeAvailable crashes with
assertion failure.  WaitForWALToBecomeAvailable should move back to
XLOG_FROM_ARCHIVE if random access is specified while streaming.
---
 src/backend/access/transam/xlog.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index d19408b3be..41ed916342 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -11831,7 +11831,8 @@ WaitForWALToBecomeAvailable(XLogRecPtr RecPtr, bool randAccess,
 	 * 1. Read from either archive or pg_wal (XLOG_FROM_ARCHIVE), or just
 	 *	  pg_wal (XLOG_FROM_PG_WAL)
 	 * 2. Check trigger file
-	 * 3. Read from primary server via walreceiver (XLOG_FROM_STREAM)
+	 * 3. Read from primary server via walreceiver (XLOG_FROM_STREAM).
+	 *Random access mode rewinds the state machine to 1.
 	 * 4. Rescan timelines
 	 * 5. Sleep wal_retrieve_retry_interval milliseconds, and loop back to 1.
 	 *
@@ -11846,7 +11847,7 @@ WaitForWALToBecomeAvailable(XLogRecPtr RecPtr, bool randAccess,
 	 */
 	if (!InArchiveRecovery)
 		currentSource = XLOG_FROM_PG_WAL;
-	else if (currentSource == 0)
+	else if (currentSource == 0 || randAccess)
 		currentSource = XLOG_FROM_ARCHIVE;
 
 	for (;;)
-- 
2.18.2

Re: Allow auto_explain to log plans before queries are executed

[Kyotaro Horiguchi]

Hello.

At Thu, 27 Feb 2020 10:18:16 +0900, Yugo NAGATA  wrote in 
> On Wed, 26 Feb 2020 18:51:21 +0100
> Julien Rouhaud  wrote:
> 
> > On Thu, Feb 27, 2020 at 02:35:18AM +0900, Yugo NAGATA wrote:
> > > Hi,
> > >
> > > Attached is a patch for allowing auto_explain to log plans before
> > > queries are executed.
> > >
> > > Currently, auto_explain logs plans only after query executions,
> > > so if a query gets stuck its plan could not be logged. If we can
> > > know plans of stuck queries, we may get some hints to resolve the
> > > stuck. This is useful when you are testing and debugging your
> > > application whose queries get stuck in some situations.
> > 
> > Indeed that could be useful.
>
> > > This patch adds  new option log_before_query to auto_explain.
> > 
> > Maybe "log_before_execution" would be better?
> 
> Thanks!  This seems better also to me.
> 
> > 
> > > Setting auto_explain.log_before_query option logs all plans before
> > > queries are executed regardless of auto_explain.log_min_duration
> > > unless this is set -1 to disable logging.  If log_before_query is
> > > enabled, only duration time is logged after query execution as in
> > > the case of when both log_statement and log_min_duration_statement
> > > are enabled.
> > 
> > I'm not sure about this behavior.  The final explain plan is needed at 
> > least if
> > log_analyze, log_buffers or log_timing are enabled.
> 
> In the current patch, log_before_query (will be log_before_execution)
> has no effect if log_analyze is enabled in order to avoid to log the
> same plans twice.  Instead, is it better to log the plan always twice,
> before and after the execution, if  log_before_query is enabled
> regardless of log_min_duration or log_analyze?

Honestly, I don't think showing plans for all queries is useful
behavior.

If you allow the stuck query to be canceled, showing plan in
PG_FINALLY() block in explain_ExecutorRun would work, which look like
this.

explain_ExecutorRun()
{
  ...
  PG_TRY();
  {
  ...
  else
 starndard_ExecutorRun();
  nesting_level--;
  }
  PG_CATCH();
  {
  nesting_level--;

  if (auto_explain_log_failed_plan &&
   )
  {
  'show the plan'
  }
   }
}

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: Allow auto_explain to log plans before queries are executed

[Pavel Stehule]

čt 27. 2. 2020 v 6:16 odesílatel Kyotaro Horiguchi 
napsal:

> Hello.
>
> At Thu, 27 Feb 2020 10:18:16 +0900, Yugo NAGATA 
> wrote in
> > On Wed, 26 Feb 2020 18:51:21 +0100
> > Julien Rouhaud  wrote:
> >
> > > On Thu, Feb 27, 2020 at 02:35:18AM +0900, Yugo NAGATA wrote:
> > > > Hi,
> > > >
> > > > Attached is a patch for allowing auto_explain to log plans before
> > > > queries are executed.
> > > >
> > > > Currently, auto_explain logs plans only after query executions,
> > > > so if a query gets stuck its plan could not be logged. If we can
> > > > know plans of stuck queries, we may get some hints to resolve the
> > > > stuck. This is useful when you are testing and debugging your
> > > > application whose queries get stuck in some situations.
> > >
> > > Indeed that could be useful.
> >
> > > > This patch adds  new option log_before_query to auto_explain.
> > >
> > > Maybe "log_before_execution" would be better?
> >
> > Thanks!  This seems better also to me.
> >
> > >
> > > > Setting auto_explain.log_before_query option logs all plans before
> > > > queries are executed regardless of auto_explain.log_min_duration
> > > > unless this is set -1 to disable logging.  If log_before_query is
> > > > enabled, only duration time is logged after query execution as in
> > > > the case of when both log_statement and log_min_duration_statement
> > > > are enabled.
> > >
> > > I'm not sure about this behavior.  The final explain plan is needed at
> least if
> > > log_analyze, log_buffers or log_timing are enabled.
> >
> > In the current patch, log_before_query (will be log_before_execution)
> > has no effect if log_analyze is enabled in order to avoid to log the
> > same plans twice.  Instead, is it better to log the plan always twice,
> > before and after the execution, if  log_before_query is enabled
> > regardless of log_min_duration or log_analyze?
>
> Honestly, I don't think showing plans for all queries is useful
> behavior.
>
> If you allow the stuck query to be canceled, showing plan in
> PG_FINALLY() block in explain_ExecutorRun would work, which look like
> this.
>
> explain_ExecutorRun()
> {
>   ...
>   PG_TRY();
>   {
>   ...
>   else
>  starndard_ExecutorRun();
>   nesting_level--;
>   }
>   PG_CATCH();
>   {
>   nesting_level--;
>
>   if (auto_explain_log_failed_plan &&
>)
>   {
>   'show the plan'
>   }
>}
> }
>
> regards.
>

It can work - but still it is not good enough solution. We need "query
debugger" that allows to get some query execution metrics online.

There was a problem with memory management for passing plans between
processes. Can we used temp files instead shared memory?

Regards

Pavel


> --
> Kyotaro Horiguchi
> NTT Open Source Software Center
>
>
>

Re: Crash by targetted recovery

[Fujii Masao]

On 2020/02/27 12:48, Kyotaro Horiguchi wrote:

Hello.

We found that targetted promotion can cause an assertion failure.  The
attached TAP test causes that.


TRAP: FailedAssertion("StandbyMode", File: "xlog.c", Line: 12078)


After recovery target is reached, StartupXLOG turns off standby mode
then refetches the last record. If the last record starts from the
previous WAL segment, the assertion failure is triggered.


Good catch!


The wrong point is that StartupXLOG does random access fetching while
WaitForWALToBecomeAvailable is thinking it is still in streaming.  I
think if it is called with random access mode,
WaitForWALToBecomeAvailable should move to XLOG_FROM_ARCHIVE even
though it is thinking that it is still reading from stream.


I failed to understand why random access while reading from
stream is bad idea. Could you elaborate why?

Isn't it sufficient to set currentSource to 0 when disabling
StandbyMode?

Regards,

--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters

Re: Allow auto_explain to log plans before queries are executed

[Yugo NAGATA]

On Thu, 27 Feb 2020 14:14:41 +0900 (JST)
Kyotaro Horiguchi  wrote:

> Hello.
> 
> At Thu, 27 Feb 2020 10:18:16 +0900, Yugo NAGATA  wrote 
> in 
> > On Wed, 26 Feb 2020 18:51:21 +0100
> > Julien Rouhaud  wrote:
> > 
> > > On Thu, Feb 27, 2020 at 02:35:18AM +0900, Yugo NAGATA wrote:
> > > > Hi,
> > > >
> > > > Attached is a patch for allowing auto_explain to log plans before
> > > > queries are executed.
> > > >
> > > > Currently, auto_explain logs plans only after query executions,
> > > > so if a query gets stuck its plan could not be logged. If we can
> > > > know plans of stuck queries, we may get some hints to resolve the
> > > > stuck. This is useful when you are testing and debugging your
> > > > application whose queries get stuck in some situations.
> > > 
> > > Indeed that could be useful.
> >
> > > > This patch adds  new option log_before_query to auto_explain.
> > > 
> > > Maybe "log_before_execution" would be better?
> > 
> > Thanks!  This seems better also to me.
> > 
> > > 
> > > > Setting auto_explain.log_before_query option logs all plans before
> > > > queries are executed regardless of auto_explain.log_min_duration
> > > > unless this is set -1 to disable logging.  If log_before_query is
> > > > enabled, only duration time is logged after query execution as in
> > > > the case of when both log_statement and log_min_duration_statement
> > > > are enabled.
> > > 
> > > I'm not sure about this behavior.  The final explain plan is needed at 
> > > least if
> > > log_analyze, log_buffers or log_timing are enabled.
> > 
> > In the current patch, log_before_query (will be log_before_execution)
> > has no effect if log_analyze is enabled in order to avoid to log the
> > same plans twice.  Instead, is it better to log the plan always twice,
> > before and after the execution, if  log_before_query is enabled
> > regardless of log_min_duration or log_analyze?
> 
> Honestly, I don't think showing plans for all queries is useful
> behavior.
> 
> If you allow the stuck query to be canceled, showing plan in
> PG_FINALLY() block in explain_ExecutorRun would work, which look like
> this.
> 
> explain_ExecutorRun()
> {
>   ...
>   PG_TRY();
>   {
>   ...
>   else
>  starndard_ExecutorRun();
>   nesting_level--;
>   }
>   PG_CATCH();
>   {
>   nesting_level--;
> 
>   if (auto_explain_log_failed_plan &&
>)
>   {
>   'show the plan'
>   }
>}
> }

That makes sense. The initial purpose is to log plans of stuck queries
not of all queries, so your suggestion, doing it only when the query
fails, is reasonable.  I'll consider it little more.

Regards,
Yugo Nagata


-- 
Yugo NAGATA 

Re: Use compiler intrinsics for bit ops in hash

[David Fetter]

On Wed, Feb 26, 2020 at 09:12:24AM +0100, David Fetter wrote:
> On Fri, Jan 31, 2020 at 04:59:18PM +0100, David Fetter wrote:
> > On Wed, Jan 15, 2020 at 03:45:12PM -0800, Jesse Zhang wrote:
> > > On Tue, Jan 14, 2020 at 2:09 PM David Fetter  wrote:
> > > > > The changes in hash AM and SIMPLEHASH do look like a net positive
> > > > > improvement. My biggest cringe might be in pg_bitutils:
> > > > >
> > > > > 1. Is ceil_log2_64 dead code?
> > > >
> > > > Let's call it nascent code. I suspect there are places it could go, if
> > > > I look for them.  Also, it seemed silly to have one without the other.
> > > >
> > > 
> > > While not absolutely required, I'd like us to find at least one
> > > place and start using it. (Clang also nags at me when we have
> > > unused functions).
> > 
> > Done in the expanded patches attached.
> 
> These bit-rotted a little, so I've updated them.

05d8449e73694585b59f8b03aaa087f04cc4679a broke this patch set, so fix.

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

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
>From 2f8778f6133be23f6b7c375a39e9940ecc9fb063 Mon Sep 17 00:00:00 2001
From: David Fetter 
Date: Wed, 29 Jan 2020 02:09:59 -0800
Subject: [PATCH v6 1/3] de-long-ify
To: hackers
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="2.24.1"

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


diff --git a/src/backend/access/gist/gistbuildbuffers.c b/src/backend/access/gist/gistbuildbuffers.c
index 4b562d8d3f..482a569814 100644
--- a/src/backend/access/gist/gistbuildbuffers.c
+++ b/src/backend/access/gist/gistbuildbuffers.c
@@ -34,11 +34,11 @@ static void gistPlaceItupToPage(GISTNodeBufferPage *pageBuffer,
 IndexTuple item);
 static void gistGetItupFromPage(GISTNodeBufferPage *pageBuffer,
 IndexTuple *item);
-static long gistBuffersGetFreeBlock(GISTBuildBuffers *gfbb);
-static void gistBuffersReleaseBlock(GISTBuildBuffers *gfbb, long blocknum);
+static uint64 gistBuffersGetFreeBlock(GISTBuildBuffers *gfbb);
+static void gistBuffersReleaseBlock(GISTBuildBuffers *gfbb, uint64 blocknum);
 
-static void ReadTempFileBlock(BufFile *file, long blknum, void *ptr);
-static void WriteTempFileBlock(BufFile *file, long blknum, void *ptr);
+static void ReadTempFileBlock(BufFile *file, uint64 blknum, void *ptr);
+static void WriteTempFileBlock(BufFile *file, uint64 blknum, void *ptr);
 
 
 /*
@@ -64,7 +64,7 @@ gistInitBuildBuffers(int pagesPerBuffer, int levelStep, int maxLevel)
 	/* Initialize free page management. */
 	gfbb->nFreeBlocks = 0;
 	gfbb->freeBlocksLen = 32;
-	gfbb->freeBlocks = (long *) palloc(gfbb->freeBlocksLen * sizeof(long));
+	gfbb->freeBlocks = (int64 *) palloc(gfbb->freeBlocksLen * sizeof(int64));
 
 	/*
 	 * Current memory context will be used for all in-memory data structures
@@ -469,7 +469,7 @@ gistPopItupFromNodeBuffer(GISTBuildBuffers *gfbb, GISTNodeBuffer *nodeBuffer,
 /*
  * Select a currently unused block for writing to.
  */
-static long
+static uint64
 gistBuffersGetFreeBlock(GISTBuildBuffers *gfbb)
 {
 	/*
@@ -487,7 +487,7 @@ gistBuffersGetFreeBlock(GISTBuildBuffers *gfbb)
  * Return a block# to the freelist.
  */
 static void
-gistBuffersReleaseBlock(GISTBuildBuffers *gfbb, long blocknum)
+gistBuffersReleaseBlock(GISTBuildBuffers *gfbb, uint64 blocknum)
 {
 	int			ndx;
 
@@ -495,9 +495,9 @@ gistBuffersReleaseBlock(GISTBuildBuffers *gfbb, long blocknum)
 	if (gfbb->nFreeBlocks >= gfbb->freeBlocksLen)
 	{
 		gfbb->freeBlocksLen *= 2;
-		gfbb->freeBlocks = (long *) repalloc(gfbb->freeBlocks,
+		gfbb->freeBlocks = (int64 *) repalloc(gfbb->freeBlocks,
 			 gfbb->freeBlocksLen *
-			 sizeof(long));
+			 sizeof(uint64));
 	}
 
 	/* Add blocknum to array */
@@ -755,7 +755,7 @@ gistRelocateBuildBuffersOnSplit(GISTBuildBuffers *gfbb, GISTSTATE *giststate,
  */
 
 static void
-ReadTempFileBlock(BufFile *file, long blknum, void *ptr)
+ReadTempFileBlock(BufFile *file, uint64 blknum, void *ptr)
 {
 	if (BufFileSeekBlock(file, blknum) != 0)
 		elog(ERROR, "could not seek temporary file: %m");
@@ -764,7 +764,7 @@ ReadTempFileBlock(BufFile *file, long blknum, void *ptr)
 }
 
 static void
-WriteTempFileBlock(BufFile *file, long blknum, void *ptr)
+WriteTempFileBlock(BufFile *file, uint64 blknum, void *ptr)
 {
 	if (BufFileSeekBlock(file, blknum) != 0)
 		elog(ERROR, "could not seek temporary file: %m");
diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index c46764bf42..4fc478640a 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -70,7 +70,7 @@ static int	_SPI_pquery(QueryDesc *queryDesc, bool fire_triggers, uint64 tcount);
 static void _SPI_error_callback(void *arg);
 
 static void _SPI_cursor_operation(Portal portal,
-  FetchDirection direction, long count,
+  FetchDirection direction, ui

Re: Allow auto_explain to log plans before queries are executed

[Kyotaro Horiguchi]

At Thu, 27 Feb 2020 06:27:24 +0100, Pavel Stehule  
wrote in 
> odesílatel Kyotaro Horiguchi 
> napsal:
> > > In the current patch, log_before_query (will be log_before_execution)
> > > has no effect if log_analyze is enabled in order to avoid to log the
> > > same plans twice.  Instead, is it better to log the plan always twice,
> > > before and after the execution, if  log_before_query is enabled
> > > regardless of log_min_duration or log_analyze?
> >
> > Honestly, I don't think showing plans for all queries is useful
> > behavior.
> >
> > If you allow the stuck query to be canceled, showing plan in
> > PG_FINALLY() block in explain_ExecutorRun would work, which look like
> > this.
...
> It can work - but still it is not good enough solution. We need "query
> debugger" that allows to get some query execution metrics online.

If we need a live plan dump of a running query, We could do that using
some kind of inter-backend triggering. (I'm not sure if PG offers
inter-backend signalling facility usable by extensions..)

=# select auto_explain.log_plan_backend(12345);

postgresql.log:
 LOG: requested plan dump: ..



> There was a problem with memory management for passing plans between
> processes. Can we used temp files instead shared memory?

=# select auto_explain.dump_plan_backend(12345);
  pid  | query   | plan
---+-+---
 12345 | SELECT 1;   | Result (cost=) (actual..)
(1 row)

Doesn't DSA work?  I think it would be easier to handle than files.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: Allow auto_explain to log plans before queries are executed

[Yugo NAGATA]

On Thu, 27 Feb 2020 06:27:24 +0100
Pavel Stehule  wrote:

> čt 27. 2. 2020 v 6:16 odesílatel Kyotaro Horiguchi 
> napsal:
> 
> > Hello.
> >
> > At Thu, 27 Feb 2020 10:18:16 +0900, Yugo NAGATA 
> > wrote in
> > > On Wed, 26 Feb 2020 18:51:21 +0100
> > > Julien Rouhaud  wrote:
> > >
> > > > On Thu, Feb 27, 2020 at 02:35:18AM +0900, Yugo NAGATA wrote:
> > > > > Hi,
> > > > >
> > > > > Attached is a patch for allowing auto_explain to log plans before
> > > > > queries are executed.
> > > > >
> > > > > Currently, auto_explain logs plans only after query executions,
> > > > > so if a query gets stuck its plan could not be logged. If we can
> > > > > know plans of stuck queries, we may get some hints to resolve the
> > > > > stuck. This is useful when you are testing and debugging your
> > > > > application whose queries get stuck in some situations.
> > > >
> > > > Indeed that could be useful.
> > >
> > > > > This patch adds  new option log_before_query to auto_explain.
> > > >
> > > > Maybe "log_before_execution" would be better?
> > >
> > > Thanks!  This seems better also to me.
> > >
> > > >
> > > > > Setting auto_explain.log_before_query option logs all plans before
> > > > > queries are executed regardless of auto_explain.log_min_duration
> > > > > unless this is set -1 to disable logging.  If log_before_query is
> > > > > enabled, only duration time is logged after query execution as in
> > > > > the case of when both log_statement and log_min_duration_statement
> > > > > are enabled.
> > > >
> > > > I'm not sure about this behavior.  The final explain plan is needed at
> > least if
> > > > log_analyze, log_buffers or log_timing are enabled.
> > >
> > > In the current patch, log_before_query (will be log_before_execution)
> > > has no effect if log_analyze is enabled in order to avoid to log the
> > > same plans twice.  Instead, is it better to log the plan always twice,
> > > before and after the execution, if  log_before_query is enabled
> > > regardless of log_min_duration or log_analyze?
> >
> > Honestly, I don't think showing plans for all queries is useful
> > behavior.
> >
> > If you allow the stuck query to be canceled, showing plan in
> > PG_FINALLY() block in explain_ExecutorRun would work, which look like
> > this.
> >
> > explain_ExecutorRun()
> > {
> >   ...
> >   PG_TRY();
> >   {
> >   ...
> >   else
> >  starndard_ExecutorRun();
> >   nesting_level--;
> >   }
> >   PG_CATCH();
> >   {
> >   nesting_level--;
> >
> >   if (auto_explain_log_failed_plan &&
> >)
> >   {
> >   'show the plan'
> >   }
> >}
> > }
> >
> > regards.
> >
> 
> It can work - but still it is not good enough solution. We need "query
> debugger" that allows to get some query execution metrics online.
> 
> There was a problem with memory management for passing plans between
> processes. Can we used temp files instead shared memory?

 I think "query debugger" feature you proposed is out of scope of
auto_explain module. I also think the feature to analyze running
query online is great, but we will need another discussion on a new
module or eature for it.

Regards,
Yugo Nagata

-- 
Yugo NAGATA 

Re: [HACKERS] [WIP] Effective storage of duplicates in B-tree index.

[Fujii Masao]

On 2020/02/27 7:43, Peter Geoghegan wrote:

On Mon, Feb 24, 2020 at 4:54 PM Peter Geoghegan  wrote:

Attached is v34, which has this change. My plan is to commit something
very close to this on Wednesday morning (barring any objections).


Pushed.


Thanks for committing this nice feature!

Here is one minor comment.

+  deduplicate_items
+  storage parameter

This should be

deduplicate_items storage parameter

 for reloption is necessary only when the GUC parameter
with the same name of the reloption exists. So, for example, you can
see that  is used in vacuum_cleanup_index_scale_factor
but not in buffering reloption.

Regards,

--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters

Re: Implementing Incremental View Maintenance

[Takuma Hoshiai]

Hi, 

Attached is the latest patch (v14) to add support for Incremental Materialized
View Maintenance (IVM). It is possible to apply to current latest master branch.

Differences from the previous patch (v13) include:

* Support base tables using RLS

If a table has the Row Level Security (RLS) policy, IMMV is updated based on
the view owner's policy when a base table is updated. However, when a policy
of base table is changed or created after creating IMMV, IMMV is not updated
based on the new RLS policy. In this case, REFRESH command must be executed.

* Use ENR instead of temporary tables for internal operation

Previously, IVM create and use a temporary tables to store view delta rows.
However it caused out of shared memory, and Tom Lane pointed out that 
using temp tables in IVM trigger is not good.

Currently, IVM uses tuplestores and ephemeral named relation (ENR) instead
of temporary tables. it doesn't cause previous problem like below:

testdb=# create table b1 (id integer, x numeric(10,3));
CREATE TABLE
testdb=# create incremental materialized view mv1 
testdb-# as select id, count(*),sum(x) from b1 group by id;
SELECT 0
testdb=# 
testdb=# do $$ 
testdb$# declare 
testdb$# i integer;
testdb$# begin 
testdb$# for i in 1..1 
testdb$# loop 
testdb$# insert into b1 values (1,1); 
testdb$# end loop; 
testdb$# end;
testdb$# $$
testdb-# ;
DO
testdb=# 

This issue is reported by PAscal.
https://www.postgresql.org/message-id/1577564109604-0.p...@n3.nabble.com


* Support pg_dump/pg_restore for IVM

IVM supports pg_dump/pg_restore command.

* Prohibit rename and unique index creation on IVM columns

When a user make a unique index on ivm columns such as ivm_count, IVM will fail 
due to
the unique constraint violation, so IVM prohibits it.
Also, rename of these columns also causes IVM fails, so IVM prohibits it too.

* Fix incorrect WHERE condition check for outer-join views

The check for non null-rejecting condition check was incorrect.

Best Regards,
Takuma Hoshiai

-- 
Takuma Hoshiai 


IVM_patches_v14.tar.gz
Description: Binary data

Re: Allow auto_explain to log plans before queries are executed

[Pavel Stehule]

čt 27. 2. 2020 v 6:58 odesílatel Kyotaro Horiguchi 
napsal:

> At Thu, 27 Feb 2020 06:27:24 +0100, Pavel Stehule 
> wrote in
> > odesílatel Kyotaro Horiguchi 
> > napsal:
> > > > In the current patch, log_before_query (will be log_before_execution)
> > > > has no effect if log_analyze is enabled in order to avoid to log the
> > > > same plans twice.  Instead, is it better to log the plan always
> twice,
> > > > before and after the execution, if  log_before_query is enabled
> > > > regardless of log_min_duration or log_analyze?
> > >
> > > Honestly, I don't think showing plans for all queries is useful
> > > behavior.
> > >
> > > If you allow the stuck query to be canceled, showing plan in
> > > PG_FINALLY() block in explain_ExecutorRun would work, which look like
> > > this.
> ...
> > It can work - but still it is not good enough solution. We need "query
> > debugger" that allows to get some query execution metrics online.
>
> If we need a live plan dump of a running query, We could do that using
> some kind of inter-backend triggering. (I'm not sure if PG offers
> inter-backend signalling facility usable by extensions..)
>
> =# select auto_explain.log_plan_backend(12345);
>
> postgresql.log:
>  LOG: requested plan dump: ..
>
>
>
> > There was a problem with memory management for passing plans between
> > processes. Can we used temp files instead shared memory?
>
> =# select auto_explain.dump_plan_backend(12345);
>   pid  | query   | plan
> ---+-+---
>  12345 | SELECT 1;   | Result (cost=) (actual..)
> (1 row)
>
> Doesn't DSA work?  I think it would be easier to handle than files.
>

I am not sure. There is hard questions when the allocated shared memory
should be  deallocated.

Maybe using third process can be the most nice, safe solution.

The execution plans can be pushed to some background worker memory, and
this process can works like stats_collector.



> regards.
>
> --
> Kyotaro Horiguchi
> NTT Open Source Software Center
>

Re: Allow auto_explain to log plans before queries are executed

[Pavel Stehule]

čt 27. 2. 2020 v 7:01 odesílatel Yugo NAGATA  napsal:

> On Thu, 27 Feb 2020 06:27:24 +0100
> Pavel Stehule  wrote:
>
> > čt 27. 2. 2020 v 6:16 odesílatel Kyotaro Horiguchi <
> horikyota@gmail.com>
> > napsal:
> >
> > > Hello.
> > >
> > > At Thu, 27 Feb 2020 10:18:16 +0900, Yugo NAGATA 
> > > wrote in
> > > > On Wed, 26 Feb 2020 18:51:21 +0100
> > > > Julien Rouhaud  wrote:
> > > >
> > > > > On Thu, Feb 27, 2020 at 02:35:18AM +0900, Yugo NAGATA wrote:
> > > > > > Hi,
> > > > > >
> > > > > > Attached is a patch for allowing auto_explain to log plans before
> > > > > > queries are executed.
> > > > > >
> > > > > > Currently, auto_explain logs plans only after query executions,
> > > > > > so if a query gets stuck its plan could not be logged. If we can
> > > > > > know plans of stuck queries, we may get some hints to resolve the
> > > > > > stuck. This is useful when you are testing and debugging your
> > > > > > application whose queries get stuck in some situations.
> > > > >
> > > > > Indeed that could be useful.
> > > >
> > > > > > This patch adds  new option log_before_query to auto_explain.
> > > > >
> > > > > Maybe "log_before_execution" would be better?
> > > >
> > > > Thanks!  This seems better also to me.
> > > >
> > > > >
> > > > > > Setting auto_explain.log_before_query option logs all plans
> before
> > > > > > queries are executed regardless of auto_explain.log_min_duration
> > > > > > unless this is set -1 to disable logging.  If log_before_query is
> > > > > > enabled, only duration time is logged after query execution as in
> > > > > > the case of when both log_statement and
> log_min_duration_statement
> > > > > > are enabled.
> > > > >
> > > > > I'm not sure about this behavior.  The final explain plan is
> needed at
> > > least if
> > > > > log_analyze, log_buffers or log_timing are enabled.
> > > >
> > > > In the current patch, log_before_query (will be log_before_execution)
> > > > has no effect if log_analyze is enabled in order to avoid to log the
> > > > same plans twice.  Instead, is it better to log the plan always
> twice,
> > > > before and after the execution, if  log_before_query is enabled
> > > > regardless of log_min_duration or log_analyze?
> > >
> > > Honestly, I don't think showing plans for all queries is useful
> > > behavior.
> > >
> > > If you allow the stuck query to be canceled, showing plan in
> > > PG_FINALLY() block in explain_ExecutorRun would work, which look like
> > > this.
> > >
> > > explain_ExecutorRun()
> > > {
> > >   ...
> > >   PG_TRY();
> > >   {
> > >   ...
> > >   else
> > >  starndard_ExecutorRun();
> > >   nesting_level--;
> > >   }
> > >   PG_CATCH();
> > >   {
> > >   nesting_level--;
> > >
> > >   if (auto_explain_log_failed_plan &&
> > >)
> > >   {
> > >   'show the plan'
> > >   }
> > >}
> > > }
> > >
> > > regards.
> > >
> >
> > It can work - but still it is not good enough solution. We need "query
> > debugger" that allows to get some query execution metrics online.
> >
> > There was a problem with memory management for passing plans between
> > processes. Can we used temp files instead shared memory?
>
>  I think "query debugger" feature you proposed is out of scope of
> auto_explain module. I also think the feature to analyze running
> query online is great, but we will need another discussion on a new
> module or eature for it.
>

sure. My note was about using auto_explain like query_debugger. It has not
too sense, and from this perspective, the original proposal to log plan
before execution has more sense.

you can log every plan with higher cost than some constant.



> Regards,
> Yugo Nagata
>
> --
> Yugo NAGATA 
>
>
>

Re: Crash by targetted recovery

[Kyotaro Horiguchi]

At Thu, 27 Feb 2020 14:40:55 +0900, Fujii Masao  
wrote in 
> 
> 
> On 2020/02/27 12:48, Kyotaro Horiguchi wrote:
> > Hello.
> > We found that targetted promotion can cause an assertion failure.  The
> > attached TAP test causes that.
> > 
> >> TRAP: FailedAssertion("StandbyMode", File: "xlog.c", Line: 12078)
> > After recovery target is reached, StartupXLOG turns off standby mode
> > then refetches the last record. If the last record starts from the
> > previous WAL segment, the assertion failure is triggered.
> 
> Good catch!
> 
> > The wrong point is that StartupXLOG does random access fetching while
> > WaitForWALToBecomeAvailable is thinking it is still in streaming.  I
> > think if it is called with random access mode,
> > WaitForWALToBecomeAvailable should move to XLOG_FROM_ARCHIVE even
> > though it is thinking that it is still reading from stream.
> 
> I failed to understand why random access while reading from
> stream is bad idea. Could you elaborate why?

It seems to me the word "streaming" suggests that WAL record should be
read sequentially. Random access, which means reading from arbitrary
location, breaks a stream.  (But the patch doesn't try to stop wal
sender if randAccess.)

> Isn't it sufficient to set currentSource to 0 when disabling
> StandbyMode?

I thought that and it should work, but I hesitated to manipulate on
currentSource in StartupXLOG. currentSource is basically a private
state of WaitForWALToBecomeAvailable. ReadRecord modifies it but I
think it's not good to modify it out of the the logic in
WaitForWALToBecomeAvailable.  Come to think of that I got to think the
following part in ReadRecord should use randAccess instead..

xlog.c:4384
> /*
-  * Before we retry, reset lastSourceFailed and currentSource
-  * so that we will check the archive next.
+  * Streaming has broken, we retry from the same LSN.
>  */
> lastSourceFailed = false;
- currentSource = 0;
+ private->randAccess = true;

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: Allow auto_explain to log plans before queries are executed

[Julien Rouhaud]

On Thu, Feb 27, 2020 at 7:12 AM Pavel Stehule  wrote:
>
> čt 27. 2. 2020 v 7:01 odesílatel Yugo NAGATA  napsal:
>>
>> On Thu, 27 Feb 2020 06:27:24 +0100
>> Pavel Stehule  wrote:
>>
>> > čt 27. 2. 2020 v 6:16 odesílatel Kyotaro Horiguchi 
>> > 
>> > napsal:
>> >
>> > > Hello.
>> > >
>> > > At Thu, 27 Feb 2020 10:18:16 +0900, Yugo NAGATA 
>> > > wrote in
>> > > > On Wed, 26 Feb 2020 18:51:21 +0100
>> > > > Julien Rouhaud  wrote:
>> > > >
>> > > > > On Thu, Feb 27, 2020 at 02:35:18AM +0900, Yugo NAGATA wrote:
>> > > > > > Hi,
>> > > > > >
>> > > > > > Attached is a patch for allowing auto_explain to log plans before
>> > > > > > queries are executed.
>> > > > > >
>> > > > > > Currently, auto_explain logs plans only after query executions,
>> > > > > > so if a query gets stuck its plan could not be logged. If we can
>> > > > > > know plans of stuck queries, we may get some hints to resolve the
>> > > > > > stuck. This is useful when you are testing and debugging your
>> > > > > > application whose queries get stuck in some situations.
>> > > > >
>> > > > > Indeed that could be useful.
>> > > >
>> > > > > > This patch adds  new option log_before_query to auto_explain.
>> > > > >
>> > > > > Maybe "log_before_execution" would be better?
>> > > >
>> > > > Thanks!  This seems better also to me.
>> > > >
>> > > > >
>> > > > > > Setting auto_explain.log_before_query option logs all plans before
>> > > > > > queries are executed regardless of auto_explain.log_min_duration
>> > > > > > unless this is set -1 to disable logging.  If log_before_query is
>> > > > > > enabled, only duration time is logged after query execution as in
>> > > > > > the case of when both log_statement and log_min_duration_statement
>> > > > > > are enabled.
>> > > > >
>> > > > > I'm not sure about this behavior.  The final explain plan is needed 
>> > > > > at
>> > > least if
>> > > > > log_analyze, log_buffers or log_timing are enabled.
>> > > >
>> > > > In the current patch, log_before_query (will be log_before_execution)
>> > > > has no effect if log_analyze is enabled in order to avoid to log the
>> > > > same plans twice.  Instead, is it better to log the plan always twice,
>> > > > before and after the execution, if  log_before_query is enabled
>> > > > regardless of log_min_duration or log_analyze?
>> > >
>> > > Honestly, I don't think showing plans for all queries is useful
>> > > behavior.
>> > >
>> > > If you allow the stuck query to be canceled, showing plan in
>> > > PG_FINALLY() block in explain_ExecutorRun would work, which look like
>> > > this.
>> > >
>> > > explain_ExecutorRun()
>> > > {
>> > >   ...
>> > >   PG_TRY();
>> > >   {
>> > >   ...
>> > >   else
>> > >  starndard_ExecutorRun();
>> > >   nesting_level--;
>> > >   }
>> > >   PG_CATCH();
>> > >   {
>> > >   nesting_level--;
>> > >
>> > >   if (auto_explain_log_failed_plan &&
>> > >)
>> > >   {
>> > >   'show the plan'
>> > >   }
>> > >}
>> > > }
>> > >
>> > > regards.
>> > >
>> >
>> > It can work - but still it is not good enough solution. We need "query
>> > debugger" that allows to get some query execution metrics online.
>> >
>> > There was a problem with memory management for passing plans between
>> > processes. Can we used temp files instead shared memory?
>>
>>  I think "query debugger" feature you proposed is out of scope of
>> auto_explain module. I also think the feature to analyze running
>> query online is great, but we will need another discussion on a new
>> module or eature for it.
>
>
> sure. My note was about using auto_explain like query_debugger. It has not 
> too sense, and from this perspective, the original proposal to log plan 
> before execution has more sense.
>
> you can log every plan with higher cost than some constant.

Yes I thought about that too.  If you're not in an OLAP environment
(or with a specific user running few expensive queries), setup an
auto_explain.log_before_execution_min_cost.

Re: Incremental View Maintenance: ERROR: out of shared memory

[Takuma Hoshiai]

Hi,

On Fri, 17 Jan 2020 17:33:48 +0900
Yugo NAGATA  wrote:

> On Sun, 29 Dec 2019 12:27:13 -0500
> Tom Lane  wrote:
> 
> > Tatsuo Ishii  writes:
> > >> here is an unexpected error found while testing IVM v11 patches
> > >> ...
> > >> ERROR:  out of shared memory
> > 
> > > I think we could avoid such an error in IVM by reusing a temp table in
> > > a session or a transaction.
> > 
> > I'm more than a little bit astonished that this proposed patch is
> > creating temp tables at all.  ISTM that that implies that it's
> > being implemented at the wrong level of abstraction, and it will be
> > full of security problems, as well as performance problems above
> > and beyond the one described here.
> 
> We realized that there is also other problems in using temp tables
> as pointed out in another thread. So, we are now working on rewrite
> our patch not to use temp tables.

We fixed this problem in latest patches (v14) in the following thread.
https://www.postgresql.org/message-id/20200227150649.101ef342d0e7d7abee320...@sraoss.co.jp

We would appreciate it if you could review this.


Best Regards,

Takuma Hoshiai


> Regards,
> Yugo Nagata
> 
> 
> -- 
> Yugo NAGATA 
> 
> 
> 


-- 
Takuma Hoshiai 

Re: Use compiler intrinsics for bit ops in hash

[John Naylor]

On Thu, Feb 27, 2020 at 1:56 PM David Fetter  wrote:
> [v6 set]

Hi David,

In 0002, the pg_bitutils functions have a test (input > 0), and the
new callers ceil_log2_* and next_power_of_2_* have asserts. That seems
backward to me. I imagine some callers of bitutils will already know
the value > 0, and it's probably good to keep that branch out of the
lowest level functions. What do you think?

-- 
John Naylorhttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [Patch] Make pg_checksums skip foreign tablespace directories

[Michael Paquier]

On Wed, Feb 26, 2020 at 06:02:22PM +0100, Bernd Helmle wrote:
> My feeling is that in the case we cannot successfully resolve a
> tablespace location from pg_tblspc, we should error out, but i could
> imagine that people would like to have just a warning instead.

Thanks, this patch is much cleaner in its approach, and I don't have
much to say about it except that the error message for lstat() should
be more consistent with the one above in scan_directory().  The
version for v11 has required a bit of rework, but nothing huge
either.

> I've updated the TAP test for pg_checksums by adding a dummy
> subdirectory into the tablespace directory already created for the
> corrupted relfilenode test, containing a file to process in case an
> unpatched pg_checksums is run. With the patch attached, these
> directories simply won't be considered to check.

What you have here is much more simple than the original proposal, so
I kept it.  Applied and back-patched down to 11.
--
Michael


signature.asc
Description: PGP signature

Re: [HACKERS] WAL logging problem in 9.4.3?

[Kyotaro Horiguchi]

At Tue, 25 Feb 2020 21:36:12 -0800, Noah Misch  wrote in 
> On Tue, Feb 25, 2020 at 10:01:51AM +0900, Kyotaro Horiguchi wrote:
> > At Sat, 22 Feb 2020 21:12:20 -0800, Noah Misch  wrote in 
> > > On Fri, Feb 21, 2020 at 04:49:59PM +0900, Kyotaro Horiguchi wrote:
> > I aggree that the new #ifdef can invite a Heisenbug. I thought that
> > you didn't want that because it doesn't make substantial difference.
> 
> v35nm added swap_relation_files() code so AssertPendingSyncs_RelationCache()
> could check rd_droppedSubid relations.  v30nm, which did not have
> rd_droppedSubid, removed swap_relation_files() code that wasn't making a
> difference.

Ok, I understand that it meant that the additional code still makes
difference in --enable-cassert build.

> > If we decide to keep the consistency there, I would like to describe
> > the code is there for consistency, not for the benefit of a specific
> > assertion.
> > 
> > (cluster.c:1116)
> > -* new. The next step for rel2 is deletion, but copy rd_*Subid for the
> > -* benefit of AssertPendingSyncs_RelationCache().
> > +* new. The next step for rel2 is deletion, but copy rd_*Subid for the
> > +* consistency of the fieles. It is checked later by
> > +* AssertPendingSyncs_RelationCache().
> 
> I think the word "consistency" is too vague for "consistency of the fields" to
> convey information.  May I just remove the last sentence of the comment
> (everything after "* new.")?

I'm fine with that:)

> > I agree that relation works as the generic name of table-like
> > objects. Addition to that, doesn't using the word "storage file" make
> > it more clearly?  I'm not confident on the wording itself, but it will
> > look like the following.
> 
> The docs rarely use "storage file" or "on-disk file" as terms.  I hesitate to
> put more emphasis on files, because they are part of the implementation, not
> part of the user interface.  The term "rewrites"/"rewriting" has the same
> problem, though.  Yet another alternative would be to talk about operations
> that change the pg_relation_filenode() return value:
> 
>   In minimal level, no information is logged for permanent
>   relations for the remainder of a transaction that creates them or changes
>   what pg_relation_filenode returns for them.
> 
> What do you think?

It sounds somewhat obscure. Coulnd't we enumetate examples? And if we
could use pg_relation_filenode, I think we can use just
"filenode". (Thuogh the word is used in the documentation, it is not
defined anywhere..)


In minimal level, no information is logged for
permanent relations for the remainder of a transaction that creates
them or changes their filenode. For example, CREATE
TABLE, CLUSTER or REFRESH MATERIALIZED VIEW are the command of that
category.


# sorry for bothering you..

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: Crash by targetted recovery

[Fujii Masao]

On 2020/02/27 15:23, Kyotaro Horiguchi wrote:

At Thu, 27 Feb 2020 14:40:55 +0900, Fujii Masao  
wrote in



On 2020/02/27 12:48, Kyotaro Horiguchi wrote:

Hello.
We found that targetted promotion can cause an assertion failure.  The
attached TAP test causes that.


TRAP: FailedAssertion("StandbyMode", File: "xlog.c", Line: 12078)

After recovery target is reached, StartupXLOG turns off standby mode
then refetches the last record. If the last record starts from the
previous WAL segment, the assertion failure is triggered.


Good catch!


The wrong point is that StartupXLOG does random access fetching while
WaitForWALToBecomeAvailable is thinking it is still in streaming.  I
think if it is called with random access mode,
WaitForWALToBecomeAvailable should move to XLOG_FROM_ARCHIVE even
though it is thinking that it is still reading from stream.


I failed to understand why random access while reading from
stream is bad idea. Could you elaborate why?


It seems to me the word "streaming" suggests that WAL record should be
read sequentially. Random access, which means reading from arbitrary
location, breaks a stream.  (But the patch doesn't try to stop wal
sender if randAccess.)


Isn't it sufficient to set currentSource to 0 when disabling
StandbyMode?


I thought that and it should work, but I hesitated to manipulate on
currentSource in StartupXLOG. currentSource is basically a private
state of WaitForWALToBecomeAvailable. ReadRecord modifies it but I
think it's not good to modify it out of the the logic in
WaitForWALToBecomeAvailable.


If so, what about adding the following at the top of
WaitForWALToBecomeAvailable()?

if (!StandbyMode && currentSource == XLOG_FROM_STREAM)
 currentSource = 0;


Come to think of that I got to think the
following part in ReadRecord should use randAccess instead..

xlog.c:4384

 /*

-  * Before we retry, reset lastSourceFailed and currentSource
-  * so that we will check the archive next.
+  * Streaming has broken, we retry from the same LSN.

  */
 lastSourceFailed = false;

- currentSource = 0;
+ private->randAccess = true;


Sorry, I failed to understand why this change is necessary...
At least the comment that you added seems incorrect
because WAL streaming should not have started yet when
we reach the above point.

Regards,

--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters

Re: reindex concurrently and two toast indexes

[Michael Paquier]

On Sat, Feb 22, 2020 at 04:06:57PM +0100, Julien Rouhaud wrote:
> Sorry, I just realized that I forgot to commit the last changes before sending
> the patch, so here's the correct v2.

Thanks for the patch.

> + if (skipit)
> + {
> + ereport(NOTICE,
> +  (errmsg("skipping invalid index \"%s.%s\"",
> +  
> get_namespace_name(get_rel_namespace(indexOid)),
> +  get_rel_name(indexOid;

ReindexRelationConcurrently() issues a WARNING when bumping on an
invalid index, shouldn't the same log level be used?

Even with this patch, it is possible to reindex an invalid toast index
with REINDEX INDEX (with and without CONCURRENTLY), which is the
problem I mentioned upthread (Er, actually only for the non-concurrent
case as told about reindex_index).  Shouldn't both cases be prevented
as well with an ERROR?
--
Michael


signature.asc
Description: PGP signature