Re: min_safe_lsn column in pg_replication_slots view

[Fujii Masao]

On 2020/06/19 10:39, Michael Paquier wrote:

On Fri, Jun 19, 2020 at 10:02:54AM +0900, Kyotaro Horiguchi wrote:

At Thu, 18 Jun 2020 18:18:37 +0530, Amit Kapila  wrote 
in

It is a little unclear to me how this or any proposed patch will solve
the original problem reported by Fujii-San?  Basically, the problem
arises because we don't have an interlock between when the checkpoint
removes the WAL segment and the view tries to acquire the same.  Am, I
missing something?


The proposed patch fetches the computation of the minimum LSN across
all slots before taking ReplicationSlotControlLock so its value gets
more lossy, and potentially older than what the slots actually
include.  So it is an attempt to take the safest spot possible.

Honestly, I find a bit silly the design to compute and use the same
minimum LSN value for all the tuples returned by
pg_get_replication_slots, and you can actually get a pretty good
estimate of that by emulating ReplicationSlotsComputeRequiredLSN()
directly with what pg_replication_slot provides as we have a min()
aggregate for pg_lsn.

For these reasons, I think that we should remove for now this
information from the view, and reconsider this part more carefully for
14~ with a clear definition of how much lossiness we are ready to
accept for the information provided here, if necessary.


Agreed. But isn't it too late to remove the columns (i.e., change
the catalog) for v13? Because v13 beta1 was already released.
IIUC the catalog should not be changed since beta1 release so that
users can upgrade PostgreSQL without initdb.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

Re: min_safe_lsn column in pg_replication_slots view

[Michael Paquier]

On Fri, Jun 19, 2020 at 04:13:27PM +0900, Fujii Masao wrote:
> Agreed. But isn't it too late to remove the columns (i.e., change
> the catalog) for v13? Because v13 beta1 was already released.
> IIUC the catalog should not be changed since beta1 release so that
> users can upgrade PostgreSQL without initdb.

Catalog bumps have happened in the past between beta versions:
git log -p REL_12_BETA1..REL_12_BETA2 src/include/catalog/catversion.h
git log -p REL_11_BETA1..REL_11_BETA2 src/include/catalog/catversion.h
git log -p REL_10_BETA1..REL_10_BETA2 src/include/catalog/catversion.h

So we usually avoid to do that between betas, but my take here is that
a catalog bump is better than regretting a change we may have to live
with after the release is sealed.
--
Michael


signature.asc
Description: PGP signature

Re: min_safe_lsn column in pg_replication_slots view

[Kyotaro Horiguchi]

At Fri, 19 Jun 2020 16:36:09 +0900, Michael Paquier  wrote 
in 
> On Fri, Jun 19, 2020 at 04:13:27PM +0900, Fujii Masao wrote:
> > Agreed. But isn't it too late to remove the columns (i.e., change
> > the catalog) for v13? Because v13 beta1 was already released.
> > IIUC the catalog should not be changed since beta1 release so that
> > users can upgrade PostgreSQL without initdb.
> 
> Catalog bumps have happened in the past between beta versions:
> git log -p REL_12_BETA1..REL_12_BETA2 src/include/catalog/catversion.h
> git log -p REL_11_BETA1..REL_11_BETA2 src/include/catalog/catversion.h
> git log -p REL_10_BETA1..REL_10_BETA2 src/include/catalog/catversion.h
> 
> So we usually avoid to do that between betas, but my take here is that
> a catalog bump is better than regretting a change we may have to live
> with after the release is sealed.

FWIW if we decide that it is really useless, I agree to remove it now.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

doing something about the broken dynloader.h symlink

[Peter Eisentraut]

When you switch a built REL_11_STABLE or earlier to REL_12_STABLE or 
later, you get during make install (or, therefore, make check) an error


cp ./*.h 'PREFIX/include/server'/
cp: cannot stat './dynloader.h': No such file or directory

This is because version 11 and earlier created src/include/dynloader.h 
as a symlink during configure, but this was changed in version 12, and 
the target that the symlink points to is no longer there in that branch.


This has been known for some time, and I figured the issue would go 
away, but people keep complaining to me, so maybe a simple fix could be 
applied.


Even if it's quite late to fix this, it's perhaps worth establishing a 
principled solution, in case we ever change any of the other symlinks 
currently created by configure.


It is worth noting that half the problem is that the cp command uses 
wildcards, where in a puristic situation all the files would be listed 
explicitly and any extra files left around would not pose problems. 
However, it seems generally bad to leave broken symlinks lying around, 
since that can also trip up other tools.


My proposed fix is to apply this patch:

diff --git a/configure.in b/configure.in
index 7d63eb2fa3..84221690e0 100644
--- a/configure.in
+++ b/configure.in
@@ -2474,7 +2474,10 @@ AC_CONFIG_LINKS([
   src/backend/port/pg_shmem.c:${SHMEM_IMPLEMENTATION}
   src/include/pg_config_os.h:src/include/port/${template}.h
   src/Makefile.port:src/makefiles/Makefile.${template}
-])
+], [],
+[# Remove links created by old versions of configure, so that there
+# are no broken symlinks in the tree
+rm -f src/include/dynloader.h])

 if test "$PORTNAME" = "win32"; then
 AC_CONFIG_COMMANDS([check_win32_symlinks],[

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

Re: More tzdb fun: POSIXRULES is being deprecated upstream

[Peter Eisentraut]

On 2020-06-17 20:08, Tom Lane wrote:

I would definitely be in favor of "nuke it now" with respect to HEAD.
It's a bit more debatable for the back branches.  However, all branches
are going to be equally exposed to updated system tzdata trees, so
we've typically felt that changes in the tz-related code should be
back-patched.


It seems sensible to me to remove it in master and possibly 
REL_13_STABLE, but leave it alone in the back branches.


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

Re: Global snapshots

[Andrey V. Lepikhov]

On 6/19/20 11:48 AM, Amit Kapila wrote:

On Wed, Jun 10, 2020 at 8:36 AM Andrey V. Lepikhov
 wrote:

On 09.06.2020 11:41, Fujii Masao wrote:

The patches seem not to be registered in CommitFest yet.
Are you planning to do that?

Not now. It is a sharding-related feature. I'm not sure that this
approach is fully consistent with the sharding way now.

Can you please explain in detail, why you think so?  There is no
commit message explaining what each patch does so it is difficult to
understand why you said so?
For now I used this patch set for providing correct visibility in the 
case of access to the table with foreign partitions from many nodes in 
parallel. So I saw at this patch set as a sharding-related feature, but 
[1] shows another useful application.

CSN-based approach has weak points such as:
1. Dependency on clocks synchronization
2. Needs guarantees of monotonically increasing of the CSN in the case 
of an instance restart/crash etc.
3. We need to delay increasing of OldestXmin because it can be needed 
for a transaction snapshot at another node.
So I do not have full conviction that it will be better than a single 
distributed transaction manager.

  Also, can you let us know if this

supports 2PC in some way and if so how is it different from what the
other thread on the same topic [1] is trying to achieve?
Yes, the patch '0003-postgres_fdw-support-for-global-snapshots' contains 
2PC machinery. Now I'd not judge which approach is better.

 Also, I

would like to know if the patch related to CSN based snapshot [2] is a
precursor for this, if not, then is it any way related to this patch
because I see the latest reply on that thread [2] which says it is an
infrastructure of sharding feature but I don't understand completely
whether these patches are related?

I need some time to study this patch. At first sight it is different.


Basically, there seem to be three threads, first, this one and then
[1] and [2] which seems to be doing the work for sharding feature but
there is no clear explanation anywhere if these are anyway related or
whether combining all these three we are aiming for a solution for
atomic commit and atomic visibility.

It can be useful to study all approaches.


I am not sure if you know answers to all these questions so I added
the people who seem to be working on the other two patches.  I am also
afraid that if there is any duplicate or conflicting work going on in
these threads so we should try to find that as well.

Ok



[1] - 
https://www.postgresql.org/message-id/CA%2Bfd4k4v%2BKdofMyN%2BjnOia8-7rto8tsh9Zs3dd7kncvHp12WYw%40mail.gmail.com
[2] - https://www.postgresql.org/message-id/2020061911294657960322%40highgo.ca



[1] 
https://www.postgresql.org/message-id/flat/20200301083601.ews6hz5dduc3w2se%40alap3.anarazel.de


--
Andrey Lepikhov
Postgres Professional
https://postgrespro.com

Re: Review for GetWALAvailability()

[Fujii Masao]

On 2020/06/18 16:36, Kyotaro Horiguchi wrote:

Mmm. I hurried too much..

At Thu, 18 Jun 2020 16:32:59 +0900 (JST), Kyotaro Horiguchi 
 wrote in

If name is specified (so slot is NULL) to
ReplicationSlotAcquireInternal and the slot is not found, the ereport
in following code dereferences NULL.


That's bogus. It is using name in that case. Sorry for the noise.

I don't find a problem by a brief look on it.


Thanks for the review! I pushed the patch.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

hash as an search key and hash collision

[Andy Fan]

I want to maintain an internal table which the primary key is sql_text and
planstmt::text, it is efficient since it both may be very long.  So a
general
idea is to use sql_hash_value and plan_hash_value. Then we have to
handle the hash collision case.  However I checked the codes both in
sr_plans[1]
and pg_stat_statements[2],  both of them didn't handle such cases, IIUC.  so
how can I understand this situation?


[1]
https://github.com/postgrespro/sr_plan/blob/41d96bf136ec072dac77dddf8d9765bba39190ff/sr_plan.c#L383
[2]
https://github.com/postgres/postgres/blob/master/contrib/pg_stat_statements/pg_stat_statements.c#L154


-- 
Best Regards
Andy Fan

Re: min_safe_lsn column in pg_replication_slots view

[Fujii Masao]

On 2020/06/19 16:43, Kyotaro Horiguchi wrote:

At Fri, 19 Jun 2020 16:36:09 +0900, Michael Paquier  wrote 
in

On Fri, Jun 19, 2020 at 04:13:27PM +0900, Fujii Masao wrote:

Agreed. But isn't it too late to remove the columns (i.e., change
the catalog) for v13? Because v13 beta1 was already released.
IIUC the catalog should not be changed since beta1 release so that
users can upgrade PostgreSQL without initdb.


Catalog bumps have happened in the past between beta versions:
git log -p REL_12_BETA1..REL_12_BETA2 src/include/catalog/catversion.h
git log -p REL_11_BETA1..REL_11_BETA2 src/include/catalog/catversion.h
git log -p REL_10_BETA1..REL_10_BETA2 src/include/catalog/catversion.h

So we usually avoid to do that between betas, but my take here is that
a catalog bump is better than regretting a change we may have to live
with after the release is sealed.


Sounds reasonable.

Regards,


--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

Re: Global snapshots

[movead...@highgo.ca]

>> would like to know if the patch related to CSN based snapshot [2] is a
>> precursor for this, if not, then is it any way related to this patch
>> because I see the latest reply on that thread [2] which says it is an
>> infrastructure of sharding feature but I don't understand completely
>> whether these patches are related?
>I need some time to study this patch. At first sight it is different.

This patch[2] is almost base on [3], because I think [1] is talking about 2PC
and FDW, so this patch focus on CSN only and I detach the global snapshot
part and FDW part from the [1] patch. 

I notice CSN will not survival after a restart in [1] patch, I think it may not 
the
right way, may be it is what in last mail "Needs guarantees of monotonically
increasing of the CSN in the case of an instance restart/crash etc" so I try to
add wal support for CSN on this patch.

That's why this thread exist.

> [1] - 
> https://www.postgresql.org/message-id/CA%2Bfd4k4v%2BKdofMyN%2BjnOia8-7rto8tsh9Zs3dd7kncvHp12WYw%40mail.gmail.com
> [2] - https://www.postgresql.org/message-id/2020061911294657960322%40highgo.ca
[3]https://www.postgresql.org/message-id/21BC916B-80A1-43BF-8650-3363CCDAE09C%40postgrespro.ru
 


Regards,
Highgo Software (Canada/China/Pakistan) 
URL : www.highgo.ca 
EMAIL: mailto:movead(dot)li(at)highgo(dot)ca

update substring pattern matching syntax

[Peter Eisentraut]

At 
 
it is described that the substring pattern matching syntax in PostgreSQL 
does not conform to the current standard.  PostgreSQL implements


SUBSTRING(text FROM pattern FOR escapechar)

whereas the current standard says

SUBSTRING(text SIMILAR pattern ESCAPE escapechar)

The former was in SQL99, but the latter has been there since SQL:2003.

It's pretty easy to implement the second form also, so here is a patch 
that does that.


--
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From e6ab38e476d65f592d8542aac68ab6a23d007668 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Fri, 19 Jun 2020 11:14:10 +0200
Subject: [PATCH 1/2] Clean up grammar a bit

Simplify the grammar specification of substring() and overlay() a bit,
simplify and update some comments.
---
 src/backend/parser/gram.y | 73 ---
 1 file changed, 23 insertions(+), 50 deletions(-)

diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index e669d75a5a..1a843049f0 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -452,7 +452,6 @@ static Node *makeRecursiveViewSelect(char *relname, List 
*aliases, Node *query);
 %typeextract_list overlay_list position_list
 %typesubstr_list trim_list
 %typeopt_interval interval_second
-%typeoverlay_placing substr_from substr_for
 %type unicode_normal_form
 
 %type  opt_instead
@@ -13797,11 +13796,6 @@ func_expr_common_subexpr:
}
| OVERLAY '(' overlay_list ')'
{
-   /* overlay(A PLACING B FROM C FOR D) is 
converted to
-* overlay(A, B, C, D)
-* overlay(A PLACING B FROM C) is 
converted to
-* overlay(A, B, C)
-*/
$$ = (Node *) 
makeFuncCall(SystemFuncName("overlay"), $3, @1);
}
| POSITION '(' position_list ')'
@@ -14437,63 +14431,45 @@ unicode_normal_form:
| NFKD  
{ $$ = "nfkd"; }
;
 
-/* OVERLAY() arguments
- * SQL99 defines the OVERLAY() function:
- * o overlay(text placing text from int for int)
- * o overlay(text placing text from int)
- * and similarly for binary strings
- */
+/* OVERLAY() arguments */
 overlay_list:
-   a_expr overlay_placing substr_from substr_for
+   a_expr PLACING a_expr FROM a_expr FOR a_expr
{
-   $$ = list_make4($1, $2, $3, $4);
+   /* overlay(A PLACING B FROM C FOR D) is 
converted to overlay(A, B, C, D) */
+   $$ = list_make4($1, $3, $5, $7);
}
-   | a_expr overlay_placing substr_from
+   | a_expr PLACING a_expr FROM a_expr
{
-   $$ = list_make3($1, $2, $3);
+   /* overlay(A PLACING B FROM C) is 
converted to overlay(A, B, C) */
+   $$ = list_make3($1, $3, $5);
}
;
 
-overlay_placing:
-   PLACING a_expr
-   { $$ = $2; }
-   ;
-
 /* position_list uses b_expr not a_expr to avoid conflict with general IN */
-
 position_list:
b_expr IN_P b_expr  
{ $$ = list_make2($3, $1); }
| /*EMPTY*/ 
{ $$ = NIL; }
;
 
-/* SUBSTRING() arguments
- * SQL9x defines a specific syntax for arguments to SUBSTRING():
- * o substring(text from int for int)
- * o substring(text from int) get entire string from starting point "int"
- * o substring(text for int) get first "int" characters of string
- * o substring(text from pattern) get entire string matching pattern
- * o substring(text from pattern for escape) same with specified escape char
- * We also want to support generic substring functions which accept
- * the usual generic list of arguments. So we will accept both styles
- * here, and convert the SQL9x style to the generic list for further
- * processing. - thomas 2000-11-28
- */
+/* SUBSTRING() arguments */
 substr_list:
-   a_expr substr_from substr_for
+   a_expr FROM a_expr FOR a_expr
{
-

Re: snowball ASCII stemmer configuration

[Peter Eisentraut]

On 2020-06-16 16:37, Tom Lane wrote:

After further reflection, I think these are indeed mistakes and we should
change them all.  The argument for the Russian/English case, AIUI, is
"if we come across an all-ASCII word, it is most certainly not Russian,
and the most likely Latin-based language is English".  Given the world
as it is, I think the same argument works for all non-Latin-alphabet
languages.  Obviously specific applications might have a different idea
of the best fallback language, but that's why we let users make their
own text search configurations.  For general-purpose use, falling back
to English seems reasonable.  And we can be dead certain that applying
a Greek stemmer to an ASCII word will do nothing useful, so the
configuration choice shown above is unhelpful.


Do we *have* to have an ASCII stemmer that corresponds to an actual 
language?  Couldn't we use the simple stemmer or no stemmer at all?


In my experience, ASCII text in, say, Russian or Greek will typically be 
acronyms or brand names or the like, and there doesn't seem to be a 
great need to stem that kind of thing.  Just doing nothing seems at 
least as good.


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

Re: Transactions involving multiple postgres foreign servers, take 2

[Ashutosh Bapat]

On Thu, Jun 18, 2020 at 6:49 PM Bruce Momjian  wrote:
>
> On Thu, Jun 18, 2020 at 04:09:56PM +0530, Amit Kapila wrote:
> > You are right and we are not going to claim that after this feature is
> > committed.  This feature has independent use cases like it can allow
> > parallel copy when foreign tables are involved once we have parallel
> > copy and surely there will be more.  I think it is clear that we need
> > atomic visibility (some way to ensure global consistency) to avoid the
> > data inconsistency problems you and I are worried about and we can do
> > that as a separate patch but at this stage, it would be good if we can
> > have some high-level design of that as well so that if we need some
> > adjustments in the design/implementation of this patch then we can do
> > it now.  I think there is some discussion on the other threads (like
> > [1]) about the kind of stuff we are worried about which I need to
> > follow up on to study the impact.
> >
> > Having said that, I don't think that is a reason to stop reviewing or
> > working on this patch.
>
> I think our first step is to allow sharding to work on read-only
> databases, e.g. data warehousing.  Read/write will require global
> snapshots.  It is true that 2PC is limited usefulness without global
> snapshots, because, by definition, systems using 2PC are read-write
> systems.   However, I can see cases where you are loading data into a
> data warehouse but want 2PC so the systems remain consistent even if
> there is a crash during loading.
>

For sharding, just implementing 2PC without global consistency
provides limited functionality. But for general purpose federated
databases 2PC serves an important functionality - atomic visibility.
When PostgreSQL is used as one of the coordinators in a heterogeneous
federated database system, it's not expected to have global
consistency or even atomic visibility. But it needs a guarantee that
once a transaction commit, all its legs are committed. 2PC provides
that guarantee as long as the other databases keep their promise that
prepared transactions will always get committed when requested so.
Subtle to this is HA requirement from these databases as well. So the
functionality provided by this patch is important outside the sharding
case as well.

As you said, even for a data warehousing application, there is some
write in the form of loading/merging data. If that write happens
across multiple servers, we need  atomic commit to be guaranteed. Some
of these applications can work even if global consistency and atomic
visibility is guaranteed eventually.

-- 
Best Wishes,
Ashutosh Bapat

Re: doing something about the broken dynloader.h symlink

[Thomas Munro]

On Fri, Jun 19, 2020 at 8:02 PM Peter Eisentraut
 wrote:
> +[# Remove links created by old versions of configure, so that there
> +# are no broken symlinks in the tree
> +rm -f src/include/dynloader.h])

+1

Re: doing something about the broken dynloader.h symlink

[Michael Paquier]

On Fri, Jun 19, 2020 at 10:08:05PM +1200, Thomas Munro wrote:
> On Fri, Jun 19, 2020 at 8:02 PM Peter Eisentraut
>  wrote:
>> +[# Remove links created by old versions of configure, so that there
>> +# are no broken symlinks in the tree
>> +rm -f src/include/dynloader.h])
> 
> +1

Not sure about your suggested patch, but +1 for doing something.
--
Michael


signature.asc
Description: PGP signature

Re: update substring pattern matching syntax

[Pavel Stehule]

pá 19. 6. 2020 v 11:42 odesílatel Peter Eisentraut <
peter.eisentr...@2ndquadrant.com> napsal:

> At
> <
> https://wiki.postgresql.org/wiki/PostgreSQL_vs_SQL_Standard#Obsolete_syntax_for_substring.28.29>
>
> it is described that the substring pattern matching syntax in PostgreSQL
> does not conform to the current standard.  PostgreSQL implements
>
>  SUBSTRING(text FROM pattern FOR escapechar)
>
> whereas the current standard says
>
>  SUBSTRING(text SIMILAR pattern ESCAPE escapechar)
>
> The former was in SQL99, but the latter has been there since SQL:2003.
>
> It's pretty easy to implement the second form also, so here is a patch
> that does that.
>

+1

Pavel


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

Re: doing something about the broken dynloader.h symlink

[Julien Rouhaud]

On Fri, Jun 19, 2020 at 12:08 PM Thomas Munro  wrote:
>
> On Fri, Jun 19, 2020 at 8:02 PM Peter Eisentraut
>  wrote:
> > +[# Remove links created by old versions of configure, so that there
> > +# are no broken symlinks in the tree
> > +rm -f src/include/dynloader.h])
>
> +1

+1

Re: Parallel copy

[Ashutosh Sharma]

Hi,

I just got some time to review the first patch in the list i.e.
0001-Copy-code-readjustment-to-support-parallel-copy.patch. As the patch
name suggests, it is just trying to reshuffle the existing code for COPY
command here and there. There is no extra changes added in the patch as
such, but still I do have some review comments, please have a look:

1) Can you please add some comments atop the new function
PopulateAttributes() describing its functionality in detail. Further, this
new function contains the code from BeginCopy() to set attribute level
options used with COPY FROM such as FORCE_QUOTE, FORCE_NOT_NULL, FORCE_NULL
etc. in cstate and along with that it also copies the code from BeginCopy()
to set other infos such as client encoding type, encoding conversion etc.
Hence, I think it would be good to give it some better name, basically
something that matches with what actually it is doing.

2) Again, the name for the new function CheckCopyFromValidity() doesn't
look good to me. From the function name it appears as if it does the sanity
check of the entire COPY FROM command, but actually it is just doing the
sanity check for the target relation specified with COPY FROM. So, probably
something like CheckTargetRelValidity would look more sensible, I think?
TBH, I am not good at naming the functions so you can always ignore my
suggestions about function and variable names :)

3) Any reason for not making CheckCopyFromValidity as a macro instead of a
new function. It is just doing the sanity check for the target relation.

4) Earlier in CopyReadLine() function while trying to clear the EOL marker
from cstate->line_buf.data (copied data), we were not checking if the line
read by CopyReadLineText() function is a header line or not, but I can see
that your patch checks that before clearing the EOL marker. Any reason for
this extra check?

5) I noticed the below spurious line removal in the patch.

@@ -3839,7 +3953,6 @@ static bool
 CopyReadLine(CopyState cstate)
 {
boolresult;
-

Please note that I haven't got a chance to look into other patches as of
now. I will do that whenever possible. Thank you.

--
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com

On Fri, Jun 12, 2020 at 11:01 AM vignesh C  wrote:

> On Thu, Jun 4, 2020 at 12:44 AM Andres Freund  wrote
> >
> >
> > Hm. you don't explicitly mention that in your design, but given how
> > small the benefits going from 0-1 workers is, I assume the leader
> > doesn't do any "chunk processing" on its own?
> >
>
> Yes you are right, the leader does not do any processing, Leader's
> work is mainly to populate the shared memory with the offset
> information for each record.
>
> >
> >
> > > Design of the Parallel Copy: The backend, to which the "COPY FROM"
> query is
> > > submitted acts as leader with the responsibility of reading data from
> the
> > > file/stdin, launching at most n number of workers as specified with
> > > PARALLEL 'n' option in the "COPY FROM" query. The leader populates the
> > > common data required for the workers execution in the DSM and shares it
> > > with the workers. The leader then executes before statement triggers if
> > > there exists any. Leader populates DSM chunks which includes the start
> > > offset and chunk size, while populating the chunks it reads as many
> blocks
> > > as required into the DSM data blocks from the file. Each block is of
> 64K
> > > size. The leader parses the data to identify a chunk, the existing
> logic
> > > from CopyReadLineText which identifies the chunks with some changes was
> > > used for this. Leader checks if a free chunk is available to copy the
> > > information, if there is no free chunk it waits till the required
> chunk is
> > > freed up by the worker and then copies the identified chunks
> information
> > > (offset & chunk size) into the DSM chunks. This process is repeated
> till
> > > the complete file is processed. Simultaneously, the workers cache the
> > > chunks(50) locally into the local memory and release the chunks to the
> > > leader for further populating. Each worker processes the chunk that it
> > > cached and inserts it into the table. The leader waits till all the
> chunks
> > > populated are processed by the workers and exits.
> >
> > Why do we need the local copy of 50 chunks? Copying memory around is far
> > from free. I don't see why it'd be better to add per-process caching,
> > rather than making the DSM bigger? I can see some benefit in marking
> > multiple chunks as being processed with one lock acquisition, but I
> > don't think adding a memory copy is a good idea.
>
> We had run performance with  csv data file, 5.1GB, 10million tuples, 2
> indexes on integer columns, results for the same are given below. We
> noticed in some cases the performance is better if we copy the 50
> records locally and release the shared memory. We will get better
> benefits as the workers increase. Thoughts?
>
> --

Re: missing support for Microsoft VSS Writer

[Juan José Santamaría Flecha]

On Fri, Jun 19, 2020 at 7:20 AM Pavel Stehule 
wrote:

> some czech user reported a broken database when he used a repair point on
> Microsoft Windows.
>
> The reply from Microsoft was, so it is not a Microsoft issue, but Postgres
> issue, because Postgres doesn't handle VSS process correctly, and then
> restore point can restore some parts of Postgres's data too.
>
>
> https://docs.microsoft.com/en-us/windows-server/storage/file-server/volume-shadow-copy-service
>

That is pretty much trying to do file system level backup [1], and the same
caveats apply. Most likely, anything but a repair point with a shutdown
server will result in a broken database when restored.

In order to get a consistent copy there has to be a specific Postgres
Writer [2]. There are some samples on how to use the interface with the
Volume Shadow Copy Service [3], but I do not think there is an interface to
a file-system consistent snapshot in any other system.

[1] https://www.postgresql.org/docs/current/backup-file.html
[2] https://docs.microsoft.com/en-us/windows/win32/vss/writers
[3]
https://github.com/microsoft/Windows-classic-samples/tree/master/Samples/VolumeShadowCopyServiceWriter

Regards,

Juan José Santamaría Flecha

Re: min_safe_lsn column in pg_replication_slots view

[Michael Paquier]

On Fri, Jun 19, 2020 at 05:34:01PM +0900, Fujii Masao wrote:
> On 2020/06/19 16:43, Kyotaro Horiguchi wrote:
>> At Fri, 19 Jun 2020 16:36:09 +0900, Michael Paquier  
>> wrote in
>>> So we usually avoid to do that between betas, but my take here is that
>>> a catalog bump is better than regretting a change we may have to live
>>> with after the release is sealed.
> 
> Sounds reasonable.

If we want to make this happen, I am afraid that the time is short as
beta2 is planned for next week.  As the version will be likely tagged
by Monday US time, it would be good to get this addressed within 48
hours to give some room to the buildfarm to react.  Attached is a
straight-forward proposal of patch.  Any thoughts?

(The change in catversion.h is a self-reminder.)
--
Michael
diff --git a/src/include/catalog/catversion.h b/src/include/catalog/catversion.h
index 7644147cf5..a3753fc9f9 100644
--- a/src/include/catalog/catversion.h
+++ b/src/include/catalog/catversion.h
@@ -53,6 +53,6 @@
  */
 
 /*			mmddN */
-#define CATALOG_VERSION_NO	202006151
+#define CATALOG_VERSION_NO	202006191
 
 #endif
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 61f2c2f5b4..c1bbb5dd63 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -10063,9 +10063,9 @@
   proname => 'pg_get_replication_slots', prorows => '10', proisstrict => 'f',
   proretset => 't', provolatile => 's', prorettype => 'record',
   proargtypes => '',
-  proallargtypes => '{name,name,text,oid,bool,bool,int4,xid,xid,pg_lsn,pg_lsn,text,pg_lsn}',
-  proargmodes => '{o,o,o,o,o,o,o,o,o,o,o,o,o}',
-  proargnames => '{slot_name,plugin,slot_type,datoid,temporary,active,active_pid,xmin,catalog_xmin,restart_lsn,confirmed_flush_lsn,wal_status,min_safe_lsn}',
+  proallargtypes => '{name,name,text,oid,bool,bool,int4,xid,xid,pg_lsn,pg_lsn,text}',
+  proargmodes => '{o,o,o,o,o,o,o,o,o,o,o,o}',
+  proargnames => '{slot_name,plugin,slot_type,datoid,temporary,active,active_pid,xmin,catalog_xmin,restart_lsn,confirmed_flush_lsn,wal_status}',
   prosrc => 'pg_get_replication_slots' },
 { oid => '3786', descr => 'set up a logical replication slot',
   proname => 'pg_create_logical_replication_slot', provolatile => 'v',
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 5314e9348f..507b602a08 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -878,8 +878,7 @@ CREATE VIEW pg_replication_slots AS
 L.catalog_xmin,
 L.restart_lsn,
 L.confirmed_flush_lsn,
-L.wal_status,
-L.min_safe_lsn
+L.wal_status
 FROM pg_get_replication_slots() AS L
 LEFT JOIN pg_database D ON (L.datoid = D.oid);
 
diff --git a/src/backend/replication/slotfuncs.c b/src/backend/replication/slotfuncs.c
index 06e4955de7..a0713bcdf2 100644
--- a/src/backend/replication/slotfuncs.c
+++ b/src/backend/replication/slotfuncs.c
@@ -236,7 +236,7 @@ pg_drop_replication_slot(PG_FUNCTION_ARGS)
 Datum
 pg_get_replication_slots(PG_FUNCTION_ARGS)
 {
-#define PG_GET_REPLICATION_SLOTS_COLS 13
+#define PG_GET_REPLICATION_SLOTS_COLS 12
 	ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo;
 	TupleDesc	tupdesc;
 	Tuplestorestate *tupstore;
@@ -366,19 +366,6 @@ pg_get_replication_slots(PG_FUNCTION_ARGS)
 elog(ERROR, "invalid walstate: %d", (int) walstate);
 		}
 
-		if (max_slot_wal_keep_size_mb >= 0 &&
-			(walstate == WALAVAIL_NORMAL || walstate == WALAVAIL_RESERVED) &&
-			((last_removed_seg = XLogGetLastRemovedSegno()) != 0))
-		{
-			XLogRecPtr	min_safe_lsn;
-
-			XLogSegNoOffsetToRecPtr(last_removed_seg + 1, 0,
-	wal_segment_size, min_safe_lsn);
-			values[i++] = Int64GetDatum(min_safe_lsn);
-		}
-		else
-			nulls[i++] = true;
-
 		Assert(i == PG_GET_REPLICATION_SLOTS_COLS);
 
 		tuplestore_putvalues(tupstore, tupdesc, values, nulls);
diff --git a/src/test/recovery/t/019_replslot_limit.pl b/src/test/recovery/t/019_replslot_limit.pl
index cba7df920c..721e94a1a3 100644
--- a/src/test/recovery/t/019_replslot_limit.pl
+++ b/src/test/recovery/t/019_replslot_limit.pl
@@ -28,9 +28,9 @@ $node_master->safe_psql('postgres',
 
 # The slot state and remain should be null before the first connection
 my $result = $node_master->safe_psql('postgres',
-	"SELECT restart_lsn IS NULL, wal_status is NULL, min_safe_lsn is NULL FROM pg_replication_slots WHERE slot_name = 'rep1'"
+	"SELECT restart_lsn IS NULL, wal_status is NULL FROM pg_replication_slots WHERE slot_name = 'rep1'"
 );
-is($result, "t|t|t", 'check the state of non-reserved slot is "unknown"');
+is($result, "t|t", 'check the state of non-reserved slot is "unknown"');
 
 
 # Take backup
@@ -54,9 +54,9 @@ $node_standby->stop;
 
 # Preparation done, the slot is the state "normal" now
 $result = $node_master->safe_psql('postgres',
-	"SELECT wal_status, min_safe_lsn is NULL FROM pg_replication_slots WHERE slot_name = 'rep1'"
+	"SELECT wal_

Re: min_safe_lsn column in pg_replication_slots view

[Kyotaro Horiguchi]

At Fri, 19 Jun 2020 21:15:52 +0900, Michael Paquier  wrote 
in 
> On Fri, Jun 19, 2020 at 05:34:01PM +0900, Fujii Masao wrote:
> > On 2020/06/19 16:43, Kyotaro Horiguchi wrote:
> >> At Fri, 19 Jun 2020 16:36:09 +0900, Michael Paquier  
> >> wrote in
> >>> So we usually avoid to do that between betas, but my take here is that
> >>> a catalog bump is better than regretting a change we may have to live
> >>> with after the release is sealed.
> > 
> > Sounds reasonable.
> 
> If we want to make this happen, I am afraid that the time is short as
> beta2 is planned for next week.  As the version will be likely tagged
> by Monday US time, it would be good to get this addressed within 48
> hours to give some room to the buildfarm to react.  Attached is a
> straight-forward proposal of patch.  Any thoughts?
> 
> (The change in catversion.h is a self-reminder.)

Thanks for the patch.

As a whole it contains all needed for ripping off the min_safe_lsn.
Some items in the TAP test gets coarse but none of them lose
significance. Compiles almost cleanly and passes all tests including
TAP test.

The variable last_removed_seg in slotfuncs.c:285 is left alone but no
longer used after applying this patch. It should be removed as well.

Other than that the patch looks good to me.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: Global snapshots

[Bruce Momjian]

On Fri, Jun 19, 2020 at 05:03:20PM +0800, movead...@highgo.ca wrote:
> 
> >> would like to know if the patch related to CSN based snapshot [2] is a
> >> precursor for this, if not, then is it any way related to this patch
> >> because I see the latest reply on that thread [2] which says it is an
> >> infrastructure of sharding feature but I don't understand completely
> >> whether these patches are related?
> >I need some time to study this patch.. At first sight it is different.
> 
> This patch[2] is almost base on [3], because I think [1] is talking about 2PC
> and FDW, so this patch focus on CSN only and I detach the global snapshot
> part and FDW part from the [1] patch. 
> 
> I notice CSN will not survival after a restart in [1] patch, I think it may 
> not
> the
> right way, may be it is what in last mail "Needs guarantees of monotonically
> increasing of the CSN in the case of an instance restart/crash etc" so I try 
> to
> add wal support for CSN on this patch.
> 
> That's why this thread exist.

I was certainly missing how these items fit together.  Sharding needs
parallel FDWs, atomic commits, and atomic snapshots.  To get atomic
snapshots, we need CSN.  This new sharding wiki pages has more details:

https://wiki.postgresql.org/wiki/WIP_PostgreSQL_Sharding

After all that is done, we will need optimizer improvements and shard
management tooling.

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

  The usefulness of a cup is in its emptiness, Bruce Lee

Re: snowball ASCII stemmer configuration

[Tom Lane]

Peter Eisentraut  writes:
> Do we *have* to have an ASCII stemmer that corresponds to an actual 
> language?  Couldn't we use the simple stemmer or no stemmer at all?
> In my experience, ASCII text in, say, Russian or Greek will typically be 
> acronyms or brand names or the like, and there doesn't seem to be a 
> great need to stem that kind of thing.  Just doing nothing seems at 
> least as good.

Well, I have no horse in this race.  But the reason it's like this for
Russian is that Oleg, Teodor, and crew set it up that way ages ago.
I'd tend to defer to their opinion about what's the most usable
configuration for Russian.  You could certainly argue that the situation
is different for $other-language ... but without some hard evidence for
that position, making these cases all behave similarly seems like a
reasonable approach.

regards, tom lane

Re: doing something about the broken dynloader.h symlink

[Tom Lane]

Peter Eisentraut  writes:
> When you switch a built REL_11_STABLE or earlier to REL_12_STABLE or 
> later, you get during make install (or, therefore, make check) an error

While I don't necessarily object to the hack you propose here, it seems
to me that really we need to caution people more strongly about not just
cavalierly checking out a different branch in the same build directory
without fully cleaning up built files (that is, "git clean -dfx" or the
like).  There are other gotchas that that creates, and I don't want to
establish a precedent that it's our job to work around them.

regards, tom lane

RE: archive status ".ready" files may be created too early

[matsumura....@fujitsu.com]

> On 5/28/20, 11:42 PM, "matsumura@fujitsu.com" 
> wrote:
> > I'm preparing a patch that backend inserting segment-crossboundary
> > WAL record leaves its EndRecPtr and someone flushing it checks
> > the EndRecPtr and notifies..


I'm sorry for my slow work.

I attach a patch.
I also attach a simple target test for primary.


1. Description in primary side

[Basic problem]
  A process flushing WAL record doesn't know whether the flushed RecPtr is 
  EndRecPtr of cross-segment-boundary WAL record or not because only process 
  inserting the WAL record knows it and it never memorizes the information to 
anywhere.

[Basic concept of the patch in primary]
  A process inserting a cross-segment-boundary WAL record memorizes its 
EndRecPtr
  (I call it CrossBoundaryEndRecPtr) to a new structure in XLogCtl.
  A flushing process creates .ready (Later, I call it just 'notify'.) against 
  a segment that is previous one including CrossBoundaryEndRecPtr only when its 
  flushed RecPtr is equal or greater than the CrossBoundaryEndRecPtr.

[Detail of implementation in primary]
* Structure of CrossBoundaryEndRecPtrs
  Requirement of structure is as the following:
  - System must memorize multiple CrossBoundaryEndRecPtr.
  - A flushing process must determine to notify or not with only flushed RecPtr 
briefly.

  Therefore, I implemented the structure as an array (I call it 
CrossBoundaryEndRecPtr array)
  that is same as xlblck array.  Strictly, it is enogh that the length is
  'xbuffers/wal_segment_size', but I choose 'xbuffers' for simplicity that makes
  enable the flushing process to use XLogRecPtrToBufIdx().
  See also the definition of XLogCtl, XLOGShmemSize(), and XLOGShmemInit() in 
my patch.

* Action of inserting process
  A inserting process memorie its CrossBoundaryEndRecPtr to 
CrossBoundaryEndRecPtr
  array element calculated by XLogRecPtrToBufIdx with its 
CrossBoundaryEndRecPtr.
  If the WAL record crosses many segments, only element against last segment
  including the EndRecPtr is set and others are not set.
  See also CopyXLogRecordToWAL() in my patch.

* Action of flushing process
  Overview has been already written as the follwing.
A flushing process creates .ready (Later, I call it just 'notify'.) against 
a segment that is previous one including CrossBoundaryEndRecPtr only when 
its 
flushed RecPtr is equal or greater than the CrossBoundaryEndRecPtr.

  An additional detail is as the following.  The flushing process may notify
  many segments if the record crosses many segments, so the process memorizes
  latest notified segment number to latestArchiveNotifiedSegNo in XLogCtl.
  The process notifies from latestArchiveNotifiedSegNo + 1 to
  flushing segment number - 1.

  And latestArchiveNotifiedSegNo is set to EndOfLog after Startup process exits
  replay-loop.  Standby also set same timing (= before promoting).

  Mutual exlusion about latestArchiveNotifiedSegNo is not required because
  the timing of accessing has been already included in WALWriteLock critical 
section.


2. Description in standby side

* Who notifies?
  walreceiver also doesn't know whether the flushed RecPtr is EndRecPtr of
  cross-segment-boundary WAL record or not.  In standby, only Startup process
  knows the information because it is hidden in WAL record itself and only
  Startup process reads and builds WAL record.

* Action of Statup process
  Therefore, I implemented that walreceiver never notify and Startup process 
does it.
  In detail, when Startup process reads one full-length WAL record, it notifies
  from a segment that includes head(ReadRecPtr) of the record to a previous 
segment that 
  includes EndRecPtr of the record.

  Now, we must pay attention about switching time line.
  The last segment of previous TimeLineID must be notified before switching.
  This case is considered when RM_XLOG_ID is detected.


3. About other notifying for segment
Two notifyings for segment are remain.  They are not needed to fix.

(1) Notifying for partial segment
It is not needed to be completed, so it's OK to notify without special 
consideration.

(2) Re-notifying
Currently, Checkpointer has notified through XLogArchiveCheckDone().
It is a safe-net for failure of notifying by backend or WAL writer.
Backend or WAL writer doesn't retry to notify if falis, but Checkpointer retries
to notify when it removes old segment. If it fails to notify, then it does not
remove the segment.  It makes Checkpointer to retry notify until the notifying 
suceeeds.
Also, in this case, we can just notify whithout special consideration
because Checkpointer guarantees that all WAL record included in the segment 
have been already flushed.


Please, your review and comments.


Regards
Ryo Matsumura


test_in_primary.sh
Description: test_in_primary.sh


bugfix_early_archiving_v1.0.patch
Description: bugfix_early_archiving_v1.0.patch

Re: [PATCH] Allow to specify restart_lsn in pg_create_physical_replication_slot()

[Alexey Kondratov]

On 2020-06-19 03:59, Michael Paquier wrote:

On Thu, Jun 18, 2020 at 03:39:09PM +0300, Vyacheslav Makarov wrote:
If the WAL segment for the specified restart_lsn (STOP_LSN of the 
backup)
exists, then the function will create a physical replication slot and 
will

keep all the WAL segments required by the replica to catch up with the
primary. Otherwise, it returns error, which means that the required 
WAL
segments have been already utilised, so we do need to take a new 
backup.

Without passing this newly added parameter
pg_create_physical_replication_slot() works as before.

What do you think about this?


I think that this was discussed in the past (perhaps one of the
threads related to WAL advancing actually?),



I have searched through the archives a bit and found one thread related 
to slots advancing [1]. It was dedicated to a problem of advancing slots 
which do not reserve WAL yet, if I get it correctly. Although it is 
somehow related to the topic, it was a slightly different issue, IMO.




and this stuff is full of
holes when it comes to think about error handling with checkpoints
running in parallel, potentially doing recycling of segments you would
expect to be around based on your input value for restart_lsn *while*
pg_create_physical_replication_slot() is still running and
manipulating the on-disk slot information. I suspect that this also
breaks a couple of assumptions behind concurrent calls of the minimum
LSN calculated across slots when a caller sees fit to recompute the
thresholds (WAL senders mainly here, depending on the replication
activity).



These are the right concerns, but all of them should be applicable to 
the pg_create_physical_replication_slot() + immediately_reserve == true 
in the same way, doesn't it? I think so, since in that case we are doing 
a pretty similar thing — trying to reserve some WAL segment that may be 
concurrently deleted.


And this is exactly the reason why ReplicationSlotReserveWal() does it 
in several steps in a loop:


1. Creates a slot with some restart_lsn.
2. Does ReplicationSlotsComputeRequiredLSN() to prevent removal of the 
WAL segment with this restart_lsn.

3. Checks that required WAL segment is still there.
4. Repeat if this attempt to prevent WAL removal has failed.

I guess that the only difference in the case of proposed scenario is 
that we do not have a chance for step 4, since we do need some specific 
restart_lsn, not any recent restart_lsn, i.e. in this case we have to:


1. Create a slot with restart_lsn specified by user.
2. Do ReplicationSlotsComputeRequiredLSN() to prevent WAL removal.
3. Check that required WAL segment is still there and report ERROR to 
the user if it is not.


I have eyeballed the attached patch and it looks like doing exactly the 
same, so issues with concurrent deletion are not obvious for me. Or, 
there are should be the same issues for 
pg_create_physical_replication_slot() + immediately_reserve == true with 
current master implementation.


[1] 
https://www.postgresql.org/message-id/flat/20180626071305.GH31353%40paquier.xyz



Regards
--
Alexey Kondratov

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

Re: [POC] Fast COPY FROM command for the table with foreign partitions

[Etsuro Fujita]

On Tue, Jun 2, 2020 at 2:51 PM Andrey Lepikhov
 wrote:
> 02.06.2020 05:02, Etsuro Fujita пишет:
> > I think I also thought something similar to this before [1].  Will take a 
> > look.

I'm still reviewing the patch, but let me comment on it.

> > [1] 
> > https://www.postgresql.org/message-id/23990375-45a6-5823-b0aa-a6a7a6a957f0%40lab.ntt.co.jp

> I have looked into the thread.
> My first version of the patch was like your idea. But when developing
> the “COPY FROM” code, the following features were discovered:
> 1. Two or more partitions can be placed at the same node. We need to
> finish COPY into one partition before start COPY into another partition
> at the same node.
> 2. On any error we need to send EOF to all started "COPY .. FROM STDIN"
> operations. Otherwise FDW can't cancel operation.
>
> Hiding the COPY code under the buffers management machinery allows us to
> generalize buffers machinery, execute one COPY operation on each buffer
> and simplify error handling.

I'm not sure that it's really a good idea that the bulk-insert API is
designed the way it's tightly coupled with the bulk-insert machinery
in the core, because 1) some FDWs might want to send tuples provided
by the core to the remote, one by one, without storing them in a
buffer, or 2) some other FDWs might want to store the tuples in the
buffer and send them in a lump as postgres_fdw in the proposed patch
but might want to do so independently of MAX_BUFFERED_TUPLES and/or
MAX_BUFFERED_BYTES defined in the bulk-insert machinery.

I agree that we would need special handling for cases you mentioned
above if we design this API based on something like the idea I
proposed in that thread.

> As i understand, main idea of the thread, mentioned by you, is to add
> "COPY FROM" support without changes in FDW API.

I don't think so; I think we should introduce new API for this feature
to keep the ExecForeignInsert() API simple.

> All that I can offer in this place now is to introduce one new
> ExecForeignBulkInsert(buf) routine that will execute single "COPY FROM
> STDIN" operation, send tuples and close the operation. We can use the
> ExecForeignInsert() routine for each buffer tuple if
> ExecForeignBulkInsert() is not supported.

Agreed.

> One of main questions here is to use COPY TO machinery for serializing a
> tuple. It is needed (if you will take a look into the patch) to
> transform the CopyTo() routine to an iterative representation:
> start/next/finish. May it be acceptable?

+1 for the general idea.

> In the attachment there is a patch with the correction of a stupid error.

Thanks for the patch!

Sorry for the delay.

Best regards,
Etsuro Fujita

Re: More tzdb fun: POSIXRULES is being deprecated upstream

[Tom Lane]

I wrote:
> I experimented with removing the posixrules support, and was quite glad
> I did, because guess what: our regression tests fall over.  If we do
> nothing we can expect that they'll start failing on various random systems
> come this fall.

To clarify, you can produce this failure without any code changes:
build a standard installation (*not* using --with-system-tzdata),
remove its .../share/timezone/posixrules file, and run "make
installcheck".  So builds that do use --with-system-tzdata will fail both
"make check" and "make installcheck" if the platform's tzdata packager
decides to get rid of the posixrules file.

However, on closer inspection, all the test cases that depend on 'PST8PDT'
are fine, because we *do* pick up the zone file by that name.  The cases
that fall over are a few in horology.sql that depend on

SET TIME ZONE 'CST7CDT';

There is no such zone file, because that's a mistake: the central US
zone is more properly rendered 'CST6CDT'.  So this is indeed a bare
POSIX zone specification, and its behavior changes if there's no
posixrules file to back-fill knowledge about pre-2007 DST laws.

These test cases originated in commit b2b6548c7.  That was too long ago
to be sure, but I suspect that the use of a bogus zone was just a thinko;
there's certainly nothing in the commit log or the text of the patch
suggesting that it was intentional.  Still, it seems good to be testing
our POSIX-zone-string code paths, so I'm inclined to leave it as CST7CDT
but remove the dependence on posixrules by adding an explicit transition
rule.

Also, I notice a couple of related documentation issues:

* The same commit added a documentation example that also cites CST7CDT.
That needs to be fixed to correspond to something that would actually
be used in the real world, probably America/Denver.  Otherwise the
example will fail to work for some people.

* We should add something to the new appendix about POSIX zone specs
pointing out that while EST5EDT, CST6CDT, MST7MDT, PST8PDT look like they
are POSIX strings, they actually are captured by IANA zone files, so
that they produce valid historical US DST transitions even when a
plain POSIX string wouldn't.

I'm less excited than I was yesterday about removing the tests' dependency
on 'PST8PDT'.  It remains possible that we might need to do that someday,
but I doubt it'd happen without plenty of warning.

regards, tom lane

Re: Add A Glossary

[Erik Rijkers]

On 2020-06-19 01:51, Alvaro Herrera wrote:

On 2020-Jun-16, Justin Pryzby wrote:

On Tue, Jun 16, 2020 at 08:09:26PM -0400, Alvaro Herrera wrote:


I noticed one typo:

'aggregates functions'  should be
'aggregate functions'


And one thing that I am not sure of (but strikes me as a bit odd):
there are several cases of
'are enforced unique'. Should that not be
'are enforced to be unique'  ?


Anther small mistake (2x):

'The name of such objects of the same type are'  should be
'The names of such objects of the same type are'

(this phrase occurs 2x wrong, 1x correct)


thanks,

Erik Rijkers

Re: hash as an search key and hash collision

[Tomas Vondra]

On Fri, Jun 19, 2020 at 04:24:01PM +0800, Andy Fan wrote:

I want to maintain an internal table which the primary key is sql_text and
planstmt::text, it is efficient since it both may be very long.  So a
general
idea is to use sql_hash_value and plan_hash_value. Then we have to
handle the hash collision case.  However I checked the codes both in
sr_plans[1]
and pg_stat_statements[2],  both of them didn't handle such cases, IIUC.  so
how can I understand this situation?



IIRC pg_stat_statements simply accepts the hash collision risk. This is
what the docs say:

In some cases, queries with visibly different texts might get merged
into a single pg_stat_statements entry. Normally this will happen
only for semantically equivalent queries, but there is a small
chance of hash collisions causing unrelated queries to be merged
into one entry. (This cannot happen for queries belonging to
different users or databases, however.)

The consequences of a hash collision are relatively harmless, enough to
make it not worth the extra checks (e.g. because the SQL text may not be
available in memory and would need to be read from the file).

I suppose sr_plan does the same thing, but I haven't checked.

regards

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

Re: Re: [HACKERS] Custom compression methods

[Robert Haas]

On Thu, Mar 7, 2019 at 2:51 AM Alexander Korotkov
 wrote:
> Yes.  I took a look at code of this patch.  I think it's in pretty good 
> shape.  But high level review/discussion is required.

I agree that the code of this patch is in pretty good shape, although
there is a lot of rebasing needed at this point. Here is an attempt at
some high level review and discussion:

- As far as I can see, there is broad agreement that we shouldn't
consider ourselves to be locked into 'pglz' forever. I believe
numerous people have reported that there are other methods of doing
compression that either compress better, or compress faster, or
decompress faster, or all of the above. This isn't surprising and nor
is it a knock on 'pglz'; Jan committed it in 1999, and it's not
surprising that in 20 years some people have come up with better
ideas. Not only that, but the quantity and quality of open source
software that is available for this kind of thing and for many other
kinds of things have improved dramatically in that time.

- I can see three possible ways of breaking our dependence on 'pglz'
for TOAST compression. Option #1 is to pick one new algorithm which we
think is better than 'pglz' in all relevant ways and use it as the
default for all new compressed datums. This would be dramatically
simpler than what this patch does, because there would be no user
interface. It would just be out with the old and in with the new.
Option #2 is to create a short list of new algorithms that have
different trade-offs; e.g. one that is very fast (like lz4) and one
that has an extremely high compression ratio, and provide an interface
for users to choose between them. This would be moderately simpler
than what this patch does, because we would expose to the user
anything about how a new compression method could be added, but it
would still require a UI for the user to choose between the available
(and hard-coded) options. It has the further advantage that every
PostgreSQL cluster will offer the same options (or a subset of them,
perhaps, depending on configure flags) and so you don't have to worry
that, say, a pg_am row gets lost and suddenly all of your toasted data
is inaccessible and uninterpretable. Option #3 is to do what this
patch actually does, which is to allow for the addition of any number
of compressors, including by extensions. It has the advantage that new
compressors can be added with core's permission, so, for example, if
it is unclear whether some excellent compressor is free of patent
problems, we can elect not to ship support for it in core, while at
the same time people who are willing to accept the associated legal
risk can add that functionality to their own copy as an extension
without having to patch core. The legal climate may even vary by
jurisdiction, so what might be questionable in country A might be
clearly just fine in country B. Aside from those issues, this approach
allows people to experiment and innovate outside of core relatively
quickly, instead of being bound by the somewhat cumbrous development
process which has left this patch in limbo for the last few years. My
view is that option #1 is likely to be impractical, because getting
people to agree is hard, and better things are likely to come along
later, and people like options. So I prefer either #2 or #3.

- The next question is how a datum compressed with some non-default
method should be represented on disk. The patch handles this first of
all by making the observation that the compressed size can't be >=1GB,
because the uncompressed size can't be >=1GB, and we wouldn't have
stored it compressed if it expanded. Therefore, the upper two bits of
the compressed size should always be zero on disk, and the patch
steals one of them to indicate whether "custom" compression is in use.
If it is, the 4-byte varlena header is followed not only by a 4-byte
size (now with the new flag bit also included) but also by a 4-byte
OID, indicating the compression AM in use. I don't think this is a
terrible approach, but I don't think it's amazing, either. 4 bytes is
quite a bit to use for this; if I guess correctly what will be a
typical cluster configuration, you probably would really only need
about 2 bits. For a datum that is both stored externally and
compressed, the overhead is likely negligible, because the length is
probably measured in kB or MB. But for a datum that is compressed but
not stored externally, it seems pretty expensive; the datum is
probably short, and having an extra 4 bytes of uncompressible data
kinda sucks. One possibility would be to allow only one byte here:
require each compression AM that is installed to advertise a one-byte
value that will denote its compressed datums. If more than one AM
tries to claim the same byte value, complain. Another possibility is
to abandon this approach and go with #2 from the previous paragraph.
Or maybe we add 1 or 2 "privileged" built-in compressors that get
dedicated bit-patterns in the upper 2 bits of th

Re: Failures with installcheck and low work_mem value in 13~

[Tomas Vondra]

On Tue, Jun 16, 2020 at 02:39:47PM +0900, Michael Paquier wrote:

On Mon, Jun 15, 2020 at 10:29:41PM +0900, Michael Paquier wrote:

Attempting to run installcheck with 13~ and a value of work_mem lower
than the default causes two failures, both related to incremental
sorts (here work_mem = 1MB):
1) Test incremental_sort:
@@ -4,12 +4,13 @@
 select * from (select * from tenk1 order by four) t order by four, ten;
 QUERY PLAN
 ---
- Sort
+ Incremental Sort
Sort Key: tenk1.four, tenk1.ten
+   Presorted Key: tenk1.four
->  Sort
  Sort Key: tenk1.four
  ->  Seq Scan on tenk1
-(5 rows)
+(6 rows)


Looking at this one, it happens that this is the first test in
incremental_sort.sql, and we have the following comment:
-- When we have to sort the entire table, incremental sort will
-- be slower than plain sort, so it should not be used.
explain (costs off)
select * from (select * from tenk1 order by four) t order by four, ten;

When using such a low value of work_mem, why do we switch to an
incremental sort if we know that it is going to be slower than a plain
sort?  Something looks wrong in the planner choice here.


I don't think it's particularly wrong. The "full sort" can't be done in
memory with such low work_mem value, while the incremental sort can. So
I think the planner choice is sane, it's more than the comment does not
explicitly state this depends on work_mem too.


regards

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

Re: Failures with installcheck and low work_mem value in 13~

[Tomas Vondra]

On Mon, Jun 15, 2020 at 10:29:41PM +0900, Michael Paquier wrote:

Hi all,

Attempting to run installcheck with 13~ and a value of work_mem lower
than the default causes two failures, both related to incremental
sorts (here work_mem = 1MB):
1) Test incremental_sort:
@@ -4,12 +4,13 @@
select * from (select * from tenk1 order by four) t order by four, ten;
QUERY PLAN
---
- Sort
+ Incremental Sort
   Sort Key: tenk1.four, tenk1.ten
+   Presorted Key: tenk1.four
   ->  Sort
 Sort Key: tenk1.four
 ->  Seq Scan on tenk1
-(5 rows)
+(6 rows)

2) Test join:
@@ -2368,12 +2368,13 @@
   ->  Merge Left Join
 Merge Cond: (x.thousand = y.unique2)
 Join Filter: ((x.twothousand = y.hundred) AND (x.fivethous = 
y.unique2))
- ->  Sort
+ ->  Incremental Sort
   Sort Key: x.thousand, x.twothousand, x.fivethous
-   ->  Seq Scan on tenk1 x
+   Presorted Key: x.thousand
+   ->  Index Scan using tenk1_thous_tenthous on tenk1 x
 ->  Materialize
   ->  Index Scan using tenk1_unique2 on tenk1 y
-(9 rows)
+(10 rows)

There are of course regression failures when changing the relation
page size or such, but we should have tests more portable when it
comes to work_mem (this issue does not exist in ~12) or people running
installcheck on a new instance would be surprised.  Please note that I
have not looked at the problem in details, but a simple solution would
be to enforce work_mem in those code paths to keep the two plans
stable.



I don't think the tests can be made not to depend on work_mem, because
it costing of sort / incremental sort depends on the value. I agree
setting the work_mem at the beginning of the test script is the right
solution.

regards

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

Re: Add A Glossary

[Alvaro Herrera]

Thanks for these fixes!  I included all of these.

On 2020-Jun-19, Erik Rijkers wrote:

> And one thing that I am not sure of (but strikes me as a bit odd):
> there are several cases of
> 'are enforced unique'. Should that not be
> 'are enforced to be unique'  ?

I included this change too; I am not too sure of it myself.  If some
English language neatnik wants to argue one way or the other, be my
guest.

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

Re: Failures with installcheck and low work_mem value in 13~

[Tom Lane]

Tomas Vondra  writes:
> I don't think the tests can be made not to depend on work_mem, because
> it costing of sort / incremental sort depends on the value. I agree
> setting the work_mem at the beginning of the test script is the right
> solution.

I'm a bit skeptical about changing anything here.  There are quite
a large number of GUCs that can affect the regression results, and
it wouldn't be sane to try to force them all to fixed values.  For
one thing, that'd be a PITA to maintain, and for another, it's not
infrequently useful to run the tests with nonstandard settings to
see what happens.

Is there a good reason for being concerned about work_mem in particular
and this test script in particular?

regards, tom lane

Re: Failures with installcheck and low work_mem value in 13~

[Peter Geoghegan]

On Fri, Jun 19, 2020 at 10:27 AM Tom Lane  wrote:
> Tomas Vondra  writes:
> > I don't think the tests can be made not to depend on work_mem, because
> > it costing of sort / incremental sort depends on the value. I agree
> > setting the work_mem at the beginning of the test script is the right
> > solution.
>
> I'm a bit skeptical about changing anything here.  There are quite
> a large number of GUCs that can affect the regression results, and
> it wouldn't be sane to try to force them all to fixed values.  For
> one thing, that'd be a PITA to maintain, and for another, it's not
> infrequently useful to run the tests with nonstandard settings to
> see what happens.

+1

-- 
Peter Geoghegan

Re: compile cube extension with float4 precision storing

[Tomas Vondra]

On Fri, Jun 12, 2020 at 02:41:08PM +0300, Siarhei D wrote:

Is it possible to make cube extension with float(4bytes) precision instead
of double(8bytes)?

I use cube extension for storing embedding vectors and calculation distance
on them. During comparing vectors, a 4byte float precision is enough.
Storing 8 byte double precision is wasting disk space.

Now to avoid disk wasting I store vectors as real[] array and create cube
objects on the fly. But this solution is wasting cpu time



I don't think there's a built-in support for that, so the only option I
can think of is creating your own cube "copy" extension using float4.

regards

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

Re: Creating a function for exposing memory usage of backend process

[Robert Haas]

On Wed, Jun 17, 2020 at 11:56 PM Fujii Masao
 wrote:
> > As a first step, to deal with (3) I'd like to add
> > pg_stat_get_backend_memory_context() which target is limited to the
> > local backend process.
>
> +1

+1 from me, too. Something that exposed this via shared memory would
be quite useful, and there are other things we'd like to expose
similarly, such as the plan(s) from the running queries, or even just
the untruncated query string(s). I'd like to have a good
infrastructure for that sort of thing, but I think it's quite tricky
to do properly. You need a variable-size chunk of shared memory, so
probably it has to use DSM somehow, and you need to update it as
things change, but if you update it too frequently performance will
stink. If you ping processes to do the updates, how do you know when
they've completed them, and what happens if they don't respond in a
timely fashion? These are probably all solvable problems, but I don't
think they are very easy ones.

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

Re: [PATCH] Allow to specify restart_lsn in pg_create_physical_replication_slot()

[Fujii Masao]

On 2020/06/19 23:20, Alexey Kondratov wrote:

On 2020-06-19 03:59, Michael Paquier wrote:

On Thu, Jun 18, 2020 at 03:39:09PM +0300, Vyacheslav Makarov wrote:

If the WAL segment for the specified restart_lsn (STOP_LSN of the backup)
exists, then the function will create a physical replication slot and will
keep all the WAL segments required by the replica to catch up with the
primary. Otherwise, it returns error, which means that the required WAL
segments have been already utilised, so we do need to take a new backup.
Without passing this newly added parameter
pg_create_physical_replication_slot() works as before.

What do you think about this?


Currently pg_create_physical_replication_slot() and CREATE_REPLICATION_SLOT
replication command seem to be "idential". So if we add new option into one,
we should add it also into another?


What happen if future LSN is specified in restart_lsn? With the patch,
in this case, if the segment at that LSN exists (e.g., because it's recycled
one), the slot seems to be successfully created. However if the LSN is
far future and the segment doesn't exist, the creation of slot seems to fail.
This behavior looks fragile and confusing. We should accept future LSN
whether its segment currently exists or not?


+   if (!RecoveryInProgress() && !SlotIsLogical(MyReplicationSlot))

With the patch, the given restart_lsn seems to be ignored during recovery.
Why?



I think that this was discussed in the past (perhaps one of the
threads related to WAL advancing actually?),



I have searched through the archives a bit and found one thread related to 
slots advancing [1]. It was dedicated to a problem of advancing slots which do 
not reserve WAL yet, if I get it correctly. Although it is somehow related to 
the topic, it was a slightly different issue, IMO.



and this stuff is full of
holes when it comes to think about error handling with checkpoints
running in parallel, potentially doing recycling of segments you would
expect to be around based on your input value for restart_lsn *while*
pg_create_physical_replication_slot() is still running and
manipulating the on-disk slot information. I suspect that this also
breaks a couple of assumptions behind concurrent calls of the minimum
LSN calculated across slots when a caller sees fit to recompute the
thresholds (WAL senders mainly here, depending on the replication
activity).



These are the right concerns, but all of them should be applicable to the 
pg_create_physical_replication_slot() + immediately_reserve == true in the same 
way, doesn't it? I think so, since in that case we are doing a pretty similar 
thing — trying to reserve some WAL segment that may be concurrently deleted.

And this is exactly the reason why ReplicationSlotReserveWal() does it in 
several steps in a loop:

1. Creates a slot with some restart_lsn.
2. Does ReplicationSlotsComputeRequiredLSN() to prevent removal of the WAL 
segment with this restart_lsn.
3. Checks that required WAL segment is still there.
4. Repeat if this attempt to prevent WAL removal has failed.


What happens if concurrent checkpoint decides to remove the segment
at restart_lsn before #2 and then actually removes it after #3?
The replication slot is successfully created with the given restart_lsn,
but the reserved segment has already been removed?



I guess that the only difference in the case of proposed scenario is that we do 
not have a chance for step 4, since we do need some specific restart_lsn, not 
any recent restart_lsn, i.e. in this case we have to:

1. Create a slot with restart_lsn specified by user.
2. Do ReplicationSlotsComputeRequiredLSN() to prevent WAL removal.
3. Check that required WAL segment is still there and report ERROR to the user 
if it is not.


The similar situation as the above may happen.

Regards,


--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

Re: Missing HashAgg EXPLAIN ANALYZE details for parallel plans

[Alvaro Herrera]

On 2020-Jun-19, David Rowley wrote:


> > +   size = offsetof(SharedAggInfo, sinstrument)
> > +   + pcxt->nworkers * sizeof(AggregateInstrumentation);
> >
> > => There's a couple places where I'd prefer to see "+" at the end of the
> > preceding line (but YMMV).
> 
> I pretty much just copied the whole of that code from nodeSort.c. I'm
> more inclined to just keep it as similar to that as possible. However,
> if pgindent decides otherwise, then I'll go with that. I imagine it
> won't move it though as that code has already been through indentation
> a few times before.

pgindent won't change your choice here.  This seems an ideological
issue; each committer has got their style.  Some people prefer to put
operators at the end of the line, others do it this way.

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

Re: Fast DSM segments

[Andres Freund]

Hi,

On 2020-06-19 17:42:41 +1200, Thomas Munro wrote:
> On Thu, Jun 18, 2020 at 6:05 PM Thomas Munro  wrote:
> > Here's a version that adds some documentation.
> 
> I jumped on a dual socket machine with 36 cores/72 threads and 144GB
> of RAM (Azure F72s_v2) running Linux, configured with 50GB of huge
> pages available, and I ran a very simple test: select count(*) from t
> t1 join t t2 using (i), where the table was created with create table
> t as select generate_series(1, 4)::int i, and then prewarmed
> into 20GB of shared_buffers.

I assume all the data fits into 20GB?

Which kernel version is this?

How much of the benefit comes from huge pages being used, how much from
avoiding the dsm overhead, and how much from the page table being shared
for that mapping? Do you have a rough idea?

Greetings,

Andres Freund

Re: More tzdb fun: POSIXRULES is being deprecated upstream

[Tom Lane]

Peter Eisentraut  writes:
> On 2020-06-17 20:08, Tom Lane wrote:
>> I would definitely be in favor of "nuke it now" with respect to HEAD.
>> It's a bit more debatable for the back branches.  However, all branches
>> are going to be equally exposed to updated system tzdata trees, so
>> we've typically felt that changes in the tz-related code should be
>> back-patched.

> It seems sensible to me to remove it in master and possibly 
> REL_13_STABLE, but leave it alone in the back branches.

For purposes of discussion, here's a patch that rips out posixrules
support altogether.  (Note that further code simplifications could
be made --- the "load_ok" variable is vestigial, for instance.  This
formulation is intended to minimize the diffs from upstream.)

A less aggressive idea would be to leave the code alone and just change
the makefiles to not install a posixrules file in our own builds.
That'd leave the door open for somebody who really needed posixrules
behavior to get it back by just creating a posixrules file.  I'm not
sure this idea has much else to recommend it though.

I'm honestly not sure what I think we should do exactly.
The main arguments in favor of the full-rip-out option seem to be

(1) It'd ensure consistent behavior of POSIX zone specs across
platforms, whether or not --with-system-tzdata is used and whether
or not the platform supplies a posixrules file.

(2) We'll presumably be forced into the no-posixrules behavior at
some point, so forcing the issue lets us dictate the timing rather
than having it be dictated to us.  If nothing else, that means we
can release-note the behavioral change in a timely fashion.

Point (2) seems like an argument for doing it only in master
(possibly plus v13), but on the other hand I'm not convinced about
how much control we really have if we wait.  What seems likely
to happen is that posixrules files will disappear from platform
tz databases over some hard-to-predict timespan.  Even if no
major platforms drop them immediately at the next IANA update,
it seems quite likely that some/many will do so within the remaining
support lifetime of v12.  So even if we continue to support the feature,
it's likely to vanish in practice at some uncertain point.

Given that the issue only affects people using nonstandard TimeZone
settings, it may be that we shouldn't agonize over it too much
either way.

Anyway, as I write this I'm kind of talking myself into the position
that we should indeed back-patch this.  The apparent stability
benefits of not doing so may be illusory, and if we back-patch then
at least we get to document that there's a change.  But an argument
could be made in the other direction too.

Thoughts?

regards, tom lane

diff --git a/doc/src/sgml/datetime.sgml b/doc/src/sgml/datetime.sgml
index 71fbf842cc..bbf50b76f8 100644
--- a/doc/src/sgml/datetime.sgml
+++ b/doc/src/sgml/datetime.sgml
@@ -718,33 +718,12 @@
   
If a daylight-savings abbreviation is given but the
transition rule field is omitted,
-   PostgreSQL attempts to determine the
-   transition times by consulting the posixrules file
-   in the IANA time zone database.  This file has the same format as a
-   full time zone entry, but only its transition timing rules are used,
-   not its UTC offsets.  Typically, this file has the same contents as the
-   US/Eastern file, so that POSIX-style time zone
-   specifications follow USA daylight-savings rules.  If needed, you can
-   adjust this behavior by replacing the posixrules
-   file.
-  
-
-  
-   
-The facility to consult a posixrules file has
-been deprecated by IANA, and it is likely to go away in the future.
-One bug in this feature, which is unlikely to be fixed before it
-disappears, is that it fails to apply DST rules to dates after 2038.
-   
-  
-
-  
-   If the posixrules file is not present,
the fallback behavior is to use the
rule M3.2.0,M11.1.0, which corresponds to USA
practice as of 2020 (that is, spring forward on the second Sunday of
March, fall back on the first Sunday of November, both transitions
-   occurring at 2AM prevailing time).
+   occurring at 2AM prevailing time).  Note that this rule does not
+   give correct USA transition dates for years before 2007.
   
 
   
@@ -765,8 +744,7 @@
because (for historical reasons) there are files by those names in the
IANA time zone database.  The practical implication of this is that
these zone names will produce valid historical USA daylight-savings
-   transitions, even when a plain POSIX specification would not due to
-   lack of a suitable posixrules file.
+   transitions, even when a plain POSIX specification would not.
   
 
   
diff --git a/src/timezone/Makefile b/src/timezone/Makefile
index bf23ac9da9..715b63cee0 100644
--- a/src/timezone/Makefile
+++ b/src/timezone/Makefile
@@ -29,10 +29,6 @@ ZICOBJS = \
 # we now distribute the timezone data as a single file
 TZDATAFILES = $(srcdir)/data/tzdata.zi
 

Re: Failures with wal_consistency_checking and 13~

[Alvaro Herrera]

On 2020-Jun-15, Michael Paquier wrote:

> I have begun my annual study of WAL consistency across replays, and
> wal_consistency_checking = 'all' is pointing out at some issues with
> at least VACUUM and SPGist:
> FATAL:  inconsistent page found, rel 1663/16385/22133, forknum 0,
> blkno 15
> CONTEXT:  WAL redo at 0/739CEDE8 for SPGist/VACUUM_REDIRECT: newest
> XID 4619
> 
> It may be possible that there are other failures, I have just run
> installcheck and this is the first failure I saw after replaying all
> the generated WAL on a standby.  Please note that I have also checked
> 12, and installcheck passes.

Umm.  Alexander, do you an idea of what this is about?

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

Re: More tzdb fun: POSIXRULES is being deprecated upstream

[Robert Haas]

On Fri, Jun 19, 2020 at 3:27 PM Tom Lane  wrote:
> Anyway, as I write this I'm kind of talking myself into the position
> that we should indeed back-patch this.  The apparent stability
> benefits of not doing so may be illusory, and if we back-patch then
> at least we get to document that there's a change.  But an argument
> could be made in the other direction too.

It's really unclear to me why we should back-patch this into
already-released branches. I grant your point that perhaps few people
will notice, and also that this might happen at some point the change
will be forced upon us. Nonetheless, we bill our back-branches as
being stable, which seems inconsistent with forcing a potentially
breaking change into them without a clear and pressing need. If you
commit this patch to master and v13, no already-release branches will
be affected immediately, and it's conceivable that some or even all of
the older branches will age out before the issue is forced. That would
be all to the good. And even if the issue is forced sooner rather than
later, how much do we really lose by waiting until we have that
problem in front of us?

I'm not in a position to judge how much additional maintenance
overhead would be imposed by not back-patching this at once, so if you
tell me that it's an intolerable burden, I can't really argue with
that. But if it's possible to take a wait-and-see attitude for the
time being, so much the better.

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

Re: More tzdb fun: POSIXRULES is being deprecated upstream

[Tom Lane]

Robert Haas  writes:
> It's really unclear to me why we should back-patch this into
> already-released branches. I grant your point that perhaps few people
> will notice, and also that this might happen at some point the change
> will be forced upon us. Nonetheless, we bill our back-branches as
> being stable, which seems inconsistent with forcing a potentially
> breaking change into them without a clear and pressing need. If you
> commit this patch to master and v13, no already-release branches will
> be affected immediately, and it's conceivable that some or even all of
> the older branches will age out before the issue is forced. That would
> be all to the good. And even if the issue is forced sooner rather than
> later, how much do we really lose by waiting until we have that
> problem in front of us?

> I'm not in a position to judge how much additional maintenance
> overhead would be imposed by not back-patching this at once, so if you
> tell me that it's an intolerable burden, I can't really argue with
> that. But if it's possible to take a wait-and-see attitude for the
> time being, so much the better.

The code delta is small enough that I don't foresee any real maintenance
problem if we let the back branches differ from HEAD/v13 on this point.
What I'm concerned about is that people depending on the existing
behavior are likely to wake up one fine morning and discover that it's
broken after a routine tzdata update.  I think that it'd be a better
user experience for them to see a release-note entry in a PG update
release explaining that this will break and here's what to do to fix it.

Yeah, we can do nothing in the back branches and hope that that doesn't
happen for the remaining lifespan of v12.  But I wonder whether that
doesn't amount to sticking our heads in the sand.

I suppose it'd be possible to have a release-note entry in the back
branches that isn't tied to any actual code change on our part, but just
warns that such a tzdata change might happen at some unpredictable future
time.  That feels weird and squishy though; and people would likely have
forgotten it by the time the change actually hits them.

regards, tom lane

Re: Parallel Seq Scan vs kernel read ahead

[Robert Haas]

On Thu, Jun 18, 2020 at 10:10 PM David Rowley  wrote:
> Here's a patch which caps the maximum chunk size to 131072.  If
> someone doubles the page size then that'll be 2GB instead of 1GB. I'm
> not personally worried about that.

Maybe use RELSEG_SIZE?

> I tested the performance on a Windows 10 laptop using the test case from [1]
>
> Master:
>
> workers=0: Time: 141175.935 ms (02:21.176)
> workers=1: Time: 316854.538 ms (05:16.855)
> workers=2: Time: 323471.791 ms (05:23.472)
> workers=3: Time: 321637.945 ms (05:21.638)
> workers=4: Time: 308689.599 ms (05:08.690)
> workers=5: Time: 289014.709 ms (04:49.015)
> workers=6: Time: 267785.270 ms (04:27.785)
> workers=7: Time: 248735.817 ms (04:08.736)
>
> Patched:
>
> workers=0: Time: 155985.204 ms (02:35.985)
> workers=1: Time: 112238.741 ms (01:52.239)
> workers=2: Time: 105861.813 ms (01:45.862)
> workers=3: Time: 91874.311 ms (01:31.874)
> workers=4: Time: 92538.646 ms (01:32.539)
> workers=5: Time: 93012.902 ms (01:33.013)
> workers=6: Time: 94269.076 ms (01:34.269)
> workers=7: Time: 90858.458 ms (01:30.858)

Nice results. I wonder if these stack with the gains Thomas was
discussing with his DSM-from-the-main-shmem-segment patch.

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

Re: More tzdb fun: POSIXRULES is being deprecated upstream

[Robert Haas]

On Fri, Jun 19, 2020 at 3:55 PM Tom Lane  wrote:
> The code delta is small enough that I don't foresee any real maintenance
> problem if we let the back branches differ from HEAD/v13 on this point.
> What I'm concerned about is that people depending on the existing
> behavior are likely to wake up one fine morning and discover that it's
> broken after a routine tzdata update.  I think that it'd be a better
> user experience for them to see a release-note entry in a PG update
> release explaining that this will break and here's what to do to fix it.

I was assuming that if you did an update of the tzdata, you'd notice
if posixrules had been nuked. I guess that wouldn't help people who
are using the system tzdata, though. It might be nice to know what
Debian, RHEL, etc. plan to do about this, but I'm not sure how
practical it is to find out.

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

Re: More tzdb fun: POSIXRULES is being deprecated upstream

[Tom Lane]

Robert Haas  writes:
> On Fri, Jun 19, 2020 at 3:55 PM Tom Lane  wrote:
>> What I'm concerned about is that people depending on the existing
>> behavior are likely to wake up one fine morning and discover that it's
>> broken after a routine tzdata update.  I think that it'd be a better
>> user experience for them to see a release-note entry in a PG update
>> release explaining that this will break and here's what to do to fix it.

> I was assuming that if you did an update of the tzdata, you'd notice
> if posixrules had been nuked. I guess that wouldn't help people who
> are using the system tzdata, though.

Yeah, exactly.  We can control this easily enough for PG-supplied tzdata
trees, but I think a significant majority of actual users are using
--with-system-tzdata builds, because we've been telling packagers to
do it that way for years.  (Nor does changing that advice seem like
a smart move.)

> It might be nice to know what
> Debian, RHEL, etc. plan to do about this, but I'm not sure how
> practical it is to find out.

There's probably no way to know until it happens :-(.  We can hope
that they'll be conservative, but it's hard to be sure.  It doesn't
help that the bigger players rely on glibc: if I understand what
Eggert was saying, nuking posixrules would bring tzcode's behavior
into closer sync with what glibc does, so they might well feel it's
a desirable change.

regards, tom lane

Re: More tzdb fun: POSIXRULES is being deprecated upstream

[Tom Lane]

I wrote:
> Robert Haas  writes:
>> It might be nice to know what
>> Debian, RHEL, etc. plan to do about this, but I'm not sure how
>> practical it is to find out.

> There's probably no way to know until it happens :-(.

On the other hand, for the open-source players, it might be easier to
guess.  I took a look at the Fedora/RHEL tzdata specfile, and I see
that "-p America/New_York" is hard-wired into it:

zic -y ./yearistype -d zoneinfo -L /dev/null -p America/New_York $FILES

This means that IANA's change of their sample Makefile will have no
direct impact, and things will only change if the Red Hat packager
actively changes the specfile.  It's still anyone's guess whether
he/she will do so, but the odds of a change seem a good bit lower
than if the IANA-supplied Makefile were being used directly.

I'm less familiar with Debian so I won't venture to dig into their
package, but maybe somebody else would like to.

regards, tom lane

Re: pg_dump, gzwrite, and errno

[Alvaro Herrera]

On 2020-Jun-18, Tom Lane wrote:

> Surely it's insufficient as-is, because there is no reason to suppose
> that errno is zero at entry.  You'd need to set errno = 0 first.

Oh, right.

> Also it's fairly customary in our sources to include a comment about
> this machination; so the full ritual is usually more like

Yeah, I had that in my local copy.  Done like that in all the most
obvious places.  But there are more places that are still wrong: I
believe every single place that calls WRITE_ERROR_EXIT is doing the
wrong thing.

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

Re: pg_regress cleans up tablespace twice.

[Thomas Munro]

On Thu, Jun 18, 2020 at 1:42 PM Michael Paquier  wrote:
> Thanks, applied this part to HEAD then after more testing.

Hmm, somehow this (well I guess it's this commit based on timing and
the area it touches, not sure exactly why) made cfbot's Windows build
fail, like this:

--- C:/projects/postgresql/src/test/regress/expected/tablespace.out
2020-06-19 21:26:24.661817000 +
+++ C:/projects/postgresql/src/test/regress/results/tablespace.out
2020-06-19 21:26:28.613257500 +
@@ -2,83 +2,78 @@
CREATE TABLESPACE regress_tblspacewith LOCATION
'C:/projects/postgresql/src/test/regress/testtablespace' WITH
(some_nonexistent_parameter = true); -- fail
ERROR: unrecognized parameter "some_nonexistent_parameter"
CREATE TABLESPACE regress_tblspacewith LOCATION
'C:/projects/postgresql/src/test/regress/testtablespace' WITH
(random_page_cost = 3.0); -- ok
+ERROR: could not set permissions on directory
"C:/projects/postgresql/src/test/regress/testtablespace": Permission
denied

Any ideas?  Here's what it does:

https://github.com/macdice/cfbot/tree/master/appveyor

Re: update substring pattern matching syntax

[Vik Fearing]

On 6/19/20 11:42 AM, Peter Eisentraut wrote:
> At
> 
> it is described that the substring pattern matching syntax in PostgreSQL
> does not conform to the current standard.  PostgreSQL implements
> 
>     SUBSTRING(text FROM pattern FOR escapechar)
> 
> whereas the current standard says
> 
>     SUBSTRING(text SIMILAR pattern ESCAPE escapechar)
> 
> The former was in SQL99, but the latter has been there since SQL:2003.
> 
> It's pretty easy to implement the second form also, so here is a patch
> that does that.


Oh good, this was on my list (I added that item to the wiki).

The patches look straightforward to me.  The grammar cleanup patch makes
things easier to read indeed.  At first I didn't see a test left over
for the old syntax, but it's there so this is all LGTM.

Thanks for doing this!
-- 
Vik Fearing

Re: Review for GetWALAvailability()

[Alvaro Herrera]

On 2020-Jun-17, Kyotaro Horiguchi wrote:

> @@ -9524,7 +9533,7 @@ GetWALAvailability(XLogRecPtr targetLSN)
>* the first WAL segment file since startup, which causes the status 
> being
>* wrong under certain abnormal conditions but that doesn't actually 
> harm.
>*/
> - oldestSeg = XLogGetLastRemovedSegno() + 1;
> + oldestSeg = last_removed_seg + 1;
>  
>   /* calculate oldest segment by max_wal_size and wal_keep_segments */
>   XLByteToSeg(currpos, currSeg, wal_segment_size);

This hunk should have updated the comment two lines above.  However:

> @@ -272,6 +273,14 @@ pg_get_replication_slots(PG_FUNCTION_ARGS)
>   rsinfo->setResult = tupstore;
>   rsinfo->setDesc = tupdesc;
>  
> + /*
> +  * Remember the last removed segment at this point for the consistency 
> in
> +  * this table. Since there's no interlock between slot data and
> +  * checkpointer, the segment can be removed in-between, but that doesn't
> +  * make any practical difference.
> +  */
> + last_removed_seg = XLogGetLastRemovedSegno();

I am mystified as to why you added this change.  I understand that your
point here is to make all slots reported their state as compared to the
same LSN, but why do it like that?  If a segment is removed in between,
it could mean that the view reports more lies than it would if we update
the segno for each slot.  I mean, suppose two slots are lagging behind
and one is reported as 'extended' because when we compute it it's still
in range; then a segment is removed.  With your coding, we'll report
both as extended, but with the original coding, we'll report the new one
as lost.  By the time the user reads the result, they'll read one
incorrect report with the original code, and two incorrect reports with
your code.  So ... yes it might be more consistent, but what does that
buy the user?

OTOH it makes GetWALAvailability gain a new argument, which we have to
document.

> + /*
> +  * However segments required by the slot has been lost, if walsender is
> +  * active the walsender can read into the first reserved slot.
> +  */
> + if (slot_is_active)
> + return WALAVAIL_BEING_REMOVED;

I don't understand this comment; can you please clarify what you mean?

I admit I don't like this slot_is_active argument you propose to add to
GetWALAvailability either; previously the function can be called with
an LSN coming from anywhere, not just a slot; the new argument implies
that the LSN comes from a slot.  (Your proposed patch doesn't document
this one either.)

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

Re: Cache relation sizes?

[Thomas Munro]

On Sat, Apr 11, 2020 at 4:10 PM Thomas Munro  wrote:
> I received a report off-list from someone who experimented with the
> patch I shared earlier on this thread[1], using a crash recovery test
> similar to one I showed on the WAL prefetching thread[2] (which he was
> also testing, separately).

Rebased.  I'll add this to the open commitfest.
From 03662f6a546329b58d41897d3b8096d5794eacdc Mon Sep 17 00:00:00 2001
From: Thomas Munro 
Date: Tue, 31 Dec 2019 13:22:49 +1300
Subject: [PATCH v2] Cache smgrnblocks() results in recovery.

Avoid repeatedly calling lseek(SEEK_END) during recovery by caching
the size of each fork.  For now, we can't use the same technique in
other processes (for example: query planning and seq scans are two
sources of high frequency lseek(SEEK_END) calls), because we lack a
shared invalidation mechanism.

Do this by generalizing the pre-existing caching used by FSM and VM
to support all forks.

Discussion: https://postgr.es/m/CAEepm%3D3SSw-Ty1DFcK%3D1rU-K6GSzYzfdD4d%2BZwapdN7dTa6%3DnQ%40mail.gmail.com
---
 contrib/pg_visibility/pg_visibility.c |  2 +-
 src/backend/access/heap/visibilitymap.c   | 18 -
 src/backend/catalog/storage.c |  4 +-
 src/backend/storage/freespace/freespace.c | 27 +++--
 src/backend/storage/smgr/smgr.c   | 49 +--
 src/include/storage/smgr.h| 12 +++---
 6 files changed, 66 insertions(+), 46 deletions(-)

diff --git a/contrib/pg_visibility/pg_visibility.c b/contrib/pg_visibility/pg_visibility.c
index 68d580ed1e..e731161734 100644
--- a/contrib/pg_visibility/pg_visibility.c
+++ b/contrib/pg_visibility/pg_visibility.c
@@ -392,7 +392,7 @@ pg_truncate_visibility_map(PG_FUNCTION_ARGS)
 	check_relation_relkind(rel);
 
 	RelationOpenSmgr(rel);
-	rel->rd_smgr->smgr_vm_nblocks = InvalidBlockNumber;
+	rel->rd_smgr->smgr_cached_nblocks[VISIBILITYMAP_FORKNUM] = InvalidBlockNumber;
 
 	block = visibilitymap_prepare_truncate(rel, 0);
 	if (BlockNumberIsValid(block))
diff --git a/src/backend/access/heap/visibilitymap.c b/src/backend/access/heap/visibilitymap.c
index 0a51678c40..b1072183bc 100644
--- a/src/backend/access/heap/visibilitymap.c
+++ b/src/backend/access/heap/visibilitymap.c
@@ -561,17 +561,16 @@ vm_readbuf(Relation rel, BlockNumber blkno, bool extend)
 	 * If we haven't cached the size of the visibility map fork yet, check it
 	 * first.
 	 */
-	if (rel->rd_smgr->smgr_vm_nblocks == InvalidBlockNumber)
+	if (rel->rd_smgr->smgr_cached_nblocks[VISIBILITYMAP_FORKNUM] == InvalidBlockNumber)
 	{
 		if (smgrexists(rel->rd_smgr, VISIBILITYMAP_FORKNUM))
-			rel->rd_smgr->smgr_vm_nblocks = smgrnblocks(rel->rd_smgr,
-		VISIBILITYMAP_FORKNUM);
+			smgrnblocks(rel->rd_smgr, VISIBILITYMAP_FORKNUM);
 		else
-			rel->rd_smgr->smgr_vm_nblocks = 0;
+			rel->rd_smgr->smgr_cached_nblocks[VISIBILITYMAP_FORKNUM] = 0;
 	}
 
 	/* Handle requests beyond EOF */
-	if (blkno >= rel->rd_smgr->smgr_vm_nblocks)
+	if (blkno >= rel->rd_smgr->smgr_cached_nblocks[VISIBILITYMAP_FORKNUM])
 	{
 		if (extend)
 			vm_extend(rel, blkno + 1);
@@ -641,11 +640,13 @@ vm_extend(Relation rel, BlockNumber vm_nblocks)
 	 * Create the file first if it doesn't exist.  If smgr_vm_nblocks is
 	 * positive then it must exist, no need for an smgrexists call.
 	 */
-	if ((rel->rd_smgr->smgr_vm_nblocks == 0 ||
-		 rel->rd_smgr->smgr_vm_nblocks == InvalidBlockNumber) &&
+	if ((rel->rd_smgr->smgr_cached_nblocks[VISIBILITYMAP_FORKNUM] == 0 ||
+		 rel->rd_smgr->smgr_cached_nblocks[VISIBILITYMAP_FORKNUM] == InvalidBlockNumber) &&
 		!smgrexists(rel->rd_smgr, VISIBILITYMAP_FORKNUM))
 		smgrcreate(rel->rd_smgr, VISIBILITYMAP_FORKNUM, false);
 
+	/* Invalidate cache so that smgrnblocks() asks the kernel. */
+	rel->rd_smgr->smgr_cached_nblocks[VISIBILITYMAP_FORKNUM] = InvalidBlockNumber;
 	vm_nblocks_now = smgrnblocks(rel->rd_smgr, VISIBILITYMAP_FORKNUM);
 
 	/* Now extend the file */
@@ -667,8 +668,5 @@ vm_extend(Relation rel, BlockNumber vm_nblocks)
 	 */
 	CacheInvalidateSmgr(rel->rd_smgr->smgr_rnode);
 
-	/* Update local cache with the up-to-date size */
-	rel->rd_smgr->smgr_vm_nblocks = vm_nblocks_now;
-
 	UnlockRelationForExtension(rel, ExclusiveLock);
 }
diff --git a/src/backend/catalog/storage.c b/src/backend/catalog/storage.c
index ec143b640a..9e6e6c42d3 100644
--- a/src/backend/catalog/storage.c
+++ b/src/backend/catalog/storage.c
@@ -290,8 +290,8 @@ RelationTruncate(Relation rel, BlockNumber nblocks)
 	 * Make sure smgr_targblock etc aren't pointing somewhere past new end
 	 */
 	rel->rd_smgr->smgr_targblock = InvalidBlockNumber;
-	rel->rd_smgr->smgr_fsm_nblocks = InvalidBlockNumber;
-	rel->rd_smgr->smgr_vm_nblocks = InvalidBlockNumber;
+	for (int i = 0; i <= MAX_FORKNUM; ++i)
+		rel->rd_smgr->smgr_cached_nblocks[i] = InvalidBlockNumber;
 
 	/* Prepare for truncation of MAIN fork of the relation */
 	forks[nforks] = MAIN_FORKNUM;
diff --git a/src/backend/storage/freespace/freespace.c b/src/backend/storage/freespace/freespace.c
index 9

Re: hash as an search key and hash collision

[Andy Fan]

On Sat, Jun 20, 2020 at 12:34 AM Tomas Vondra 
wrote:

> On Fri, Jun 19, 2020 at 04:24:01PM +0800, Andy Fan wrote:
> >I want to maintain an internal table which the primary key is sql_text and
> >planstmt::text, it is efficient since it both may be very long.  So a
> >general
> >idea is to use sql_hash_value and plan_hash_value. Then we have to
> >handle the hash collision case.  However I checked the codes both in
> >sr_plans[1]
> >and pg_stat_statements[2],  both of them didn't handle such cases, IIUC.
> so
> >how can I understand this situation?
> >
>
> IIRC pg_stat_statements simply accepts the hash collision risk. This is
> what the docs say:
>
>  In some cases, queries with visibly different texts might get merged
>  into a single pg_stat_statements entry. Normally this will happen
>  only for semantically equivalent queries, but there is a small
>  chance of hash collisions causing unrelated queries to be merged
>  into one entry. (This cannot happen for queries belonging to
>  different users or databases, however.)
>
> The consequences of a hash collision are relatively harmless, enough to
> make it not worth the extra checks (e.g. because the SQL text may not be
> available in memory and would need to be read from the file).
>

I see.  Thank you for this information,  this does make sense.

I suppose sr_plan does the same thing, but I haven't checked.
>

sr_plans is used to map a sql hash value to a PlannedStmts,  if hash
collisions
happen,  it may execute a query B while the user wants to execute Query A.
this should be more sensitive than pg_stat_statements which doesn't require
exact data.   I added Ildus who is the author of sr_plan to the cc list
in case he wants to take a look.

-- 
Best Regards
Andy Fan

Re: min_safe_lsn column in pg_replication_slots view

[Fujii Masao]

On 2020/06/19 21:15, Michael Paquier wrote:

On Fri, Jun 19, 2020 at 05:34:01PM +0900, Fujii Masao wrote:

On 2020/06/19 16:43, Kyotaro Horiguchi wrote:

At Fri, 19 Jun 2020 16:36:09 +0900, Michael Paquier  wrote 
in

So we usually avoid to do that between betas, but my take here is that
a catalog bump is better than regretting a change we may have to live
with after the release is sealed.


Sounds reasonable.


If we want to make this happen, I am afraid that the time is short as
beta2 is planned for next week.  As the version will be likely tagged
by Monday US time, it would be good to get this addressed within 48
hours to give some room to the buildfarm to react.  Attached is a
straight-forward proposal of patch.  Any thoughts?


It's better if we can do that. But I think that we should hear Alvaro's opinion
about this before rushing to push the patch. Even if we miss beta2 as the result
of that, I'm ok. We would be able to push something better into beta3.
So, CC Alvaro.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

Re: min_safe_lsn column in pg_replication_slots view

[Alvaro Herrera]

On 2020-Jun-20, Fujii Masao wrote:

> It's better if we can do that. But I think that we should hear Alvaro's 
> opinion
> about this before rushing to push the patch. Even if we miss beta2 as the 
> result
> of that, I'm ok. We would be able to push something better into beta3.
> So, CC Alvaro.

Uh, I was not aware of this thread.  I'll go over it now and let you
know.

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

Re: min_safe_lsn column in pg_replication_slots view

[Alvaro Herrera]

On 2020-Jun-19, Michael Paquier wrote:

> If we want to make this happen, I am afraid that the time is short as
> beta2 is planned for next week.  As the version will be likely tagged
> by Monday US time, it would be good to get this addressed within 48
> hours to give some room to the buildfarm to react.  Attached is a
> straight-forward proposal of patch.  Any thoughts?

I don't disagree with removing the LSN column, but at the same time we
need to provide *some* way for users to monitor this, so let's add a
function to extract the value they need for that.  It seems simple
enough.

I cannot implement it myself now, though.  I've reached the end of my
week and I'm not sure I'll be able to work on it during the weekend.

I agree with Kyotaro's opinion that the pg_walfile_name() function seems
too single-minded ...

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

Re: pg_regress cleans up tablespace twice.

[Michael Paquier]

On Sat, Jun 20, 2020 at 09:33:26AM +1200, Thomas Munro wrote:
> Hmm, somehow this (well I guess it's this commit based on timing and
> the area it touches, not sure exactly why) made cfbot's Windows build
> fail, like this:
> 
> --- C:/projects/postgresql/src/test/regress/expected/tablespace.out
> 2020-06-19 21:26:24.661817000 +
> +++ C:/projects/postgresql/src/test/regress/results/tablespace.out
> 2020-06-19 21:26:28.613257500 +
> @@ -2,83 +2,78 @@
> CREATE TABLESPACE regress_tblspacewith LOCATION
> 'C:/projects/postgresql/src/test/regress/testtablespace' WITH
> (some_nonexistent_parameter = true); -- fail
> ERROR: unrecognized parameter "some_nonexistent_parameter"
> CREATE TABLESPACE regress_tblspacewith LOCATION
> 'C:/projects/postgresql/src/test/regress/testtablespace' WITH
> (random_page_cost = 3.0); -- ok
> +ERROR: could not set permissions on directory
> "C:/projects/postgresql/src/test/regress/testtablespace": Permission
> denied
> 
> Any ideas?  Here's what it does:
> 
> https://github.com/macdice/cfbot/tree/master/appveyor

I am not sure, and I am not really familiar with this stuff.  Your
code does a simple vcregress check, and that should take care of
automatically cleaning up the testtablespace path.  The buildfarm uses
this code for MSVC builds and does not complain, nor do my own VMs
complain.  A difference in the processing after 2b2a070d is that the
tablespace cleanup/creation does not happen while holding a restricted 
token [1] anymore because it got out of pg_regress.c.  Are there any
kind of restrictions applied to the user running appveyor on Windows?

[1]: https://docs.microsoft.com/en-us/windows/win32/secauthz/restricted-tokens
--
Michael


signature.asc
Description: PGP signature

Re: pg_regress cleans up tablespace twice.

[Thomas Munro]

On Sat, Jun 20, 2020 at 2:42 PM Michael Paquier  wrote:
> > +ERROR: could not set permissions on directory
> > "C:/projects/postgresql/src/test/regress/testtablespace": Permission
> > denied
> >
> > Any ideas?  Here's what it does:
> >
> > https://github.com/macdice/cfbot/tree/master/appveyor
>
> I am not sure, and I am not really familiar with this stuff.  Your
> code does a simple vcregress check, and that should take care of
> automatically cleaning up the testtablespace path.  The buildfarm uses
> this code for MSVC builds and does not complain, nor do my own VMs
> complain.  A difference in the processing after 2b2a070d is that the
> tablespace cleanup/creation does not happen while holding a restricted
> token [1] anymore because it got out of pg_regress.c.  Are there any
> kind of restrictions applied to the user running appveyor on Windows?

Thanks for the clue.  Appveyor runs your build script as a privileged
user (unlike, I assume, the build farm animals), and that has caused a
problem with this test in the past, though I don't know the details.
I might go and teach it to skip that test until a fix can be found.

Re: Failures with installcheck and low work_mem value in 13~

[Michael Paquier]

On Fri, Jun 19, 2020 at 10:28:56AM -0700, Peter Geoghegan wrote:
> On Fri, Jun 19, 2020 at 10:27 AM Tom Lane  wrote:
>> I'm a bit skeptical about changing anything here.  There are quite
>> a large number of GUCs that can affect the regression results, and
>> it wouldn't be sane to try to force them all to fixed values.  For
>> one thing, that'd be a PITA to maintain, and for another, it's not
>> infrequently useful to run the tests with nonstandard settings to
>> see what happens.
> 
> +1

We cared about such plan stability that in the past FWIW, see for
example c588df9 as work_mem is a setting that people like to change.
Why should this be different?  work_mem is a popular configuration
setting.  Perhaps people will not complain about that being an issue
if running installcheck, we'll know with the time.  Anyway, I am fine
to just change my default configuration if the conclusion is to not
touch that and let it be, but I find a bit annoying that switching
work_mem from 4MB to 1MB is enough to destabilize the tests.  And this
worked just fine in past releases.
--
Michael


signature.asc
Description: PGP signature

Re: min_safe_lsn column in pg_replication_slots view

[Amit Kapila]

On Sat, Jun 20, 2020 at 7:12 AM Alvaro Herrera  wrote:
>
> On 2020-Jun-19, Michael Paquier wrote:
>
> > If we want to make this happen, I am afraid that the time is short as
> > beta2 is planned for next week.  As the version will be likely tagged
> > by Monday US time, it would be good to get this addressed within 48
> > hours to give some room to the buildfarm to react.  Attached is a
> > straight-forward proposal of patch.  Any thoughts?
>
> I don't disagree with removing the LSN column, but at the same time we
> need to provide *some* way for users to monitor this, so let's add a
> function to extract the value they need for that.  It seems simple
> enough.
>

Isn't this information specific to checkpoints, so maybe better to
display in view pg_stat_bgwriter?

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

Re: pg_regress cleans up tablespace twice.

[Michael Paquier]

On Sat, Jun 20, 2020 at 03:01:36PM +1200, Thomas Munro wrote:
> Thanks for the clue.  Appveyor runs your build script as a privileged
> user (unlike, I assume, the build farm animals), and that has caused a
> problem with this test in the past, though I don't know the details.
> I might go and teach it to skip that test until a fix can be found.

Thanks, I was not aware of that.  Is it a fix that involves your code
or something else?  How long do you think it would take to address
that?  Another strategy that we could do is also a revert of 2b2a070
for now to allow the cfbot to go through and then register this thread
in the CF app to allow the bot to pick it up and test it, so as there
is more room to get a fix.  The next CF is in ten days, so it would be
annoying to reduce the automatic test coverage the cfbot provides :/
--
Michael


signature.asc
Description: PGP signature

Re: min_safe_lsn column in pg_replication_slots view

[Michael Paquier]

On Sat, Jun 20, 2020 at 09:45:52AM +0530, Amit Kapila wrote:
> On Sat, Jun 20, 2020 at 7:12 AM Alvaro Herrera  
> wrote:
>> I don't disagree with removing the LSN column, but at the same time we
>> need to provide *some* way for users to monitor this, so let's add a
>> function to extract the value they need for that.  It seems simple
>> enough.
> 
> Isn't this information specific to checkpoints, so maybe better to
> display in view pg_stat_bgwriter?

Not sure that's a good match.  If we decide to expose that, a separate
function returning a LSN based on the segment number from
XLogGetLastRemovedSegno() sounds fine to me, like
pg_wal_last_recycled_lsn().  Perhaps somebody has a better name in
mind?
--
Michael


signature.asc
Description: PGP signature