Re: heapam_index_build_range_scan's anyvisible

[Michael Paquier]

On Wed, Sep 25, 2019 at 10:24:05PM -0700, Ashwin Agrawal wrote:
> Thanks for reporting, I did indeed missed out contrib. Please find attached
> the v2 of the patch which includes the change required in contrib as well.

Okay, that makes sense.  The patch looks good to me so I have switched
it to ready for committer.  Andres, Robert, would you prefer
committing this one yourself?  If not, I'll take care of it tomorrow
after a second look.
--
Michael


signature.asc
Description: PGP signature

Re: pgbench - extend initialization phase control

[btkimurayuzk]

2019-11-06 11:31 に Fujii Masao さんは書きました:
On Wed, Nov 6, 2019 at 6:23 AM Fabien COELHO  
wrote:



Hello,

>>> - for (step = initialize_steps; *step != '\0'; step++)
>>> + for (const char *step = initialize_steps; *step != '\0'; step++)
>
> But I still wonder why we should apply such change here.

Because it removes one declaration and reduces the scope of one 
variable?


> If there is the reason why this change is necessary here,

Nope, such changes are never necessary.

> I'm OK with that. But if not, basically I'd like to avoid the change.
> Otherwise it may make the back-patch a bit harder
> when we change the surrounding code.

I think that this is small enough so that it can be managed, if any 
back

patch occurs on the surrounding code, which is anyway pretty unlikely.

> Attached is the slightly updated version of the patch. Based on your
> patch, I added the descriptions about logging of "g" and "G" steps into
> the doc, and did some cosmetic changes. Barrying any objections,
> I'm thinking to commit this patch.

I'd suggest:

"to print one message each ..." -> "to print one message every ..."

"to print no progress ..." -> "not to print any progress ..."

I would not call "fprintf(stderr" twice in a row if I can call it 
once.


Thanks for the suggestion!
I updated the patch in that way and committed it!

This commit doesn't include the change "for (const char ...)"
and "merge two fprintf into one" ones that we were discussing.
Because they are trivial but I'm not sure if they are improvements
or not, yet. If they are, probably it's better to apply such changes
to all the places having the similar issues. But that seems overkill.



> While reviewing the patch, I found that current code allows space
> character to be specified in -I. That is, checkInitSteps() accepts
> space character. Why should we do this?

> Probably I understand why runInitSteps() needs to accept space character
> (because "v" in the specified string with -I is replaced with a space
> character when --no-vacuum option is given).

Yes, that is the reason, otherwise the string would have to be 
shifted.


> But I'm not sure why that's also necessary in checkInitSteps(). Instead,
> we should treat a space character as invalid in checkInitSteps()?

I think that it may break --no-vacuum, and I thought that there may be
other option which remove things, eventually. Also, having a NO-OP 
looks

ok to me.


As far as I read the code, checkInitSteps() checks the initialization
steps that users specified. The initialization steps string that
"v" was replaced with blank character is not given to checkInitSteps().
So ISTM that dropping the handling of blank character from
checkInitSteps() doesn't break --no-vacuum.


This is a patch which does not allow space character in -I options .

Regard,
Yu Kimura

diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 14dbc4510c..95b23895ff 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -4080,7 +4080,7 @@ checkInitSteps(const char *initialize_steps)
 
 	for (const char *step = initialize_steps; *step != '\0'; step++)
 	{
-		if (strchr(ALL_INIT_STEPS " ", *step) == NULL)
+		if (strchr(ALL_INIT_STEPS, *step) == NULL)
 		{
 			fprintf(stderr,
 	"unrecognized initialization step \"%c\"\n",

Re: Add SQL function to show total block numbers in the relation

[btkimurayuzk]

btkimurayuzk  writes:

I propose new simple sql query, which shows total block numbers in the
relation.
...
Of cource, we can know this value such as
select (pg_relation_size('t') /
current_setting('block_size')::bigint)::int;


I don't really see why the existing solution isn't sufficient.


I think it's a little difficult to introduce the block size using two 
values `current block size` and `reference size`
for beginners who are not familiar with the internal structure of 
Postgres,


This is the reason why the existing solution was insufficient.

What do you think?

Regards,
Yu Kimura

Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.

[Gilles Darold]

Hi Kyotaro,

Le 07/11/2019 à 08:10, Kyotaro Horiguchi a écrit :
> Hello.
>
> At Wed, 6 Nov 2019 20:13:10 +0900, Etsuro Fujita  
> wrote in 
>> Hi Michael-san,
>>
>> On Wed, Nov 6, 2019 at 4:35 PM Michael Paquier  wrote:
>>> On Wed, Nov 06, 2019 at 03:12:04PM +0900, Etsuro Fujita wrote:
 On Wed, Nov 6, 2019 at 1:13 PM Michael Paquier  wrote:
> "postgres_fdw foreign tables" sounds weird to me.  Could "foreign
> tables using postgres_fdw" be a better wording?  I am wondering as
> well if we should not split this information into two parts: one for
> the actual error message which only mentions foreign tables, and a
> second one with a hint to mention that postgres_fdw has been used.
 We use "postgres_fdw foreign tables" or "postgres_fdw tables" in
 release notes, so I thought it was OK to use that in error messages as
 well.  But actually, these wordings are not suitable for error
 messages?
>>> It is true that the docs of postgres_fdw use that and that it is used
>>> in some comments.  Still, I found this wording a bit weird..  If you
>>> think that what you have is better, I am also fine to let you have the
>>> final word, so please feel to ignore me :)
>> I'd like to hear the opinions of others.
> FWIW, I see it a bit weird, too. And perhaps "prepare" should be in
> upper case letters. Plus, any operation including a SELECT on a
> temporary table inhibits PREAPRE TRANSACTION, but the same on a
> postgres_fdw foreign tables is not. So the error message is rather
> wrong.


This is not what I've experienced, see the first message of the thread.
A SELECT on foreign table prevent to use PREPARE TRANSACTION like with
temporary table. Perhaps postgres_fdw should not throw an error with
readonly queries on foreign tables but I guess that it is pretty hard to
know especially on a later PREPARE event. But maybe I'm wrong, it is not
easy every day :-) Can you share the SQL code you have executed to allow
PREPARE transaction after a SELECT on a postgres_fdw foreign table?


-- 
Gilles Darold

Re: Do we have a CF manager for November?

[Michael Paquier]

On Tue, Nov 05, 2019 at 08:50:54PM +0500, Ibrar Ahmed wrote:
> On Tue, Nov 5, 2019 at 7:18 AM Michael Paquier  wrote:
>> That may have been me.  I can take this one if there is nobody else
>> around.

Okay, so it is.  I have begun browsing the patch history, and we have
a lt of work ahead.

> I am ready to help you with this.

Any help is welcome, thanks.
--
Michael


signature.asc
Description: PGP signature

Re: Collation versioning

[Julien Rouhaud]

On Mon, Nov 4, 2019 at 9:59 PM Thomas Munro  wrote:
>
> On Tue, Nov 5, 2019 at 4:18 AM Julien Rouhaud  wrote:
> > On Mon, Nov 4, 2019 at 11:13 AM Julien Rouhaud  wrote:
> > > On Mon, Nov 4, 2019 at 4:58 AM Thomas Munro  
> > > wrote:
> > > > Here are some problems to think about:
> > > >
> > > > * We'd need to track dependencies on the default collation once we
> > > > have versioning for that [...]
> >
> > Another problem I just thought about is how to avoid discrepancy of
> > collation version for indexes on shared objects, such as
> > pg_database_datname_index.
>
> I didn't look closely at the code, but I think when "name" recently
> became collation-aware (commit 586b98fd), it switched to using
> C_COLLATION_OID as its typcollation, and "C" doesn't need versioning,
> so I think it would only be a problem if there are shared catalogs
> that have "name" columns that have a non-type-default collation.
> There are none of those, and you can't create them, right?  If there
> were, if we take this patch set to its logical conclusion, we'd also
> need pg_shdepend.refobjversion, but we don't need it AFAICS.

That's entirely correct, I should have checked that.

Re: pgbench - extend initialization phase control

[Fabien COELHO]

I think that it may break --no-vacuum, and I thought that there may be
other option which remove things, eventually. Also, having a NO-OP looks
ok to me.


As far as I read the code, checkInitSteps() checks the initialization
steps that users specified. The initialization steps string that
"v" was replaced with blank character is not given to checkInitSteps().
So ISTM that dropping the handling of blank character from
checkInitSteps() doesn't break --no-vacuum.


This is a patch which does not allow space character in -I options .


I do not think that this is desirable. It would be a regression, and 
allowing a no-op is not an issue in anyway.


--
Fabien Coelho - CRI, MINES ParisTech

Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.

[Etsuro Fujita]

Horiguchi-san,

On Thu, Nov 7, 2019 at 4:11 PM Kyotaro Horiguchi
 wrote:
> At Wed, 6 Nov 2019 20:13:10 +0900, Etsuro Fujita  
> wrote in
> > On Wed, Nov 6, 2019 at 4:35 PM Michael Paquier  wrote:
> > > On Wed, Nov 06, 2019 at 03:12:04PM +0900, Etsuro Fujita wrote:
> > > > On Wed, Nov 6, 2019 at 1:13 PM Michael Paquier  
> > > > wrote:
> > > >> "postgres_fdw foreign tables" sounds weird to me.  Could "foreign
> > > >> tables using postgres_fdw" be a better wording?  I am wondering as
> > > >> well if we should not split this information into two parts: one for
> > > >> the actual error message which only mentions foreign tables, and a
> > > >> second one with a hint to mention that postgres_fdw has been used.
> > > >
> > > > We use "postgres_fdw foreign tables" or "postgres_fdw tables" in
> > > > release notes, so I thought it was OK to use that in error messages as
> > > > well.  But actually, these wordings are not suitable for error
> > > > messages?
> > >
> > > It is true that the docs of postgres_fdw use that and that it is used
> > > in some comments.  Still, I found this wording a bit weird..  If you
> > > think that what you have is better, I am also fine to let you have the
> > > final word, so please feel to ignore me :)
> >
> > I'd like to hear the opinions of others.
>
> FWIW, I see it a bit weird, too.

Only two people complaining about the wording?  Considering as well
that we use that wording in the docs and there were no complains about
that IIRC, I don't feel a need to change that, TBH.

> And perhaps "prepare" should be in
> upper case letters.

Seems like a good idea.

> Plus, any operation including a SELECT on a
> temporary table inhibits PREAPRE TRANSACTION, but the same on a
> postgres_fdw foreign tables is not. So the error message is rather
> wrong.
>
> A verbose alternative can be:
>
> "cannot PREPARE a transaction that has modified data on foreign tables using 
> postgres_fdw"

I don't think that's better, because that doesn't address the original
issue reported in this thread, as Gilles pointed out just before in
his email.  See the commit message in the patch I posted.

Thanks for the comments!

Best regards,
Etsuro Fujita

Re: Keep compiler silence (clang 10, implicit conversion from 'long' to 'double' )

[Yuya Watari]

Hello Horiguchi-san,

On Thu, Nov 7, 2019 at 3:10 PM Kyotaro Horiguchi
 wrote:
> Mmm? See the bit in the patch cited below (v5).
>
> +   /* Range check */
> +   if (unlikely(!FLOAT8_FITS_IN_INT32(num)) || isnan(num))
>
> If compiler doesn't any fancy, num is fed to an arithmetic before
> checking if it is NaN. That seems have a chance of exception.

Thank you for pointing it out. That's my mistake. I fixed it and
attached the patch.

Best regards,
Yuya Watari
NTT Software Innovation Center
watari.y...@gmail.com


v6-keep-compiler-silence.patch
Description: Binary data

Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.

[Kyotaro Horiguchi]

Hello Gilles. I made a silly mistake.

At Thu, 7 Nov 2019 09:05:55 +0100, Gilles Darold  wrote in 
> > FWIW, I see it a bit weird, too. And perhaps "prepare" should be in
> > upper case letters. Plus, any operation including a SELECT on a
> > temporary table inhibits PREAPRE TRANSACTION, but the same on a
> > postgres_fdw foreign tables is not. So the error message is rather
> > wrong.
> 
> 
> This is not what I've experienced, see the first message of the thread.
> A SELECT on foreign table prevent to use PREPARE TRANSACTION like with
> temporary table. Perhaps postgres_fdw should not throw an error with
> readonly queries on foreign tables but I guess that it is pretty hard to
> know especially on a later PREPARE event. But maybe I'm wrong, it is not
> easy every day :-) Can you share the SQL code you have executed to allow
> PREPARE transaction after a SELECT on a postgres_fdw foreign table?

Oooops!

After reading this, I came to be afraid that I did something wrong,
then I rechecked actual behavior. Finally I found that SELECT * FROM
foregn_tbl prohibits PREPARE TRANSACTION. I might have used a local
table instead of foreign tabel at the previous trial.

Sorry for the mistake and thank you for pointing it.

So my fixed proposals are:

"cannot PREPARE a transaction that has operated on foreign tables using 
postgres_fdw"

Or

"postgres_fdw doesn't support PREPARE of a transaction that has accessed 
foreign tables"

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.

[Kyotaro Horiguchi]

Hello, Fujita-san.

At Thu, 7 Nov 2019 17:20:07 +0900, Etsuro Fujita  
wrote in 
> Only two people complaining about the wording?  Considering as well
> that we use that wording in the docs and there were no complains about
> that IIRC, I don't feel a need to change that, TBH.
> 
> > And perhaps "prepare" should be in
> > upper case letters.
> 
> Seems like a good idea.
> 
> > Plus, any operation including a SELECT on a
> > temporary table inhibits PREAPRE TRANSACTION, but the same on a
> > postgres_fdw foreign tables is not. So the error message is rather
> > wrong.
> >
> > A verbose alternative can be:
> >
> > "cannot PREPARE a transaction that has modified data on foreign tables 
> > using postgres_fdw"
> 
> I don't think that's better, because that doesn't address the original
> issue reported in this thread, as Gilles pointed out just before in
> his email.  See the commit message in the patch I posted.

"modified" is my mistake as in the just posted mail. But the most
significant point in the previous mail is using "foreign tables using
postgres_fdw" instead of "postgres_fdw foreign tables". And the other
point is using different message from temporary tables.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.

[Kyotaro Horiguchi]

At Thu, 07 Nov 2019 17:27:47 +0900 (JST), Kyotaro Horiguchi 
 wrote in 
> "modified" is my mistake as in the just posted mail. But the most
> significant point in the previous mail is using "foreign tables using
> postgres_fdw" instead of "postgres_fdw foreign tables". And the other
> point is using different message from temporary tables.

I forgot to mention that the comment in XACT_EVENT_PRE_PREPARE
contains the same mistake and needs more or less the same fix.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: Minimal logical decoding on standbys

[Rahila Syed]

Hi Amit,

I am reading about this feature and reviewing it.
To start with, I reviewed the patch:
0005-Doc-changes-describing-details-about-logical-decodin.patch.

>prevent VACUUM from removing required rows from the system catalogs,
>hot_standby_feedback should be set on the standby. In spite of that,
>if any required rows get removed on standby, the slot gets dropped.
IIUC, you mean `if any required rows get removed on *the master* the slot
gets
dropped`, right?

Thank you,
-- 
Rahila Syed
Performance Engineer
2ndQuadrant
http://www.2ndQuadrant.com 
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: TAP tests aren't using the magic words for Windows file access

[Juan José Santamaría Flecha]

On Thu, Nov 7, 2019 at 1:57 AM Andrew Dunstan <
andrew.duns...@2ndquadrant.com> wrote:

>
> In any case, the patch will fail as written - on the Msys 1 system I
> just tested Win32::API is not available to the DTK perl we need to use
> to run TAP tests.
>
>
May I ask which version of Msys is that system using? In a recent
installation (post 1.0.11) I see that those modules are available.

Regards,

Juan José Santamaría Flecha

Re: pgbench - extend initialization phase control

[Fujii Masao]

On Thu, Nov 7, 2019 at 5:18 PM Fabien COELHO  wrote:
>
>
> >>> I think that it may break --no-vacuum, and I thought that there may be
> >>> other option which remove things, eventually. Also, having a NO-OP looks
> >>> ok to me.
> >>
> >> As far as I read the code, checkInitSteps() checks the initialization
> >> steps that users specified. The initialization steps string that
> >> "v" was replaced with blank character is not given to checkInitSteps().
> >> So ISTM that dropping the handling of blank character from
> >> checkInitSteps() doesn't break --no-vacuum.
> >>
> > This is a patch which does not allow space character in -I options .
>
> I do not think that this is desirable. It would be a regression, and
> allowing a no-op is not an issue in anyway.

Why is that regression, you think? I think that's an oversight.
If I'm missing something and accepting a blank character as no-op in
also checkInitSteps() is really necessary for some reasons,
which should be documented. But, if so, another question is;
why should only blank character be treated as no-op, in checkInitSteps()?

Regards,

-- 
Fujii Masao

Re: Add SQL function to show total block numbers in the relation

[Kyotaro Horiguchi]

Hello, Kimura-san.

At Thu, 07 Nov 2019 17:04:51 +0900, btkimurayuzk  
wrote in 
> > btkimurayuzk  writes:
> >> I propose new simple sql query, which shows total block numbers in the
> >> relation.
> >> ...
> >> Of cource, we can know this value such as
> >> select (pg_relation_size('t') /
> >> current_setting('block_size')::bigint)::int;
> > I don't really see why the existing solution isn't sufficient.
> 
> I think it's a little difficult to introduce the block size using two
> values `current block size` and `reference size`
> for beginners who are not familiar with the internal structure of
> Postgres,
> 
> This is the reason why the existing solution was insufficient.
> 
> What do you think?

Sorry, but I also vote -1 for the new function.

Size in block number is useless for those who doesn't understand the
notion of block, or block size. Those who understands the notion
should come up with the simple formula (except the annoying
casts). Anyone can find the clue to the base values by searching the
document in the Web with the keywords "block size" and "relation size"
or even with "table size". (FWIW, I would even do the same for the new
function if any...) If they need it so frequently, a user-defined
function is easily made up.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: alternative to PG_CATCH

[Peter Eisentraut]

On 2019-11-06 15:49, Tom Lane wrote:

Peter Eisentraut  writes:

On 2019-11-04 16:01, Tom Lane wrote:

Now that I've actually looked at the patched code, there's a far
more severe problem with it.  Namely, that use of PG_FINALLY
means that the "finally" segment is run without having popped
the error context stack, which means that any error thrown
within that segment will sigsetjmp right back to the top,
resulting in an infinite loop.  (Well, not infinite, because
it'll crash out once the error nesting depth limit is hit.)
We *must* pop the stack before running recovery code.



I can confirm that that indeed happens. :(



Here is a patch to fix it.


This seems all right from here.  Since PG_RE_THROW() is guaranteed
noreturn, I personally wouldn't have bothered with an "else" after it,
but that's just stylistic.


Committed, without the "else".

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

Re: Resume vacuum and autovacuum from interruption and cancellation

[btkimurayuzk]

+   VACOPT_RESUME = 1 << 8/* resume from the previous point */

I think this unused ENUM value is not needed.

Regards,

Yu Kimura

Re: Collation versioning

[Julien Rouhaud]

On Mon, Nov 4, 2019 at 11:13 AM Julien Rouhaud  wrote:
>
> On Mon, Nov 4, 2019 at 4:58 AM Thomas Munro  wrote:
> >
> > Unfortunately I haven't had time to work on it seriously, but here's a
> > quick rebase to get the proof-of-concept back into working shape.

Thanks!  I also removed the test for REFRESH VERSION command that was
forgotten in the patch set, and run a pgindent.

> > Here are some problems to think about:
> >
> > * We'd need to track dependencies on the default collation once we
> > have versioning for that (see
> > https://www.postgresql.org/message-id/flat/5e756dd6-0e91-d778-96fd-b1bcb06c161a%402ndquadrant.com).
> > That is how most people actually consume collations out there in real
> > life, and yet we don't normally track dependencies on the default
> > collation and I don't know if that's simply a matter of ripping out
> > all the code that looks like "xxx != DEFAULT_COLLATION_ID" in the
> > dependency analysis code or if there's more to it.
>
> This isn't enough.  What would remain is:
>
> - teach get_collation_version_for_oid() to return the  default
> collation name, which is simple
> - have recordDependencyOnVersion() actually records the dependency,
> which wouldn't happen as the default collation is pinned.
>
> An easy fix would be to teach isObjectPinned() to ignore
> CollationRelationId / DEFAULT_COLLATION_OID, but that's ugly and would
> allow too many dependencies to be stored.  Not pinning the default
> collation during initdb doesn't sound a good alternative either.
> Maybe adding a force flag or a new DependencyType that'd mean "normal
> but forced" would be ok?

Attached 4th patch handles default collation.  I went with an
ignore_systempin flag in recordMultipleDependencies.

>
> > * Andres mentioned off-list that pg_depend rows might get blown away
> > and recreated in some DDL circumstances.  We need to look into that.

I tried various flavour of DDL but I couldn't wipe out the pg_depend
rows without having an index rebuild triggered (like changing the
underlying column datatype).  Do you have any scenario where the index
rebuild wouldn't be triggered?

> > * Another is that pg_upgrade won't preserve pg_depend rows, so you'd
> > need some catalog manipulation (direct or via new DDL) to fix that.

Attached 5th patch add a new "ALTER INDEX idx_name DEPENDS ON
COLLATION coll_oid VERSION coll_version_text" that can only be
executed in binary upgrade mode, and teach pg_dump to generate such
commands (including for indexes created for constraints).  One issue
is that older versions don't have pg_depend information,  so pg_dump
can't find the dependencies to generate such commands and override the
version with anything else.  It means that the upgraded cluster will
show all indexes as depending on the current collation provider
version.  I'm not sure if that's the best thing to do, or if we should
change all pg_depend rows to mention "unknown" version or something
like that.  It would generate so much noise that it's probably not
worth it.

I didn't do anything for the spurious warning when running a reindex,
and kept original approach of pg_depend catalog.


0002-Add-pg_depend.refobjversion.patch
Description: Binary data


0005-Preserve-index-dependencies-on-collation-during-pg_u.patch
Description: Binary data


0001-Remove-pg_collation.collversion.patch
Description: Binary data


0004-Also-track-default-collation-version.patch
Description: Binary data


0003-Track-collation-versions-for-indexes.patch
Description: Binary data

Re: Does 'instead of delete' trigger support modification of OLD

[Eugen Konkov]

> I looked in the CREATE TRIGGER manual page and found this:

> https://www.postgresql.org/docs/12/sql-createtrigger.html
> If the trigger fires before or instead of the event, the trigger
> can skip the operation for the current row, or change the row
> being inserted (for INSERT and UPDATE operations only).

> I don't see the "(for INSERT and UPDATE operations only)" language in
> the main trigger documentation,
> https://www.postgresql.org/docs/current/trigger-definition.html.  I have
> written the attached patch to fix that.  Does that help?

No.   If   we document that PG does not allow to modify OLD at instead
of  trigger,  the  we can not implement that. Probably we can put note
that  "currently  modification of the trigger row for RETURNING is not
implemented"

> As far as allowing DELETE to modify the trigger row for RETURNING, I am
> not sure how much work it would take to allow that, but it seems like it
> is a valid requite, and if so, I can add it to the TODO list.

Yes,  Add please into TODO the feature to "allowing DELETE to modify the 
trigger row
for  RETURNING".  Becuase, as I have described at first letter, without
this the RETURNING rows **does not correspond actually deleted data**

Thank you.

-- 
Best regards,
Eugen Konkov

Re: [proposal] recovery_target "latest"

[Grigory Smolkin]

On 11/7/19 8:36 AM, Kyotaro Horiguchi wrote:

At Thu, 7 Nov 2019 02:28:39 +0300, Grigory Smolkin  
wrote in

On 11/6/19 1:55 PM, Grigory Smolkin wrote:

On 11/6/19 12:56 PM, Fujii Masao wrote:

On Wed, Nov 6, 2019 at 6:33 PM Grigory Smolkin
 wrote:

On 11/6/19 10:39 AM, Peter Eisentraut wrote:

This seems to also be related to this discussion:


Yes, in a way. Strengthening current lax recovery behavior is a very
good idea.


I like this idea.

I don't like the name "latest".  What does that mean?  Other
documentation talks about the "end of the archive".  What does that
mean?  It means until restore_command errors.  Let's think of a name
that reflects that better.  Maybe "all_archive" or something like
that.

As with "immediate", "latest" reflects the latest possible state this
PostgreSQL instance can achieve when using PITR. I think it is simple
and easy to understand for an end user, which sees PITR as a way to go
from one state to another. In my experience, at least, which is, of
course, subjective.

But if we want an argument name to be technically accurate, then, I
think, something like "end-of-available-WAL"/"all-WAL", "end-of-WAL"
is
a way to go.

What happens if this parameter is set to latest in the standby mode?
Or the combination of those settings should be prohibited?


Currently it will behave just like regular standby, so I think, to
avoid confusion and keep things simple, the combination of them should
be prohibited.
Thank you for pointing this out, I will work on it.

Attached new patch revision, now it is impossible to use
recovery_target 'latest' in standby mode.
TAP test is updated to reflect this behavior.

In the first place, latest (or anything it could be named as) is
defined as the explit label for the default behavior. Thus the latest
should work as if nothing is set to recovery_target* following the
definition.  That might seems somewhat strange but I think at least it
is harmless.



Well, it was more about getting default behavior by using some explicit 
recovery_target, not the other way around. Because it will break some 
3rd party backup and replication applications, that may rely on old 
behavior of ignoring recovery_target_action when no recovery_target is 
provided.

But you think that it is worth pursuing, I can do that.



recovery_target=immediate + r_t_action=shutdown for a standby works as
commanded. Do we need to inhibit that, too?


Why something, that work as expected, should be inhibited?





The other way around, as I see it, is to define RECOVERY_TARGET_LATEST
as something more complex, for example, the latest possible endptr in
latest WAL segment. But it is tricky, because WAL archive may keeps
growing as recovery is progressing or, in case of standby, master
keeps sending more and more WAL.

regards.


--
Grigory Smolkin
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: Does 'instead of delete' trigger support modification of OLD

[Eugen Konkov]

Hello Eugen,

Thursday, November 7, 2019, 11:20:32 AM, you wrote:

>> I looked in the CREATE TRIGGER manual page and found this:

>> https://www.postgresql.org/docs/12/sql-createtrigger.html
>> If the trigger fires before or instead of the event, the trigger
>> can skip the operation for the current row, or change the row
>> being inserted (for INSERT and UPDATE operations only).

>> I don't see the "(for INSERT and UPDATE operations only)" language in
>> the main trigger documentation,
>> https://www.postgresql.org/docs/current/trigger-definition.html.  I have
>> written the attached patch to fix that.  Does that help?

> No.   If   we document that PG does not allow to modify OLD at instead
> of  trigger,  the  we can not implement that. Probably we can put note
> that  "currently  modification of the trigger row for RETURNING is not
> implemented"

sorry, typo. Please read:
"currently  modification of the trigger row for DELETE RETURNING is 
notimplemented"


>> As far as allowing DELETE to modify the trigger row for RETURNING, I am
>> not sure how much work it would take to allow that, but it seems like it
>> is a valid requite, and if so, I can add it to the TODO list.

> Yes,  Add please into TODO the feature to "allowing DELETE to modify the 
> trigger row
> for  RETURNING".  Becuase, as I have described at first letter, without
> this the RETURNING rows **does not correspond actually deleted data**

> Thank you.




-- 
Best regards,
Eugen Konkov

Re: [Proposal] Global temporary tables

[曾文旌(义从)]

> 2019年11月7日 上午12:08，Konstantin Knizhnik  写道：
> 
> 
> 
> On 06.11.2019 16:24, 曾文旌(义从) wrote:
>> Dear Hackers
>> 
>> 
>> I attached the patch of GTT implementationI base on PG12.
>> The GTT design came from my first email.
>> Some limitations in patch will be eliminated in later versions.
>> 
>> Later, I will comment on Konstantin's patch and make some proposals for 
>> cooperation.
>> Looking forward to your feedback.
>> 
>> Thanks.
>> 
>> Zeng Wenjing
>> 
> 
> Thank you for this patch.
> My first comments:
> 
> 1.  I have ported you patch to the latest Postgres version (my patch is 
> attached).
> 2. You patch is supporting only B-Tree index for GTT. All other indexes 
> (hash, gin, gist, brin,...) are not currently supported.
Currently I only support btree index.
I noticed that your patch supports more index types, which is where I'd like to 
work with you.

> 3. I do not understand the reason for the following limitation:
> "We allow to create index on global temp table only this session use it"
> 
> First of all it seems to significantly reduce usage of global temp tables.
> Why do we need GTT at all? Mostly because we need to access temporary data in 
> more than one backend. Otherwise we can just use normal table.
> If temp table is expected to be larger enough, so that we need to create 
> index for it, then it is hard to believe that it will be needed only in one 
> backend.
> 
> May be the assumption is that all indexes has to be created before GTT start 
> to be used.
Yes, Currently, GTT's index is only supported and created in an empty table 
state, and other sessions are not using it.
There has two improvements pointer:
1 Index can create on GTT(A) when the GTT(A)  in the current session is not 
empty, requiring the GTT table to be empty in the other session.
Index_build needs to be done in the current session just like a normal table. 
This improvement is relatively easy.

2 Index can create on GTT(A)  when more than one session are using this GTT(A).
Because when I'm done creating an index of the GTT in this session and setting 
it to be an valid index, it's not true for the GTT in other sessions.
Indexes on gtt in other sessions require "rebuild_index" before using it. 
I don't have a better solution right now, maybe you have some suggestions.


> But right now this check is not working correctly in any case - if you insert 
> some data into the table, then
> you can not create index any more:
> 
> postgres=# create global temp table gtt(x integer primary key, y integer);
> CREATE TABLE
> postgres=# insert into gtt values (generate_series(1,10), 
> generate_series(1,10));
> INSERT 0 10
> postgres=# create index on gtt(y);
> ERROR:  can not create index when have one or more backend attached this 
> global temp table
> 
> I wonder why do you need such restriction?
> 
> 
> -- 
> Konstantin Knizhnik
> Postgres Professional: http://www.postgrespro.com
> The Russian Postgres Company
> 
> 

Re: pgbench - extend initialization phase control

[Fabien COELHO]

Hello Masao-san,


I do not think that this is desirable. It would be a regression, and
allowing a no-op is not an issue in anyway.


Why is that regression, you think?


Because "pgbench -I ' d'" currently works and it would cease to work after 
the patch.


I think that's an oversight. If I'm missing something and accepting a 
blank character as no-op in also checkInitSteps() is really necessary 
for some reasons, which should be documented. But, if so, another 
question is; why should only blank character be treated as no-op, in 
checkInitSteps()?


The idea is to have one character that can be substituted to remove any 
operation.


On principle, allowing a no-op character, whatever the choice, is a good 
idea, because it means that the caller can take advantage of that if need 
be.


I think that the actual oversight is that the checkInitSteps should be 
called at the beginning of processing initialization steps rather than 
while processing -I, because currently other places modify the 
initialization string (no-vacuum, foreign key) and thus are not checked.


I agree that it should be documented.

Attached patch adds a doc and moves the check where it should be, and 
modifies a test with an explicit no-op space initialization step.


--
Fabien.diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index 4c48a58ed2..5008377998 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -168,7 +168,8 @@ pgbench  options  d
 initialization steps to be performed, using one character per step.
 Each step is invoked in the specified order.
 The default is dtgvp.
-The available steps are:
+The space character is accepted as a no-op, and the other available
+steps are:
 
 
  
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 14dbc4510c..4178127c21 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -4104,6 +4104,9 @@ runInitSteps(const char *initialize_steps)
 	double		run_time = 0.0;
 	bool		first = true;
 
+	/* check that all steps are valid before executing them */
+	checkInitSteps(initialize_steps);
+
 	initPQExpBuffer(&stats);
 
 	if ((con = doConnect()) == NULL)
@@ -5501,7 +5504,6 @@ main(int argc, char **argv)
 if (initialize_steps)
 	pg_free(initialize_steps);
 initialize_steps = pg_strdup(optarg);
-checkInitSteps(initialize_steps);
 initialization_option_set = true;
 break;
 			case 'h':
diff --git a/src/bin/pgbench/t/001_pgbench_with_server.pl b/src/bin/pgbench/t/001_pgbench_with_server.pl
index 1845869016..4e3d3e464c 100644
--- a/src/bin/pgbench/t/001_pgbench_with_server.pl
+++ b/src/bin/pgbench/t/001_pgbench_with_server.pl
@@ -130,7 +130,7 @@ pgbench(
 
 # Test interaction of --init-steps with legacy step-selection options
 pgbench(
-	'--initialize --init-steps=dtpvGvv --no-vacuum --foreign-keys --unlogged-tables --partitions=3',
+	'--initialize --no-vacuum --foreign-keys --unlogged-tables --partitions=3',
 	0,
 	[qr{^$}],
 	[
@@ -143,7 +143,9 @@ pgbench(
 		qr{(?!vacuuming)}, # no vacuum
 		qr{done in \d+\.\d\d s }
 	],
-	'pgbench --init-steps');
+	'pgbench --init-steps',
+	undef,
+	'--init-steps= dtpvGvv');
 
 # Run all builtin scripts, for a few transactions each
 pgbench(

Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.

[Etsuro Fujita]

Horiguchi-san,

On Thu, Nov 7, 2019 at 5:28 PM Kyotaro Horiguchi
 wrote:
> At Thu, 7 Nov 2019 17:20:07 +0900, Etsuro Fujita  
> wrote in
> > Only two people complaining about the wording?  Considering as well
> > that we use that wording in the docs and there were no complains about
> > that IIRC, I don't feel a need to change that, TBH.

> But the most
> significant point in the previous mail is using "foreign tables using
> postgres_fdw" instead of "postgres_fdw foreign tables".

OK, but as I said above, I don't feel the need to change that.  How
about leaving it for another patch to improve the wording in that
message and/or the documentation if we really need to do it.

> And the other
> point is using different message from temporary tables.

You mean we should do s/prepare/PREPARE/?

Best regards,
Etsuro Fujita

Re: [Proposal] Global temporary tables

[Pavel Stehule]

čt 7. 11. 2019 v 10:30 odesílatel 曾文旌(义从) 
napsal:

>
>
> > 2019年11月7日 上午12:08，Konstantin Knizhnik  写道：
> >
> >
> >
> > On 06.11.2019 16:24, 曾文旌(义从) wrote:
> >> Dear Hackers
> >>
> >>
> >> I attached the patch of GTT implementationI base on PG12.
> >> The GTT design came from my first email.
> >> Some limitations in patch will be eliminated in later versions.
> >>
> >> Later, I will comment on Konstantin's patch and make some proposals for
> cooperation.
> >> Looking forward to your feedback.
> >>
> >> Thanks.
> >>
> >> Zeng Wenjing
> >>
> >
> > Thank you for this patch.
> > My first comments:
> >
> > 1.  I have ported you patch to the latest Postgres version (my patch is
> attached).
> > 2. You patch is supporting only B-Tree index for GTT. All other indexes
> (hash, gin, gist, brin,...) are not currently supported.
> Currently I only support btree index.
> I noticed that your patch supports more index types, which is where I'd
> like to work with you.
>
> > 3. I do not understand the reason for the following limitation:
> > "We allow to create index on global temp table only this session use it"
> >
> > First of all it seems to significantly reduce usage of global temp
> tables.
> > Why do we need GTT at all? Mostly because we need to access temporary
> data in more than one backend. Otherwise we can just use normal table.
> > If temp table is expected to be larger enough, so that we need to create
> index for it, then it is hard to believe that it will be needed only in one
> backend.
> >
> > May be the assumption is that all indexes has to be created before GTT
> start to be used.
> Yes, Currently, GTT's index is only supported and created in an empty
> table state, and other sessions are not using it.
> There has two improvements pointer:
> 1 Index can create on GTT(A) when the GTT(A)  in the current session is
> not empty, requiring the GTT table to be empty in the other session.
> Index_build needs to be done in the current session just like a normal
> table. This improvement is relatively easy.
>
> 2 Index can create on GTT(A)  when more than one session are using this
> GTT(A).
> Because when I'm done creating an index of the GTT in this session and
> setting it to be an valid index, it's not true for the GTT in other
> sessions.
> Indexes on gtt in other sessions require "rebuild_index" before using it.
> I don't have a better solution right now, maybe you have some suggestions.
>

I think so DDL operations can be implemented in some reduced form - so DDL
are active only for one session, and for other sessions are invisible.
Important is state of GTT object on session start.

For example ALTER TABLE DROP COLUMN can has very fatal impact on other
sessions. So I think the best of GTT can be pattern - the structure of GTT
table is immutable for any session that doesn't do DDL operations.


>
> > But right now this check is not working correctly in any case - if you
> insert some data into the table, then
> > you can not create index any more:
> >
> > postgres=# create global temp table gtt(x integer primary key, y
> integer);
> > CREATE TABLE
> > postgres=# insert into gtt values (generate_series(1,10),
> generate_series(1,10));
> > INSERT 0 10
> > postgres=# create index on gtt(y);
> > ERROR:  can not create index when have one or more backend attached this
> global temp table
> >
> > I wonder why do you need such restriction?
> >
> >
> > --
> > Konstantin Knizhnik
> > Postgres Professional: http://www.postgrespro.com
> > The Russian Postgres Company
> >
> > 
>
>

Re: [Patch proposal] libpq portal support

[Sergei Fedorov]

Hello everybody,

Yes, we will be happy to put our patch under the PostgreSQL License.

Patch is attached to this email, master was rebased to head prior to
creating the patch.

We are using a C++ wrapper on top of libpq for using database connections
in multithreaded asynchronous applications. For security reasons (and
partially because we are too lazy to escape query parameters) we use
prepared queries and parameter binding for execution. There are situations
when we need to fetch the query results not in one batch but in a `paged`
way, the most convenient way is to use the portals feature of PosgreSQL
protocol.

пт, 18 окт. 2019 г. в 15:21, Craig Ringer :

> On Thu, 17 Oct 2019 at 03:12, Sergei Fedorov 
> wrote:
>
>> Hello everybody,
>>
>> Our company was in desperate need of portals in async interface of libpq,
>> so we patched it.
>>
>> We would be happy to upstream the changes.
>>
>> The description of changes:
>>
>> Two functions in libpq-fe.h:
>> PQsendPortalBindParams for sending a command to bind a portal to a
>> previously prepared statement;
>> PQsendPortalExecute for executing a previously bound portal with a given
>> number of rows.
>>
>> A patch to pqParseInput3 in fe-protocol3.c to handle the `portal
>> suspended` message tag.
>>
>> The patch is ready for review, but it lacks documentation, tests and
>> usage examples.
>>
>> There are no functions for sending bind without params and no functions
>> for sync interface, but they can easily be added to the feature.
>>
>
> If you are happy to put it under The PostgreSQL License, then sending it
> as an attachment here is the first step.
>
> If possible, please rebase it on top of git master.
>
> Some explanation for why you have this need and what problems this solves
> for you would be helpful as well.
>
> --
>  Craig Ringer   http://www.2ndQuadrant.com/
>  2ndQuadrant - PostgreSQL Solutions for the Enterprise
>


-- 
Thank you,
Sergei Fedorov


libpq-portals.patch
Description: Binary data

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

[Amit Kapila]

On Wed, Nov 6, 2019 at 11:33 AM vignesh C  wrote:
>
> I have made one change to the configuration file in
> contrib/test_decoding directory, with that the coverage seems to be
> fine. I have seen that the coverage is almost like the code before
> applying the patch. I have attached the test change and the coverage
> report for reference. Coverage report includes the core logical work
> memory files for base code and by applying
> 0001-Add-logical_decoding_work_mem-to-limit-ReorderBuffer and
> 0002-Track-statistics-for-spilling patches.
>

Thanks,  I have incorporated your test changes and modified the two
patches.  Please see attached.

Changes:
---
1. In guc.c, we should include reorderbuffer.h, not logical.h as we
define logical_decoding_work_mem in earlier.

2.
+ *   To limit the amount of memory used by decoded changes, we track memory
+ *   used at the reorder buffer level (i.e. total amount of memory), and for
+ *   each toplevel transaction. When the total amount of used memory exceeds
+ *   the limit, the toplevel transaction consuming the most memory is then
+ *   serialized to disk.

In the above comments, removed 'toplevel' as we track memory usage for
both toplevel and subtransactions.

3. There were still a few mentions of streaming which I have removed.

4. In the docs, the type for stats spill_* was integer whereas it
should be bigint.

5.
+UpdateSpillStats(LogicalDecodingContext *ctx)
+{
+ ReorderBuffer *rb = ctx->reorder;
+
+ SpinLockAcquire(&MyWalSnd->mutex);
+
+ MyWalSnd->spillTxns = rb->spillTxns;
+ MyWalSnd->spillCount = rb->spillCount;
+ MyWalSnd->spillBytes = rb->spillBytes;
+
+ elog(WARNING, "UpdateSpillStats: updating stats %p %ld %ld %ld",
+ rb, rb->spillTxns, rb->spillCount, rb->spillBytes);

Changed the above elog to DEBUG1 as otherwise it was getting printed
very frequently.  I think we can make it DEBUG2 if we want.

6. There was an extra space in rules.out due to which test was
failing.  I have fixed it.

What do you think?

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


0001-Add-logical_decoding_work_mem-to-limit-ReorderBuffer.patch
Description: Binary data


0002-Track-statistics-for-spilling.patch
Description: Binary data

Re: [proposal] recovery_target "latest"

[Kyotaro Horiguchi]

At Thu, 7 Nov 2019 12:22:28 +0300, Grigory Smolkin  
wrote in 
> 
> On 11/7/19 8:36 AM, Kyotaro Horiguchi wrote:
> > At Thu, 7 Nov 2019 02:28:39 +0300, Grigory Smolkin
> >  wrote in
> >> On 11/6/19 1:55 PM, Grigory Smolkin wrote:
> >>> On 11/6/19 12:56 PM, Fujii Masao wrote:
>  What happens if this parameter is set to latest in the standby mode?
>  Or the combination of those settings should be prohibited?
> >>>
> >>> Currently it will behave just like regular standby, so I think, to
> >>> avoid confusion and keep things simple, the combination of them should
> >>> be prohibited.
> >>> Thank you for pointing this out, I will work on it.
> >> Attached new patch revision, now it is impossible to use
> >> recovery_target 'latest' in standby mode.
> >> TAP test is updated to reflect this behavior.
> > In the first place, latest (or anything it could be named as) is
> > defined as the explit label for the default behavior. Thus the latest
> > should work as if nothing is set to recovery_target* following the
> > definition.  That might seems somewhat strange but I think at least it
> > is harmless.
> 
> 
> Well, it was more about getting default behavior by using some
> explicit recovery_target, not the other way around. Because it will
> break some 3rd party backup and replication applications, that may
> rely on old behavior of ignoring recovery_target_action when no
> recovery_target is provided.
> But you think that it is worth pursuing, I can do that.

Ah. Sorry for the misleading statement. What I had in my mind was
somewhat the mixture of them.  I thought that recovery_target =''
behaves the same way as now, r_t_action is ignored. And 'latest' just
makes recovery_target_action work as the current non-empty
recovery_target's does. But I'm not confident that it is a good
design.

> > recovery_target=immediate + r_t_action=shutdown for a standby works as
> > commanded. Do we need to inhibit that, too?
> 
> Why something, that work as expected, should be inhibited?

To make sure, I don't think we should do that. I meant by the above
that standby mode is already accepting recovery_target_action so
inhibiting that only for 'latest' is not orthogonal and could be more
confusing for users, and complicatig the code. So my opinion is we
shouldn't inhibit 'latest' unless r_t_action harms.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: tableam vs. TOAST

[Peter Eisentraut]

On 2019-11-06 18:00, Andres Freund wrote:

I'd like an AM to have the *option* of implementing something better, or
at least go in the direction of making that possible.


I don't think the presented design prevents that.  An AM can just return 
false from relation_needs_toast_table in all cases and implement 
something internally.



It seems perfectly possible to have a helper function implementing the
current logic that you just can call with the fixed chunk size as an
additional parameter. Which'd basically mean there's no meaningful
difference in complexity compared to providing the chunk size as an
external AM property. In one case you have a callback that just calls a
helper function with one parameter, in the other you fill in a member of
the struct.


I can see a "moral" concern about having TOAST be part of the table AM 
API.  It should be an implementation concern of the AM.  How much more 
work would it be to refactor TOAST into a separate API that an AM 
implementation could use or not?  How much more complicated would the 
result be?  I guess you would like to at least have it explored.


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

Re: pgbench - extend initialization phase control

[Fujii Masao]

On Thu, Nov 7, 2019 at 6:35 PM Fabien COELHO  wrote:
>
>
> Hello Masao-san,
>
> >> I do not think that this is desirable. It would be a regression, and
> >> allowing a no-op is not an issue in anyway.
> >
> > Why is that regression, you think?
>
> Because "pgbench -I ' d'" currently works and it would cease to work after
> the patch.

If the behavior has been documented and visible to users,
I agree that it should not be dropped for compatibility basically.
But in this case, that was not.

> > I think that's an oversight. If I'm missing something and accepting a
> > blank character as no-op in also checkInitSteps() is really necessary
> > for some reasons, which should be documented. But, if so, another
> > question is; why should only blank character be treated as no-op, in
> > checkInitSteps()?
>
> The idea is to have one character that can be substituted to remove any
> operation.

Probably I understand that idea is necessary in the internal of pgbench
because pgbench internally may modify the initialization steps string.
But I'm not sure why it needs to be exposed, yet.

> On principle, allowing a no-op character, whatever the choice, is a good
> idea, because it means that the caller can take advantage of that if need
> be.
>
> I think that the actual oversight is that the checkInitSteps should be
> called at the beginning of processing initialization steps rather than
> while processing -I, because currently other places modify the
> initialization string (no-vacuum, foreign key) and thus are not checked.

As far as I read the code, runInitSteps() does the check. If the initialization
steps string contains unrecognized character, runInitSteps() emits an error.

* (We could just leave it to runInitSteps() to fail if there are wrong
* characters, but since initialization can take awhile, it seems friendlier
* to check during option parsing.)

The above comment in checkInitSteps() seems to explain why
checkInitSteps() is called at the beginning of processing initialization
steps.

Regards,

-- 
Fujii Masao

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

[Dilip Kumar]

On Thu, Nov 7, 2019 at 3:19 PM Amit Kapila  wrote:
>
> On Wed, Nov 6, 2019 at 11:33 AM vignesh C  wrote:
> >
> > I have made one change to the configuration file in
> > contrib/test_decoding directory, with that the coverage seems to be
> > fine. I have seen that the coverage is almost like the code before
> > applying the patch. I have attached the test change and the coverage
> > report for reference. Coverage report includes the core logical work
> > memory files for base code and by applying
> > 0001-Add-logical_decoding_work_mem-to-limit-ReorderBuffer and
> > 0002-Track-statistics-for-spilling patches.
> >
>
> Thanks,  I have incorporated your test changes and modified the two
> patches.  Please see attached.
>
> Changes:
> ---
> 1. In guc.c, we should include reorderbuffer.h, not logical.h as we
> define logical_decoding_work_mem in earlier.
Yeah Right.
>
> 2.
> + *   To limit the amount of memory used by decoded changes, we track memory
> + *   used at the reorder buffer level (i.e. total amount of memory), and for
> + *   each toplevel transaction. When the total amount of used memory exceeds
> + *   the limit, the toplevel transaction consuming the most memory is then
> + *   serialized to disk.
>
> In the above comments, removed 'toplevel' as we track memory usage for
> both toplevel and subtransactions.
Correct.
>
> 3. There were still a few mentions of streaming which I have removed.
>
ok
> 4. In the docs, the type for stats spill_* was integer whereas it
> should be bigint.
ok
>
> 5.
> +UpdateSpillStats(LogicalDecodingContext *ctx)
> +{
> + ReorderBuffer *rb = ctx->reorder;
> +
> + SpinLockAcquire(&MyWalSnd->mutex);
> +
> + MyWalSnd->spillTxns = rb->spillTxns;
> + MyWalSnd->spillCount = rb->spillCount;
> + MyWalSnd->spillBytes = rb->spillBytes;
> +
> + elog(WARNING, "UpdateSpillStats: updating stats %p %ld %ld %ld",
> + rb, rb->spillTxns, rb->spillCount, rb->spillBytes);
>
> Changed the above elog to DEBUG1 as otherwise it was getting printed
> very frequently.  I think we can make it DEBUG2 if we want.
Yeah, it should not be WARNING.
>
> 6. There was an extra space in rules.out due to which test was
> failing.  I have fixed it.
My Bad.  I have induced while separating out the changes for the spilling.

> What do you think?
I have reviewed your changes and looks fine to me.

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

Re: Parallel leader process info in EXPLAIN

[Rafia Sabih]

On Mon, 4 Nov 2019 at 00:30, Thomas Munro  wrote:

> On Mon, Nov 4, 2019 at 12:11 PM Thomas Munro 
> wrote:
> > I guess I thought of that as a debugging feature and took it out
> > because it was too verbose, but maybe it just needs to be controlled
> > by the VERBOSE switch.  Do you think we should put that back?
>
> By which I mean: would you like to send a patch?  :-)
>
> Here is a new version of the "Leader:" patch, because cfbot told me
> that gcc didn't like it as much as clang.
>

I was reviewing this patch and here are a few comments,

+static void
+ExplainNodePerProcess(ExplainState *es, bool *opened_group,
+  int worker_number, Instrumentation *instrument)
+{

A small description about this routine would be helpful and will give the
file a consistent look.

Also, I noticed that the worker details are displayed for sort node even
without verbose, but for scans it is only with verbose. Am I missing
something or there is something behind? However, I am not sure if this is
the introduced by this patch-set.

-- 
Regards,
Rafia Sabih

Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.

[Etsuro Fujita]

Horiguchi-san,

On Thu, Nov 7, 2019 at 5:31 PM Kyotaro Horiguchi
 wrote:
> I forgot to mention that the comment in XACT_EVENT_PRE_PREPARE
> contains the same mistake and needs more or less the same fix.

Good catch!  How about rewriting "We disallow remote transactions that
modified anything" in the comment simply to "We disallow any remote
transactions" or something like that?  Attached is an updated patch.
In the patch, I did s/prepare/PREPARE/ to the error message as well,
as you proposed.

Thanks again for reviewing!

Best regards,
Etsuro Fujita


fix_postgres_fdw_prepared_transaction-v4.diff
Description: Binary data

Re: Using multiple extended statistics for estimates

[Tomas Vondra]

On Thu, Nov 07, 2019 at 01:38:20PM +0900, Kyotaro Horiguchi wrote:

Hello.

At Wed, 6 Nov 2019 20:58:49 +0100, Tomas Vondra  
wrote in

>Here is a slightly polished v2 of the patch, the main difference being
>that computing clause_attnums was moved to a separate function.
>

This time with the attachment ;-)


This patch is a kind of straight-forward, which repeats what the
previous statext_mcv_clauselist_selectivity did as long as remaining
clauses matches any of MV-MCVs. Almost no regression in the cases
where zero or just one MV-MCV applies to the given clause list.

It applies cleanly on the current master and seems working as
expected.


I have some comments.

Could we have description in the documentation on what multiple
MV-MCVs are used in a query? And don't we need some regression tests?



Yes, regression tests are certainly needed - I though I've added them,
but it seems I failed to include them in the patch. Will fix.

I agree it's probably worth mentioning we can consider multiple stats,
but I'm a bit hesitant to put the exact rules how we pick the "best"
statistic to the docs. It's not 100% deterministic and it's likely
we'll need to tweak it a bit in the future.

I'd prefer showing the stats in EXPLAIN, but that's a separate patch.



+/*
+ * statext_mcv_clause_attnums
+ * Recalculate attnums from compatible but not-yet-estimated 
clauses.

It returns attnums collected from multiple clause*s*. Is the name OK
with "clause_attnums"?

The comment says as if it checks the compatibility of each clause but
the work is done in the caller side. I'm not sure such strictness is
required, but it might be better that the comment represents what
exactly the function does.



But the incompatible clauses have the pre-computed attnums set to NULL,
so technically the comment is correct. But I'll clarify.



+ */
+static Bitmapset *
+statext_mcv_clause_attnums(int nclauses, Bitmapset **estimatedclauses,
+  Bitmapset **list_attnums)

The last two parameters are in the same type in notation but in
different actual types.. that is, one is a pointer to Bitmapset*, and
another is an array of Bitmaptset*. The code in the function itself
suggests that, but it would be helpful if a brief explanation of the
parameters is seen in the function comment.



OK, will explain in a comment.


+   /*
+* Recompute attnums in the remaining clauses (we simply use 
the bitmaps
+* computed earlier, so that we don't have to inspect the 
clauses again).
+*/
+   clauses_attnums = 
statext_mcv_clause_attnums(list_length(clauses),

Couldn't we avoid calling this function twice with the same parameters
at the first round in the loop?



Hmmm, yeah. That's a good point.


+   foreach(l, clauses)
{
-   stat_clauses = lappend(stat_clauses, (Node *) 
lfirst(l));
-   *estimatedclauses = bms_add_member(*estimatedclauses, 
listidx);
+   /*
+* If the clause is compatible with the selected 
statistics, mark it
+* as estimated and add it to the list to estimate.
+*/
+   if (list_attnums[listidx] != NULL &&
+   bms_is_subset(list_attnums[listidx], 
stat->keys))
+   {
+   stat_clauses = lappend(stat_clauses, (Node *) 
lfirst(l));
+   *estimatedclauses = 
bms_add_member(*estimatedclauses, listidx);
+   }

The loop runs through all clauses every time. I agree that that is
better than using a copy of the clauses to avoid to step on already
estimated clauses, but maybe we need an Assertion that the listidx is
not a part of estimatedclauses to make sure no clauses are not
estimated twice.



Well, we can't really operate on a smaller "copy" of the list anyway,
because that would break the precalculation logic (the listidx value
would be incorrect for the new list), and tweaking it would be more
expensive than just iterating over all clauses. The assumption is that
we won't see extremely large number of clauses here.

Adding an assert seems reasonable. And maybe a comment why we should not
see any already-estimated clauses here.

regards

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

Re: Add Change Badges to documentation

[Fabien COELHO]

Hello Corey,


Attached is a patch to implement change badges in our documentation.


More precisely, it is a POC to show that the infra works. It adds 3 badges 
on various entries.


Patch applies cleanly, compiles, and indeed (too) green boxes show up. 
Good.


Maybe it would be better with badges at the end of lines, otherwise it 
interferes with the proper alignment of text. Also, ISTM that the shorter 
the badge contents the better, so "v13" is better than "new in version 
13".


Maybe it would be nice to have a on/off CSS/JS controled feature, so that 
they can be hidden easily?


I'm wondering about the maintainability of the feature if badges need to 
be updated, but if this is only "v13" to say that a feature appears in 
v13, probably it is okay, there is no need to update.


However, if a feature is changed, should we start accumulating badges?

Updating the documentation would be a great pain. Maybe it could be partly 
automated.




1. It shows a casual user what pieces are new on that page (new functions,
new keywords, new command options, etc).


Ok.


2. It also works in the negative: a user can quickly skim a page, and
lacking any badges, feel confident that everything there works in the way
that it did in version N-1.


Possibly. If the maintainer thought about it.


3. It also acts as a subtle cue for the user to click on the previous
version to see what it used to look like, confident that there *will* be a
difference on the previous version.


Which suggests links to do that?


1. All new documentation pages would get a "NEW" badge in their title.


Hmmm, I do not think that we want to add and remove NEW badges on every 
version, that would be too troublesome. ISTM that maybe we can add "v13" 
and have some JS/CSS which says that it is new when looking at v13.



2. New function definitions, new command options, etc would get a "NEW"
badge as visually close to the change as is practical.

3. Changes to existing functions, options, etc. would get a badge of
"UPDATED"


Idem, maintainability? Unless this is automated.


4. At major release time, we could do one of two things:

4a. We could keep the NEW/UPDATED badges in the fixed release version, and
then completely remove them from the master, because for version N+1, they
won't be new anymore. This can be accomplished with an XSL transform
looking for any tag with the "revision" attribute


Hmmm.


4b. We could code in the version number at release time, and leave it in
place. So in version 14 you could find both "v13" and "v14" badges, and in
version 15 you could find badges for 15, 14, and 13. At some point (say
v17), we start retiring the v13 badges, and in v18 we'd retire the v14
badges, and so on, to keep the clutter to a minimum.


Hmmm.


Back to the patch:
I implemented this only for html output, and the colors I chose are very
off-brand for postgres, so that will have to change. There's probably some
spacing/padding issues I haven't thought of. Please try it out, make some
modifications to existing document pages to see how badges would work in
those contexts.



--
Fabien Coelho - CRI, MINES ParisTech

Re: Reorderbuffer crash during recovery

[Tomas Vondra]

On Thu, Nov 07, 2019 at 11:01:17AM +0530, Dilip Kumar wrote:

On Thu, Nov 7, 2019 at 9:55 AM vignesh C  wrote:


On Wed, Nov 6, 2019 at 5:41 PM Dilip Kumar  wrote:
>
> On Wed, Nov 6, 2019 at 5:20 PM vignesh C  wrote:
> >
> > Hi,
> >
> > ...
> >
> > Issue1 it seems like if all the reorderbuffer has been flushed and
> > then the server restarts. This problem occurs.
> > Issue 2 it seems like if there are many subtransactions present and
> > then the server restarts. This problem occurs. The subtransaction's
> > final_lsn is not being set and when ReorderBufferRestoreCleanup is
> > called the assert fails. May be for this we might have to set the
> > subtransaction's final_lsn before cleanup(not sure).
> >
> > I could not reproduce this issue consistently with a test case, But I
> > felt this looks like a problem from review.
> >
> > For issue1, I could reproduce by the following steps:
> > 1) Change ReorderBufferCheckSerializeTXN so that it gets flushed always.
> > 2) Have many open transactions with subtransactions open.
> > 3) Attach one of the transaction from gdb and call abort().
>
> Do you need subtransactions for the issue1? It appears that after the
> restart if the changes list is empty it will hit the assert.  Am I
> missing something?
>

When I had reported this issue I could reproduce this issue with
sub-transactions. Now I have tried without using sub-transactions and
could still reproduce this issue. You are right Issue 1 will appear in
both the cases with and without subtransactions.


Okay, thanks for the confirmation.



I'm a bit confused - does this happen only with the logical_work_mem
patches, or with clean master too? If only with the patches, which
version exactly?

regards

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

Re: pgbench - extend initialization phase control

[Fabien COELHO]

Hello,


I think that the actual oversight is that the checkInitSteps should be
called at the beginning of processing initialization steps rather than
while processing -I, because currently other places modify the
initialization string (no-vacuum, foreign key) and thus are not checked.


As far as I read the code, runInitSteps() does the check. If the initialization
steps string contains unrecognized character, runInitSteps() emits an error.


Sure, but the previous step have been executed and committed, the point of 
the check is to detect the issue before starting the execution.



   * (We could just leave it to runInitSteps() to fail if there are wrong
   * characters, but since initialization can take awhile, it seems friendlier
   * to check during option parsing.)

The above comment in checkInitSteps() seems to explain why 
checkInitSteps() is called at the beginning of processing initialization 
steps.


Yep, the comment is right in the motivation, but not accurate anymore wrt 
the submitted patch. V2 attached updates this comment.


--
Fabien.

Re: Reorderbuffer crash during recovery

[Amit Kapila]

On Thu, Nov 7, 2019 at 4:48 PM Tomas Vondra
 wrote:
>
> I'm a bit confused - does this happen only with the logical_work_mem
> patches, or with clean master too?
>

This occurs with the clean master.  This is a base code problem
revealed while doing stress testing of logical_work_mem patches.


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

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

[Amit Kapila]

On Thu, Nov 7, 2019 at 3:50 PM Dilip Kumar  wrote:
>
> On Thu, Nov 7, 2019 at 3:19 PM Amit Kapila  wrote:
>
> > What do you think?
> I have reviewed your changes and looks fine to me.
>

Okay, thanks.  I am also happy with the two patches I have posted in
my last email [1].

Tomas, would you like to take a look at those patches and commit them
if you are happy or would you like me to do the same?

Some notes before commit:
--
1.
Commit message need to be changed for the first patch
-
A.
> The memory limit is defined by a new logical_decoding_work_mem GUC, so for 
> example we can do this

SET logical_decoding_work_mem = '128kB'

> to trigger very aggressive streaming. The minimum value is 64kB.

I think this patch doesn't contain streaming, so we either need to
reword it or remove it.

B.
> The logical_decoding_work_mem may be set either in postgresql.conf, in which 
> case it serves as the default for all publishers on that instance, or when 
> creating the
> subscription, using a work_mem paramemter in the WITH clause (specifies 
> number of kilobytes).

We need to reword this as we have decided to remove the setting from
the subscription side as of now.

2. I think we can change the message level in UpdateSpillStats() to DEBUG2.

3. I think we need catversion bump for the second patch.

4. I think we can combine both patches and commit as one patch, but it
is okay to commit them separately as well.


[1] - 
https://www.postgresql.org/message-id/CAA4eK1Kdmi6VVguKEHV6Ho2isCPVFdQtt0WLsK10fiuE59_0Yw%40mail.gmail.com

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

Re: Why overhead of SPI is so large?

[Kyotaro Horiguchi]

Hello.

At Tue, 5 Nov 2019 22:14:40 +0100, Pavel Stehule  
wrote in 
> Hi
> 
> pá 23. 8. 2019 v 16:32 odesílatel Konstantin Knizhnik <
> k.knizh...@postgrespro.ru> napsal:
> 
> >
> >
> > On 23.08.2019 14:42, Pavel Stehule wrote:
> >
> >
> > In reality it is not IMMUTABLE function. On second hand, there are lot of
> > application that depends on this behave.
> >
> > It is well know trick how to reduce estimation errors related to JOINs.
> > When immutable function has constant parameters, then it is evaluated in
> > planning time.
> >
> > So sometimes was necessary to use
> >
> > SELECT ... FROM tab WHERE foreign_key = immutable_function('constant
> > parameter')
> >
> > instead JOIN.
> >
> > It is ugly, but it is working perfectly. I think so until we will have
> > multi table statistics, this behave should be available in Postgres.
> >
> > Sure, this function should not be used for functional indexes.
> >
> >
> >
> > What about the following version of the patch?
> >
> 
> I am sending review of this small patch.
> 
> This small patch reduce a overhead of usage buildin immutable functions in
> volatile functions with simple trick. Starts snapshot only when it is
> necessary.
> 
> In decrease runtime time about 25 % on this small example.
> 
> do $$
> declare i int;
> begin
>   i := 0;
>   while i < 1000
>   loop
> i := i + 1;
>   end loop;
> end;
> $$;
> 
> If there are more expressions, then speedup can be more interesting. If
> there are other bottlenecks, then the speedup will be less. 25% is not bad,
> so we want to this feature.
> 
> I believe so similar method can be used more aggressively with more
> significant performance benefit, but this is low hanging fruit and isn't
> reason to wait for future.
> 
> This patch doesn't introduce any new feature, so new tests and new doc is
> not necessary.
> The patch is readable, well  formatted, only comments are too long. I fixed
> it.
> All tests passed
> I fixed few warnings, and I reformated little bit function
> expr_needs_snapshot to use if instead case, what is more usual in these
> cases.
> 
> I think so this code can be marked as ready for commit

I have some comments on this.

expr_needs_snapshot checks out some of the node already checked out in
exec_simple_check_plan but not all. However I don't see the criteria
of the choice.

I might be too worrying, but maybe we should write the function in
white-listed way, that is, expr_needs_snapshot returns false only if
the whole tree consists of only the node types known to the
function. And any unknown node makes the function return true
immediately.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: Why overhead of SPI is so large?

[Pavel Stehule]

čt 7. 11. 2019 v 13:03 odesílatel Kyotaro Horiguchi 
napsal:

> Hello.
>
> At Tue, 5 Nov 2019 22:14:40 +0100, Pavel Stehule 
> wrote in
> > Hi
> >
> > pá 23. 8. 2019 v 16:32 odesílatel Konstantin Knizhnik <
> > k.knizh...@postgrespro.ru> napsal:
> >
> > >
> > >
> > > On 23.08.2019 14:42, Pavel Stehule wrote:
> > >
> > >
> > > In reality it is not IMMUTABLE function. On second hand, there are lot
> of
> > > application that depends on this behave.
> > >
> > > It is well know trick how to reduce estimation errors related to JOINs.
> > > When immutable function has constant parameters, then it is evaluated
> in
> > > planning time.
> > >
> > > So sometimes was necessary to use
> > >
> > > SELECT ... FROM tab WHERE foreign_key = immutable_function('constant
> > > parameter')
> > >
> > > instead JOIN.
> > >
> > > It is ugly, but it is working perfectly. I think so until we will have
> > > multi table statistics, this behave should be available in Postgres.
> > >
> > > Sure, this function should not be used for functional indexes.
> > >
> > >
> > >
> > > What about the following version of the patch?
> > >
> >
> > I am sending review of this small patch.
> >
> > This small patch reduce a overhead of usage buildin immutable functions
> in
> > volatile functions with simple trick. Starts snapshot only when it is
> > necessary.
> >
> > In decrease runtime time about 25 % on this small example.
> >
> > do $$
> > declare i int;
> > begin
> >   i := 0;
> >   while i < 1000
> >   loop
> > i := i + 1;
> >   end loop;
> > end;
> > $$;
> >
> > If there are more expressions, then speedup can be more interesting. If
> > there are other bottlenecks, then the speedup will be less. 25% is not
> bad,
> > so we want to this feature.
> >
> > I believe so similar method can be used more aggressively with more
> > significant performance benefit, but this is low hanging fruit and isn't
> > reason to wait for future.
> >
> > This patch doesn't introduce any new feature, so new tests and new doc is
> > not necessary.
> > The patch is readable, well  formatted, only comments are too long. I
> fixed
> > it.
> > All tests passed
> > I fixed few warnings, and I reformated little bit function
> > expr_needs_snapshot to use if instead case, what is more usual in these
> > cases.
> >
> > I think so this code can be marked as ready for commit
>
> I have some comments on this.
>
> expr_needs_snapshot checks out some of the node already checked out in
> exec_simple_check_plan but not all. However I don't see the criteria
> of the choice.
>
> I might be too worrying, but maybe we should write the function in
> white-listed way, that is, expr_needs_snapshot returns false only if
> the whole tree consists of only the node types known to the
> function. And any unknown node makes the function return true
> immediately.
>

has sense

Pavel


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

Re: [Proposal] Global temporary tables

[曾文旌(义从)]

> 2019年11月7日 下午5:40，Pavel Stehule  写道：
> 
> 
> 
> čt 7. 11. 2019 v 10:30 odesílatel 曾文旌(义从)  > napsal:
> 
> 
> > 2019年11月7日 上午12:08，Konstantin Knizhnik  > > 写道：
> > 
> > 
> > 
> > On 06.11.2019 16:24, 曾文旌(义从) wrote:
> >> Dear Hackers
> >> 
> >> 
> >> I attached the patch of GTT implementationI base on PG12.
> >> The GTT design came from my first email.
> >> Some limitations in patch will be eliminated in later versions.
> >> 
> >> Later, I will comment on Konstantin's patch and make some proposals for 
> >> cooperation.
> >> Looking forward to your feedback.
> >> 
> >> Thanks.
> >> 
> >> Zeng Wenjing
> >> 
> > 
> > Thank you for this patch.
> > My first comments:
> > 
> > 1.  I have ported you patch to the latest Postgres version (my patch is 
> > attached).
> > 2. You patch is supporting only B-Tree index for GTT. All other indexes 
> > (hash, gin, gist, brin,...) are not currently supported.
> Currently I only support btree index.
> I noticed that your patch supports more index types, which is where I'd like 
> to work with you.
> 
> > 3. I do not understand the reason for the following limitation:
> > "We allow to create index on global temp table only this session use it"
> > 
> > First of all it seems to significantly reduce usage of global temp tables.
> > Why do we need GTT at all? Mostly because we need to access temporary data 
> > in more than one backend. Otherwise we can just use normal table.
> > If temp table is expected to be larger enough, so that we need to create 
> > index for it, then it is hard to believe that it will be needed only in one 
> > backend.
> > 
> > May be the assumption is that all indexes has to be created before GTT 
> > start to be used.
> Yes, Currently, GTT's index is only supported and created in an empty table 
> state, and other sessions are not using it.
> There has two improvements pointer:
> 1 Index can create on GTT(A) when the GTT(A)  in the current session is not 
> empty, requiring the GTT table to be empty in the other session.
> Index_build needs to be done in the current session just like a normal table. 
> This improvement is relatively easy.
> 
> 2 Index can create on GTT(A)  when more than one session are using this 
> GTT(A).
> Because when I'm done creating an index of the GTT in this session and 
> setting it to be an valid index, it's not true for the GTT in other sessions.
> Indexes on gtt in other sessions require "rebuild_index" before using it. 
> I don't have a better solution right now, maybe you have some suggestions.
> 
> I think so DDL operations can be implemented in some reduced form - so DDL 
> are active only for one session, and for other sessions are invisible. 
> Important is state of GTT object on session start. 
> 
> For example ALTER TABLE DROP COLUMN can has very fatal impact on other 
> sessions. So I think the best of GTT can be pattern - the structure of GTT 
> table is immutable for any session that doesn't do DDL operations.
Yes, Those ddl that need to rewrite data files will have this problem.
This is why I disabled alter GTT in the current version.
It can be improved, such as Alter GTT can also be allowed when only the current 
session is in use.
Users can also choose to kick off other sessions that are using gtt, then do 
alter GTT.
I provide a function(pg_gtt_attached_pid(relation, schema)) to query which 
session a GTT is being used by.

> 
> 
> 
> > But right now this check is not working correctly in any case - if you 
> > insert some data into the table, then
> > you can not create index any more:
> > 
> > postgres=# create global temp table gtt(x integer primary key, y integer);
> > CREATE TABLE
> > postgres=# insert into gtt values (generate_series(1,10), 
> > generate_series(1,10));
> > INSERT 0 10
> > postgres=# create index on gtt(y);
> > ERROR:  can not create index when have one or more backend attached this 
> > global temp table
> > 
> > I wonder why do you need such restriction?
> > 
> > 
> > -- 
> > Konstantin Knizhnik
> > Postgres Professional: http://www.postgrespro.com 
> > 
> > The Russian Postgres Company
> > 
> > 
> 

Re: [Proposal] Global temporary tables

[Pavel Stehule]

čt 7. 11. 2019 v 13:17 odesílatel 曾文旌(义从) 
napsal:

>
>
> 2019年11月7日 下午5:40，Pavel Stehule  写道：
>
>
>
> čt 7. 11. 2019 v 10:30 odesílatel 曾文旌(义从) 
> napsal:
>
>>
>>
>> > 2019年11月7日 上午12:08，Konstantin Knizhnik  写道：
>> >
>> >
>> >
>> > On 06.11.2019 16:24, 曾文旌(义从) wrote:
>> >> Dear Hackers
>> >>
>> >>
>> >> I attached the patch of GTT implementationI base on PG12.
>> >> The GTT design came from my first email.
>> >> Some limitations in patch will be eliminated in later versions.
>> >>
>> >> Later, I will comment on Konstantin's patch and make some proposals
>> for cooperation.
>> >> Looking forward to your feedback.
>> >>
>> >> Thanks.
>> >>
>> >> Zeng Wenjing
>> >>
>> >
>> > Thank you for this patch.
>> > My first comments:
>> >
>> > 1.  I have ported you patch to the latest Postgres version (my patch is
>> attached).
>> > 2. You patch is supporting only B-Tree index for GTT. All other indexes
>> (hash, gin, gist, brin,...) are not currently supported.
>> Currently I only support btree index.
>> I noticed that your patch supports more index types, which is where I'd
>> like to work with you.
>>
>> > 3. I do not understand the reason for the following limitation:
>> > "We allow to create index on global temp table only this session use it"
>> >
>> > First of all it seems to significantly reduce usage of global temp
>> tables.
>> > Why do we need GTT at all? Mostly because we need to access temporary
>> data in more than one backend. Otherwise we can just use normal table.
>> > If temp table is expected to be larger enough, so that we need to
>> create index for it, then it is hard to believe that it will be needed only
>> in one backend.
>> >
>> > May be the assumption is that all indexes has to be created before GTT
>> start to be used.
>> Yes, Currently, GTT's index is only supported and created in an empty
>> table state, and other sessions are not using it.
>> There has two improvements pointer:
>> 1 Index can create on GTT(A) when the GTT(A)  in the current session is
>> not empty, requiring the GTT table to be empty in the other session.
>> Index_build needs to be done in the current session just like a normal
>> table. This improvement is relatively easy.
>>
>> 2 Index can create on GTT(A)  when more than one session are using this
>> GTT(A).
>> Because when I'm done creating an index of the GTT in this session and
>> setting it to be an valid index, it's not true for the GTT in other
>> sessions.
>> Indexes on gtt in other sessions require "rebuild_index" before using it.
>> I don't have a better solution right now, maybe you have some suggestions.
>>
>
> I think so DDL operations can be implemented in some reduced form - so DDL
> are active only for one session, and for other sessions are invisible.
> Important is state of GTT object on session start.
>
> For example ALTER TABLE DROP COLUMN can has very fatal impact on other
> sessions. So I think the best of GTT can be pattern - the structure of GTT
> table is immutable for any session that doesn't do DDL operations.
>
> Yes, Those ddl that need to rewrite data files will have this problem.
> This is why I disabled alter GTT in the current version.
> It can be improved, such as Alter GTT can also be allowed when only the
> current session is in use.
>

I think so it is acceptable solution for some first steps, but I cannot to
imagine so this behave can be good for production usage. But can be good
enough for some time.

Regards

Pavel

Users can also choose to kick off other sessions that are using gtt, then
> do alter GTT.
> I provide a function(pg_gtt_attached_pid(relation, schema)) to query which
> session a GTT is being used by.
>
>
>
>>
>> > But right now this check is not working correctly in any case - if you
>> insert some data into the table, then
>> > you can not create index any more:
>> >
>> > postgres=# create global temp table gtt(x integer primary key, y
>> integer);
>> > CREATE TABLE
>> > postgres=# insert into gtt values (generate_series(1,10),
>> generate_series(1,10));
>> > INSERT 0 10
>> > postgres=# create index on gtt(y);
>> > ERROR:  can not create index when have one or more backend attached
>> this global temp table
>> >
>> > I wonder why do you need such restriction?
>> >
>> >
>> > --
>> > Konstantin Knizhnik
>> > Postgres Professional: http://www.postgrespro.com
>> > The Russian Postgres Company
>> >
>> > 
>>
>>
>

Re: Remove HAVE_LONG_LONG_INT

[Peter Eisentraut]

On 2019-10-30 14:49, Peter Eisentraut wrote:

HAVE_LONG_LONG_INT is now implied by the requirement for C99, so the
separate Autoconf check can be removed.  The uses are almost all in ecpg
code, and AFAICT the check was originally added specifically for ecpg.


committed

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

Re: [proposal] recovery_target "latest"

[Grigory Smolkin]

On 11/7/19 12:56 PM, Kyotaro Horiguchi wrote:

At Thu, 7 Nov 2019 12:22:28 +0300, Grigory Smolkin  
wrote in

On 11/7/19 8:36 AM, Kyotaro Horiguchi wrote:

At Thu, 7 Nov 2019 02:28:39 +0300, Grigory Smolkin
 wrote in

On 11/6/19 1:55 PM, Grigory Smolkin wrote:

On 11/6/19 12:56 PM, Fujii Masao wrote:

What happens if this parameter is set to latest in the standby mode?
Or the combination of those settings should be prohibited?

Currently it will behave just like regular standby, so I think, to
avoid confusion and keep things simple, the combination of them should
be prohibited.
Thank you for pointing this out, I will work on it.

Attached new patch revision, now it is impossible to use
recovery_target 'latest' in standby mode.
TAP test is updated to reflect this behavior.

In the first place, latest (or anything it could be named as) is
defined as the explit label for the default behavior. Thus the latest
should work as if nothing is set to recovery_target* following the
definition.  That might seems somewhat strange but I think at least it
is harmless.


Well, it was more about getting default behavior by using some
explicit recovery_target, not the other way around. Because it will
break some 3rd party backup and replication applications, that may
rely on old behavior of ignoring recovery_target_action when no
recovery_target is provided.
But you think that it is worth pursuing, I can do that.

Ah. Sorry for the misleading statement. What I had in my mind was
somewhat the mixture of them.  I thought that recovery_target =''
behaves the same way as now, r_t_action is ignored. And 'latest' just
makes recovery_target_action work as the current non-empty
recovery_target's does. But I'm not confident that it is a good
design.


recovery_target=immediate + r_t_action=shutdown for a standby works as
commanded. Do we need to inhibit that, too?

Why something, that work as expected, should be inhibited?

To make sure, I don't think we should do that. I meant by the above
that standby mode is already accepting recovery_target_action so
inhibiting that only for 'latest' is not orthogonal and could be more
confusing for users, and complicatig the code. So my opinion is we
shouldn't inhibit 'latest' unless r_t_action harms.


I gave it some thought and now think that prohibiting recovery_target 
'latest' and standby was a bad idea.
All recovery_targets follow the same pattern of usage, so 
recovery_target "latest" also must be capable of working in standby mode.
All recovery_targets have a clear deterministic 'target' where recovery 
should end.
In case of recovery_target "latest" this target is the end of available 
WAL, therefore the end of available WAL must be more clearly defined.

I will work on it.

Thank you for a feedback.




regards.


--
Grigory Smolkin
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: TAP tests aren't using the magic words for Windows file access

[Andrew Dunstan]

On 11/7/19 3:42 AM, Juan José Santamaría Flecha wrote:
>
> On Thu, Nov 7, 2019 at 1:57 AM Andrew Dunstan
>  > wrote:
>
>
> In any case, the patch will fail as written - on the Msys 1 system I
> just tested Win32::API is not available to the DTK perl we need to use
> to run TAP tests.
>
>
> May I ask which version of Msys is that system using? In a recent
> installation (post 1.0.11) I see that those modules are available.
>
>

Not sure how I discover that. The path is c:\mingw\msys\1.0, looks like
it was installed in 2013 some time. perl reports version 5.8.8 built for
msys-int64

This is the machine that runs jacana on the buildfarm.

The test I'm running is:

    perl -MWin32::API -e ';'

And perl reports it can't find the module.

However, the perl on my pretty recent Msys2 system (the one that runs
fairywren) reports the same problem. That's 5.30.0 built for
x86_64-msys-thread-multi.

So my question is which perl you're testing with? If it's a Windows
native perl version such as ActivePerl or StrawberryPerl that won't do -
the buildfarm needs to use msys-perl to run prove.


cheers


andrew


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

Re: TAP tests aren't using the magic words for Windows file access

[Alvaro Herrera]

On 2019-Nov-07, Andrew Dunstan wrote:

> The test I'm running is:
> 
>     perl -MWin32::API -e ';'
> 
> And perl reports it can't find the module.

That's a curious test to try, given that the module is called
Win32API::File.

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

Re: TAP tests aren't using the magic words for Windows file access

[Andrew Dunstan]

On 11/7/19 8:53 AM, Alvaro Herrera wrote:
> On 2019-Nov-07, Andrew Dunstan wrote:
>
>> The test I'm running is:
>>
>>     perl -MWin32::API -e ';'
>>
>> And perl reports it can't find the module.
> That's a curious test to try, given that the module is called
> Win32API::File.
>


The patch says:


+        require Win32::API;
+        Win32::API->import;


cheers


andrew

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

Re: tableam vs. TOAST

[Robert Haas]

On Thu, Nov 7, 2019 at 1:15 AM Ashutosh Sharma  wrote:
> @Robert, Myself and Prabhat have tried running the test-cases that
> caused the checkpointer process to crash earlier multiple times but we
> are not able to reproduce it both with and without the patch. However,
> from the stack trace shared earlier by Prabhat, it is clear that the
> checkpointer process panicked due to fsync failure. But, there is no
> further data to know the exact reason for the fsync failure. From the
> code of checkpointer process (basically the function to process fsync
> requests) it is understood that, the checkpointer process can PANIC
> due to one of the following two reasons.

Oh, I didn't realize this was a panic due to an fsync() failure when I
looked at the stack trace before.  I think it's concerning that
fsync() failed on Prabhat's machine, and it would be interesting to
know why that happened, but I don't see how this patch could possibly
*cause* fsync() to fail, so I think we can say that whatever is
happening on his machine is unrelated to this patch -- and probably
also unrelated to PostgreSQL.

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

Re: TAP tests aren't using the magic words for Windows file access

[Alvaro Herrera]

On 2019-Nov-07, Andrew Dunstan wrote:

> On 11/7/19 8:53 AM, Alvaro Herrera wrote:

> > That's a curious test to try, given that the module is called
> > Win32API::File.
> 
> The patch says:
> 
> +        require Win32::API;
> +        Win32::API->import;

Oh, you're right, it does.  I wonder why, though:

$ corelist -a Win32::API

Data for 2018-11-29
Win32::API was not in CORE (or so I think)

$ corelist -a Win32API::File

Data for 2018-11-29
Win32API::File was first released with perl v5.8.9
  v5.8.9 0.1001_01 
  v5.9.4 0.1001
  v5.9.5 0.1001_01 
  v5.10.00.1001_01 
 ...


According to the Win32API::File manual, you can request a file to be
shared by passing the string "r" to svShare to method createFile().
So do we really need all those extremely ugly "droppings" Juanjo added
to the patch?

(BTW the Win32API::File manual also says this:
"The default for $svShare is "rw" which provides the same sharing as
using regular perl open()."
I wonder why "the regular perl open()" is not doing the sharing thing
correctly ... has the problem has been misdiagnosed?).

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

Re: TAP tests aren't using the magic words for Windows file access

[Andrew Dunstan]

On 11/7/19 9:12 AM, Alvaro Herrera wrote:
> On 2019-Nov-07, Andrew Dunstan wrote:
>
>> On 11/7/19 8:53 AM, Alvaro Herrera wrote:
>>> That's a curious test to try, given that the module is called
>>> Win32API::File.
>> The patch says:
>>
>> +        require Win32::API;
>> +        Win32::API->import;
> Oh, you're right, it does.  I wonder why, though:
>
> $ corelist -a Win32::API
>
> Data for 2018-11-29
> Win32::API was not in CORE (or so I think)
>
> $ corelist -a Win32API::File
>
> Data for 2018-11-29
> Win32API::File was first released with perl v5.8.9
>   v5.8.9 0.1001_01 
>   v5.9.4 0.1001
>   v5.9.5 0.1001_01 
>   v5.10.00.1001_01 
>  ...


Yes, that's present on jacana and fairywren (not on frogmouth, which is
running a very old perl, but it doesn't run TAP tests anyway.)


>
> According to the Win32API::File manual, you can request a file to be
> shared by passing the string "r" to svShare to method createFile().
> So do we really need all those extremely ugly "droppings" Juanjo added
> to the patch?
>
> (BTW the Win32API::File manual also says this:
> "The default for $svShare is "rw" which provides the same sharing as
> using regular perl open()."
> I wonder why "the regular perl open()" is not doing the sharing thing
> correctly ... has the problem has been misdiagnosed?).
>


Maybe we need "rwd"?


cheers


andrew

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

Re: Missing test of SPI copy functionality

[Mark Dilger]

On 11/6/19 6:27 PM, Michael Paquier wrote:

On Wed, Nov 06, 2019 at 04:16:14PM -0800, Mark Dilger wrote:

While working on cleaning up the SPI interface, I found that one of the SPI
error codes, SPI_ERROR_COPY, is never encountered in any test case when
running `make check-world`.  This case is certainly reachable by a user, as
is shown in the attached patch.  Is this tested from some other
infrastructure?


Hard to say, but I think that it would be good to test that part
independently anyway.  The transaction part close by is actually
getting stressed with plpgsql_transaction, so the split done in your
patch looks fine.  I'll look at it again in a couple of days to
double-check for missing spots, and commit it if there are no
objections.


Thanks for reviewing!


--
Mark Dilger

Re: TAP tests aren't using the magic words for Windows file access

[Andrew Dunstan]

On 11/7/19 9:12 AM, Alvaro Herrera wrote:
>>
>> The patch says:
>>
>> +        require Win32::API;
>> +        Win32::API->import;
> Oh, you're right, it does.  I wonder why, though:
>

On further inspection I think those lines are unnecessary. The remainder
of the patch doesn't use this at all, AFAICT.


cheers


andrew

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

Re: tableam vs. TOAST

[Ashutosh Sharma]

On Thu, Nov 7, 2019 at 7:35 PM Robert Haas  wrote:
>
> On Thu, Nov 7, 2019 at 1:15 AM Ashutosh Sharma  wrote:
> > @Robert, Myself and Prabhat have tried running the test-cases that
> > caused the checkpointer process to crash earlier multiple times but we
> > are not able to reproduce it both with and without the patch. However,
> > from the stack trace shared earlier by Prabhat, it is clear that the
> > checkpointer process panicked due to fsync failure. But, there is no
> > further data to know the exact reason for the fsync failure. From the
> > code of checkpointer process (basically the function to process fsync
> > requests) it is understood that, the checkpointer process can PANIC
> > due to one of the following two reasons.
>
> Oh, I didn't realize this was a panic due to an fsync() failure when I
> looked at the stack trace before.  I think it's concerning that
> fsync() failed on Prabhat's machine, and it would be interesting to
> know why that happened, but I don't see how this patch could possibly
> *cause* fsync() to fail, so I think we can say that whatever is
> happening on his machine is unrelated to this patch -- and probably
> also unrelated to PostgreSQL.
>

That's right and that's exactly what I mentioned in my conclusion too.

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

Re: 64 bit transaction id

[Bruce Momjian]

On Tue, Nov  5, 2019 at 09:34:52AM +1300, Thomas Munro wrote:
> On Tue, Nov 5, 2019 at 8:45 AM Tomas Vondra
> > Agreed. I think code complexity is part of the trade-off. IMO it's fine
> > to hack existing heap AM initially, and only explore the separate AM if
> > that turns out to be promising.
> 
> I thought a bit about how to make a minimally-diffferent-from-heap
> non-freezing table AM using 64 bit xids, as a thought experiment when
> trying to understand or explain to others what zheap is about.
> Committed transactions are easy (you don't have to freeze fxid
> references from the ancient past because they don't wrap around so
> they always look old), but how do you deal with *aborted* transactions
> when truncating the CLOG (given that our current rule is "if it's
> before the CLOG begins, it must be committed")?  I see three
> possibilities: (1) don't truncate the CLOG anymore (use 64 bit
> addressing and let it leak disk forever, like we did before commit
> 2589735d and later work), (2) freeze aborted transactions only, using
> a wraparound vacuum (and now you have failed, if the goal was to avoid
> having to scan all tuples periodically to freeze stuff, though
> admittedly it will require less IO to freeze only the aborted
> transactions), (3) go and remove aborted fxid references eagerly, when
> you roll back (this could be done using the undo technology that we
> have been developing to support zheap).  Another way to explain (3) is
> that this hypothetical table AM, let's call it "yheap", takes the
> minimum parts of the zheap technology stack required to get rid of
> vacuum-for-wraparound, without doing in-place updates or any of that
> hard stuff.  To make this really work you'd also have to deal with
> multixacts, which also require freezing.  If that all sounds too
> complicated, you're back to (2) which seems a bit weak to me.  Or
> perhaps I'm missing something?

The above is a very good summary of the constraints that have led to our
current handling of XID wraparound.  If we are concerned about excessive
vacuum freeze overhead, why is the default autovacuum_freeze_max_age =
2 so low?  That causes feezing when the pg_xact directory holds
200 million xids or 50 megabytes of xid status?

As far as I understand it, we cause the database to stop writes when the
xid counter gets within 2 billion xids of the current transaction
counter, so 200 million is only 1/10th to that limit, and even then, I
am not sure why we couldn't make it stop writes at 3 billion or
something.  My point is that increasing the default
autovacuum_freeze_max_age value seems like an easy way to reduce vacuum
freeze.  (While, the visibility map helps avoid vacuum freeze from
reading all heap pages, and we still need to read all index pages.)

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

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: Index Skip Scan

[Dmitry Dolgov]

> On Sun, Nov 03, 2019 at 05:45:47PM +0100, Dmitry Dolgov wrote:
> > * The extra scankeys that you are using in most of the new nbtsearch.c
> > code is an insertion scankey -- not a search style scankey. I think
> > that you should try to be a bit clearer on that distinction in
> > comments. It is already confusing now, but at least there is only zero
> > or one insertion scankeys per scan (for the initial positioning).
> >
> > * There are two _bt_skip() prototypes in nbtree.h (actually, there is
> > a btskip() and a _bt_skip()). I understand that the former is a public
> > wrapper of the latter, but I find the naming a little bit confusing.
> > Maybe rename _bt_skip() to something that is a little bit more
> > suggestive of its purpose.
> >
> > * Suggest running pgindent on the patch.
>
> Sure, I'll incorporate mentioned improvements into the next patch
> version (hopefully soon).

Here is the new version, that addresses mentioned issues.

> > * Is _bt_scankey_within_page() really doing the right thing within empty 
> > pages?
> >
> > It looks like you're accidentally using the high key when the leaf
> > page is empty with forward scans (assuming that the leaf page isn't
> > rightmost). You'll need to think about empty pages for both forward
> > and backward direction scans there.
>
> Yes, you're right, that's an issue I need to fix.

If I didn't misunderstood something, for the purpose of this function it
makes sense to return false in the case of empty page. That's what I've
added into the patch.

> > Actually, using the high key in some cases may be desirable, once the
> > details are worked out -- the high key is actually very helpful with
> > low cardinality indexes. If you populate an index with retail
> > insertions (i.e. don't just do a CREATE INDEX after the table is
> > populated), and use low cardinality data in the indexed columns then
> > you'll see this effect.
>
> Can you please elaborate a bit more? I see that using high key will help
> a forward index scans to access the minimum number of leaf pages, but
> I'm not following how is it connected to the _bt_scankey_within_page? Or
> is this commentary related in general to the whole implementation?

This question is still open.
>From f0e287da04bc314dfba48f3bfb0c8bb224938ce1 Mon Sep 17 00:00:00 2001
From: erthalion <9erthali...@gmail.com>
Date: Wed, 3 Jul 2019 16:25:20 +0200
Subject: [PATCH v28] Index skip scan

Implementation of Index Skip Scan (see Loose Index Scan in the wiki [1])
on top of IndexOnlyScan and IndexScan. To make it suitable for both
situations when there are small number of distinct values and
significant amount of distinct values the following approach is taken -
instead of searching from the root for every value we're searching for
then first on the current page, and then if not found continue searching
from the root.

Original patch and design were proposed by Thomas Munro [2], revived and
improved by Dmitry Dolgov and Jesper Pedersen.

[1] https://wiki.postgresql.org/wiki/Loose_indexscan
[2] https://www.postgresql.org/message-id/flat/CADLWmXXbTSBxP-MzJuPAYSsL_2f0iPm5VWPbCvDbVvfX93FKkw%40mail.gmail.com

Author: Jesper Pedersen, Dmitry Dolgov
Reviewed-by: Thomas Munro, David Rowley, Floris Van Nee
---
 contrib/bloom/blutils.c   |   1 +
 doc/src/sgml/config.sgml  |  15 +
 doc/src/sgml/indexam.sgml |  63 +++
 doc/src/sgml/indices.sgml |  24 +
 src/backend/access/brin/brin.c|   1 +
 src/backend/access/gin/ginutil.c  |   1 +
 src/backend/access/gist/gist.c|   1 +
 src/backend/access/hash/hash.c|   1 +
 src/backend/access/index/indexam.c|  18 +
 src/backend/access/nbtree/nbtree.c|  13 +
 src/backend/access/nbtree/nbtsearch.c | 359 
 src/backend/access/spgist/spgutils.c  |   1 +
 src/backend/commands/explain.c|  29 +
 src/backend/executor/nodeIndexonlyscan.c  |  51 +-
 src/backend/executor/nodeIndexscan.c  |  51 +-
 src/backend/nodes/copyfuncs.c |   2 +
 src/backend/nodes/outfuncs.c  |   3 +
 src/backend/nodes/readfuncs.c |   2 +
 src/backend/optimizer/path/costsize.c |   1 +
 src/backend/optimizer/path/pathkeys.c |  84 ++-
 src/backend/optimizer/plan/createplan.c   |  20 +-
 src/backend/optimizer/plan/planagg.c  |   1 +
 src/backend/optimizer/plan/planner.c  |  91 +++-
 src/backend/optimizer/util/pathnode.c |  40 ++
 src/backend/optimizer/util/plancat.c  |   1 +
 src/backend/utils/misc/guc.c  |   9 +
 src/backend/utils/misc/postgresql.conf.sample |   1 +
 src/include/access/amapi.h|   8 +
 src/include/access/genam.h|   2 +
 src/include/access/nbtree.h   |   7 +
 src/include/access/sdir.h |   7 +
 src/include/nodes/execnodes

Re: Keep compiler silence (clang 10, implicit conversion from 'long' to 'double' )

[Tom Lane]

Yuya Watari  writes:
> On Thu, Nov 7, 2019 at 3:10 PM Kyotaro Horiguchi
>  wrote:
>> +   if (unlikely(!FLOAT8_FITS_IN_INT32(num)) || isnan(num))
>> If compiler doesn't any fancy, num is fed to an arithmetic before
>> checking if it is NaN. That seems have a chance of exception.

> Thank you for pointing it out. That's my mistake. I fixed it and
> attached the patch.

Actually, that mistake is very old --- the existing functions tested
isnan() last for a long time.  I agree that testing isnan() first
is safer, but it seems that the behavior of throwing an exception
for comparisons on NaN is rarer than one might guess from the C spec.

Another issue in the patch as it stands is that the FITS_IN_ macros
require the input to have already been rounded with rint(), else they'll
give the wrong answer for values just a bit smaller than -PG_INTnn_MIN.
The existing uses of the technique did that, and interval_mul already
did too, but I had to adjust pgbench.  This is largely a documentation
failure: not only did you fail to add any commentary about the new macros,
but you removed most of the commentary that had been in-line in the
existing usages.

I fixed those things and pushed it.

regards, tom lane

Re: Reorderbuffer crash during recovery

[Andres Freund]

Hi,

On 2019-11-07 17:03:44 +0530, Amit Kapila wrote:
> On Thu, Nov 7, 2019 at 4:48 PM Tomas Vondra
>  wrote:
> >
> > I'm a bit confused - does this happen only with the logical_work_mem
> > patches, or with clean master too?
> >
> 
> This occurs with the clean master.  This is a base code problem
> revealed while doing stress testing of logical_work_mem patches.

As far as I can tell there are no repro steps included? Any chance to
get those?

Greetings,

Andres Freund

Re: [Proposal] Global temporary tables

[Konstantin Knizhnik]

On 07.11.2019 12:30, 曾文旌(义从) wrote:



May be the assumption is that all indexes has to be created before GTT start to 
be used.

Yes, Currently, GTT's index is only supported and created in an empty table 
state, and other sessions are not using it.
There has two improvements pointer:
1 Index can create on GTT(A) when the GTT(A)  in the current session is not 
empty, requiring the GTT table to be empty in the other session.
Index_build needs to be done in the current session just like a normal table. 
This improvement is relatively easy.

2 Index can create on GTT(A)  when more than one session are using this GTT(A).
Because when I'm done creating an index of the GTT in this session and setting 
it to be an valid index, it's not true for the GTT in other sessions.
Indexes on gtt in other sessions require "rebuild_index" before using it.
I don't have a better solution right now, maybe you have some suggestions.

It is possible to create index on demand:

Buffer
_bt_getbuf(Relation rel, BlockNumber blkno, int access)
{
    Buffer        buf;

    if (blkno != P_NEW)
    {
        /* Read an existing block of the relation */
        buf = ReadBuffer(rel, blkno);
        /* Session temporary relation may be not yet initialized for 
this backend. */
        if (blkno == BTREE_METAPAGE && 
GlobalTempRelationPageIsNotInitialized(rel, BufferGetPage(buf)))

        {
            Relation heap = RelationIdGetRelation(rel->rd_index->indrelid);
            ReleaseBuffer(buf);
            DropRelFileNodeLocalBuffers(rel->rd_node, MAIN_FORKNUM, blkno);
            btbuild(heap, rel, BuildIndexInfo(rel));
            RelationClose(heap);
            buf = ReadBuffer(rel, blkno);
            LockBuffer(buf, access);
        }
        else
        {
            LockBuffer(buf, access);
            _bt_checkpage(rel, buf);
        }
    }
    ...


This code initializes B-Tree and load data in it when GTT index is 
access and is not initialized yet.

It looks a little bit hacker but it works.

I also wonder why you are keeping information about GTT in shared 
memory. Looks like the only information we really need to share is 
table's metadata.
But it is already shared though catalog. All other GTT related 
information is private to backend so I do not see reasons to place it in 
shared memory.


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

Re: SKIP_LOCKED test causes random buildfarm failures

[Tom Lane]

Michael Paquier  writes:
> On Wed, Nov 06, 2019 at 03:01:11PM -0800, Andres Freund wrote:
>> I don't know what lead us to doing so, but it doesn't seem reasonable to
>> allow the user to see whether the table has actually been vacuumed. I
>> would assume that one uses SKIP_LOCKED partially to avoid unnecessary
>> impacts in production due to other tasks starting to block on e.g. a
>> VACUUM FULL, even though without the "ordered queueing" everything could
>> just go on working fine. I'm not sure that indicates whether WARNING or
>> NOTICE is the best choice.

> Good question.  That's a historical choice, still I have seen cases
> where those warnings are helpful while not making the logs too
> verbose to see some congestion in the jobs.

I kind of feel that NOTICE is more semantically appropriate, but
perhaps there's an argument for keeping it at WARNING.

>> So I'd be inclined to go with the client_min_messages approach?

> The main purpose of the tests in regress/ is to check after the
> grammar, so using client_min_messages sounds like a plan.  We have
> a second set of tests in isolation/ where I would actually like to
> disable autovacuum by default on a subset of tables.  Thoughts about
> the attached?

I do not want to fix this in the main tests by disabling autovacuum,
because that'd actually reduce the tests' cross-section.  The fact
that this happens occasionally is a Good Thing for verifying that the
code paths actually work.  So it seems that there's a consensus on
adjusting client_min_messages to fix the test output instability ---
but we need to agree on whether to do s/WARNING/NOTICE/ first, so we
can know what to set client_min_messages to.

As for the case in the isolation test, shouldn't we also use
client_min_messages there, rather than prevent the conflict
from arising?  Or would that test fail in some larger way if
autovacuum gets in the way?

regards, tom lane

Re: RFC: split OBJS lines to one object per line

[Tom Lane]

Michael Paquier  writes:
> On Tue, Nov 05, 2019 at 02:47:55PM -0800, Andres Freund wrote:
>> Pushed a patch going with the former. Let's see what the buildfarm
>> says...

> Thanks Andres.  On a rather related note, would it make sense to do
> the same for regression and isolation tests in our in-core modules?

I don't think it'd be a great idea to change parallel_schedule like
that.  Independently adding test scripts to the same parallel batch
probably won't end well: you might end up over the concurrency limit,
or the scripts might conflict through sharing table names or the like.
So I'd rather see that there's a conflict to worry about.

Anyway, merge conflicts there aren't so common IME.

regards, tom lane

plpythonu -> python3

[Christoph Berg]

The docs currently say

  The language named plpythonu implements
  PL/Python based on the default Python language variant, which is
  currently Python 2.  (This default is independent of what any
  local Python installations might consider to be
  their default, for example,
  what /usr/bin/python might be.)  The
  default will probably be changed to Python 3 in a distant future
  release of PostgreSQL, depending on the progress of the
  migration to Python 3 in the Python community.

As python2 is EOL very soon, I'd say that point is now, i.e. we should
make plpythonu.control point at plpython3u in PG13+. And probably drop
python2 support altogether.

For PG12, I have the problem that I don't want to keep supporting
python2 (Debian is already working hard on removing all python2
references), and have therefore already disabled building the
plpython2 packages for Debian, shipping only plpython3.

PostGIS developer Raúl Marín has rightfully noticed that this leaves
us without the "plpythonu" extension, forcing everyone to move to
"plpython3u" even when their code works with both.

How do other packagers handle that? Are you still supporting python2?
Would it be ok to make plpythonu.control point at python3 in PG12 in
Debian, even the upstream default is still python2?

Christoph

Re: Checking return value of SPI_execute

[Mark Dilger]

On 11/6/19 7:11 AM, Alvaro Herrera wrote:

On 2019-Nov-06, Pavel Stehule wrote:


My comment was about maybe obsolescence of this API. Probably it was
designed before exception introduction.

For example - syntax error is ended by exception. Wrong numbers of argument
is signalized by error status. I didn't study this code, but maybe was much
more effective to raise exceptions inside SPI instead return status code.
These errors are finished by exceptions, but these exceptions coming from
different places. For me it looks strange, if some functions returns error
status, but can be ended by exception too.


Yeah, I think I'd rather have more status codes and less exceptions,
than the other way around.  The problem with throwing exceptions for
every kind of error is that we don't allow exceptions to be caught (per
project policy) except to be rethrown.  It seems like for errors where
the SPI code can clean up its own resources (free memory, close portals
etc), it should do such cleanup then return SPI_SYNTAX_ERROR or whatever
and the caller can decide whether to turn this into an exception or
handle in a different way; whereas for exceptions thrown by callees (say
OOM) it would just propagate the exception.  This mean callers are
forced into adding code to check for return codes, but it allows more
flexibility.



I like to distinguish between (a) errors that can happen when a well 
written bit of C code passes possibly bad SQL through SPI, and (b) 
errors that can only happen when SPI is called from a poorly written C 
program.


Examples of (a) are SPI_ERROR_COPY and SPI_ERROR_TRANSACTION, which can 
both happen from disallowed actions within a plpgsql function.


An example of (b) is SPI_ERROR_PARAM, which only gets returned if the 
caller passed into SPI a plan which has nargs > 0 but then negligently 
passed in NULL for the args and/or argtypes.


I'd like to keep the status codes for (a) but deprecate error codes for 
(b) in favor of elog(ERROR).  I don't see that these elogs should ever 
be a problem, since getting one in testing would indicate the need to 
fix bad C code, not the need to catch an exception and take remedial 
action at run time.  Does this adequately address your concern?


My research so far indicates that most return codes are either totally 
unused or of type (b), with only a few of type (a).


--
Mark Dilger

Re: RFC: split OBJS lines to one object per line

[Andres Freund]

Hi,

On 2019-11-07 11:24:37 +0900, Michael Paquier wrote:
> On Tue, Nov 05, 2019 at 02:47:55PM -0800, Andres Freund wrote:
> > Pushed a patch going with the former. Let's see what the buildfarm
> > says...
> 
> Thanks Andres.  On a rather related note, would it make sense to do
> the same for regression and isolation tests in our in-core modules?

I don't see them as being frequent sources of conflicts (partially
because we don't change linebreaks due to line length limits, I think),
so I don't think it's really worthwhile.

One I could see some benefit in, would be the SUBDIRS makefile lines.

Greetings,

Andres Freund

Re: heapam_index_build_range_scan's anyvisible

[Andres Freund]

Hi,

On 2019-11-07 17:02:36 +0900, Michael Paquier wrote:
> On Wed, Sep 25, 2019 at 10:24:05PM -0700, Ashwin Agrawal wrote:
> > Thanks for reporting, I did indeed missed out contrib. Please find attached
> > the v2 of the patch which includes the change required in contrib as well.
> 
> Okay, that makes sense.  The patch looks good to me so I have switched
> it to ready for committer.  Andres, Robert, would you prefer
> committing this one yourself?  If not, I'll take care of it tomorrow
> after a second look.

Let me take a look this afternoon. Swapped out of my brain right now
unfortunately.

Greetings,

Andres Freund

Re: deferrable FK constraints on partitioned rels

[Alvaro Herrera]

On 2019-Nov-05, Alvaro Herrera wrote:

> Uh, somehow I posted a previous version of the patch that implements my
> rejected approach, instead of the final version I described.  Here's the
> real patch (which also includes tests).

This was broken in pg11 also.  Pushed to all branches.

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

Re: ssl passphrase callback

[Andrew Dunstan]

On 11/4/19 4:43 PM, Thomas Munro wrote:
>
> It looks like the new declarations in libpq-be.h are ifdef'd out in a
> non-USE_SSL build, but then we still try to build the new test module
> and it fails:
>
> https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.64071



I think this updated patch should fix things.


cheers


andrew


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

diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c
index 629919cc6e..bf4493c94a 100644
--- a/src/backend/libpq/be-secure-openssl.c
+++ b/src/backend/libpq/be-secure-openssl.c
@@ -45,6 +45,9 @@
 #include "utils/memutils.h"
 
 
+ssl_passphrase_func_cb ssl_passphrase_function = NULL;
+bool ssl_passphrase_function_supports_reload = false;
+
 static int	my_sock_read(BIO *h, char *buf, int size);
 static int	my_sock_write(BIO *h, const char *buf, int size);
 static BIO_METHOD *my_BIO_s_socket(void);
@@ -124,12 +127,16 @@ be_tls_init(bool isServerStart)
 	 */
 	if (isServerStart)
 	{
-		if (ssl_passphrase_command[0])
+		if (ssl_passphrase_function)
+			SSL_CTX_set_default_passwd_cb(context, ssl_passphrase_function);
+		else if (ssl_passphrase_command[0])
 			SSL_CTX_set_default_passwd_cb(context, ssl_external_passwd_cb);
 	}
 	else
 	{
-		if (ssl_passphrase_command[0] && ssl_passphrase_command_supports_reload)
+		if (ssl_passphrase_function && ssl_passphrase_function_supports_reload)
+			SSL_CTX_set_default_passwd_cb(context, ssl_passphrase_function);
+		else if (ssl_passphrase_command[0] && ssl_passphrase_command_supports_reload)
 			SSL_CTX_set_default_passwd_cb(context, ssl_external_passwd_cb);
 		else
 
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 5f30359165..18dd8578d9 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -972,17 +972,6 @@ PostmasterMain(int argc, char *argv[])
 	 */
 	LocalProcessControlFile(false);
 
-	/*
-	 * Initialize SSL library, if specified.
-	 */
-#ifdef USE_SSL
-	if (EnableSSL)
-	{
-		(void) secure_initialize(true);
-		LoadedSSL = true;
-	}
-#endif
-
 	/*
 	 * Register the apply launcher.  Since it registers a background worker,
 	 * it needs to be called before InitializeMaxBackends(), and it's probably
@@ -996,6 +985,17 @@ PostmasterMain(int argc, char *argv[])
 	 */
 	process_shared_preload_libraries();
 
+	/*
+	 * Initialize SSL library, if specified.
+	 */
+#ifdef USE_SSL
+	if (EnableSSL)
+	{
+		(void) secure_initialize(true);
+		LoadedSSL = true;
+	}
+#endif
+
 	/*
 	 * Now that loadable modules have had their chance to register background
 	 * workers, calculate MaxBackends.
diff --git a/src/include/libpq/libpq-be.h b/src/include/libpq/libpq-be.h
index 541f970f99..dd2b5fc6c8 100644
--- a/src/include/libpq/libpq-be.h
+++ b/src/include/libpq/libpq-be.h
@@ -287,6 +287,17 @@ extern void be_tls_get_peer_serial(Port *port, char *ptr, size_t len);
 extern char *be_tls_get_certificate_hash(Port *port, size_t *len);
 #endif
 
+/*
+ * ssl_passphrase_function can be filled in by a shared preloaded module
+ * to supply a passphrase for a key file, as can the flag noting whether the
+ * function supports reloading.
+ */
+
+typedef int (* ssl_passphrase_func_cb) (char *buf, int size, int rwflag,
+		void *userdata);
+extern ssl_passphrase_func_cb ssl_passphrase_function;
+extern bool ssl_passphrase_function_supports_reload;
+
 #endif			/* USE_SSL */
 
 #ifdef ENABLE_GSS
diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile
index b2eaef3bff..5f975ebcba 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -25,4 +25,9 @@ SUBDIRS = \
 		  unsafe_tests \
 		  worker_spi
 
+ifeq ($(with_openssl),yes)
+SUBDIRS += ssl_passphrase_callback
+endif
+
+
 $(recurse)
diff --git a/src/test/modules/ssl_passphrase_callback/.gitignore b/src/test/modules/ssl_passphrase_callback/.gitignore
new file mode 100644
index 00..1dbadf7baf
--- /dev/null
+++ b/src/test/modules/ssl_passphrase_callback/.gitignore
@@ -0,0 +1 @@
+tmp_check
diff --git a/src/test/modules/ssl_passphrase_callback/Makefile b/src/test/modules/ssl_passphrase_callback/Makefile
new file mode 100644
index 00..685b3aed74
--- /dev/null
+++ b/src/test/modules/ssl_passphrase_callback/Makefile
@@ -0,0 +1,25 @@
+# ssl_passphrase Makefile
+
+export with_openssl
+
+MODULE_big = ssl_passphrase_func
+OBJS = ssl_passphrase_func.o $(WIN32RES)
+PGFILEDESC = "callback function to provide a passphrase"
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = src/test/modules/ssl_passphrase_callback
+top_builddir = ../../../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
+
+check: prove-check
+
+prove-check: ssl_passphrase_func$(DLSUFFIX) | temp-install
+	@echo running prove ...
+	$(prove_check)

Re: plpythonu -> python3

[Tom Lane]

Christoph Berg  writes:
> The docs currently say
>   The language named plpythonu implements
>   PL/Python based on the default Python language variant, which is
>   currently Python 2.  (This default is independent of what any
>   local Python installations might consider to be
>   their default, for example,
>   what /usr/bin/python might be.)  The
>   default will probably be changed to Python 3 in a distant future
>   release of PostgreSQL, depending on the progress of the
>   migration to Python 3 in the Python community.

> As python2 is EOL very soon, I'd say that point is now, i.e. we should
> make plpythonu.control point at plpython3u in PG13+.

We're starting to work on that; it's not a trivial change.  Among other
things, pg_pltemplate has got pointers at plpython2 as well.  See [1]
for one preliminary step, and there are other discussions in the archives
about things we could do to make this smoother.

> And probably drop python2 support altogether.

I think it'll be quite some time before that happens.  People who
are still using ancient versions of Postgres are not likely to be
impressed by arguments about how python2 is out of support.

> For PG12, I have the problem that I don't want to keep supporting
> python2 (Debian is already working hard on removing all python2
> references), and have therefore already disabled building the
> plpython2 packages for Debian, shipping only plpython3.

You're fully within your rights to stop building plpython2 in what you
ship.  That's not an argument for removing the upstream support.

> Would it be ok to make plpythonu.control point at python3 in PG12 in
> Debian, even the upstream default is still python2?

I do not think you should do that.  This transition is going to be
painful enough without distributions making their own ad-hoc changes
that are different from what other people are doing.

Right at the moment, given that Debian and others have already stopped
shipping "/usr/bin/python", I'd say that the equivalent thing is just to
stop building plpython2, and force users to deal with the change manually.
If you didn't decide to symlink /usr/bin/python to python3 instead of
python2, what's the justification for doing the moral equivalent of that
with plpython?

regards, tom lane

[1] https://www.postgresql.org/message-id/flat/5889.1566415...@sss.pgh.pa.us

Re: Application name for pg_basebackup and friends

[Jesper Pedersen]

On 11/7/19 1:51 AM, Michael Paquier wrote:

I don't think we need a new comand line switch for it.


+1.


Please note that I have marked this patch as rejected in the CF app,
per the arguments upthread.


Ok, agreed.

Thanks for the feedback !

Best regards,
 Jesper

Re: plpythonu -> python3

[Christoph Berg]

Re: Tom Lane 2019-11-07 <14186.1573147...@sss.pgh.pa.us>
> > And probably drop python2 support altogether.
> 
> I think it'll be quite some time before that happens.  People who
> are still using ancient versions of Postgres are not likely to be
> impressed by arguments about how python2 is out of support.

Fwiw, I meant to suggest dropping python2 support in PG13+. (At the
moment there are some "interesting" scripts in src/pl/plpython that
convert the plpython2 things on the fly to be python3 compatible,
these could go away, simplifying some parts of the build system.)

Christoph

Re: Checking return value of SPI_execute

[Alvaro Herrera]

On 2019-Nov-07, Mark Dilger wrote:

> I'd like to keep the status codes for (a) but deprecate error codes for (b)
> in favor of elog(ERROR).  I don't see that these elogs should ever be a
> problem, since getting one in testing would indicate the need to fix bad C
> code, not the need to catch an exception and take remedial action at run
> time.  Does this adequately address your concern?

Yes, I think it does.


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

Re: plpythonu -> python3

[Patrik Novotny]

On Thu, Nov 7, 2019 at 6:04 PM Christoph Berg  wrote:
> How do other packagers handle that? Are you still supporting python2?
> Would it be ok to make plpythonu.control point at python3 in PG12 in
> Debian, even the upstream default is still python2?
Speaking for Fedora and RHEL, I'd say the best way to approach this from the
packager standpoint would be to simply stop building plpython2 for releases
without python2 support.

> Would it be ok to make plpythonu.control point at python3 in PG12 in
> Debian, even the upstream default is still python2?
IMHO, this should be done upstream. The reason for this would be to have
a uniform approach to this across distributions, and it is explained
in more detail
in some of the older threads of this list.


-- 
Patrik Novotný
Associate Software Engineer
Red Hat
panov...@redhat.com

Re: plpythonu -> python3

[Tom Lane]

I wrote:
> Christoph Berg  writes:
>> As python2 is EOL very soon, I'd say that point is now, i.e. we should
>> make plpythonu.control point at plpython3u in PG13+.

> We're starting to work on that; it's not a trivial change.  Among other
> things, pg_pltemplate has got pointers at plpython2 as well.  See [1]
> for one preliminary step, and there are other discussions in the archives
> about things we could do to make this smoother.

Some of that prior discussion is here:

https://www.postgresql.org/message-id/flat/5351890.TdMePpdHBD%40nb.usersys.redhat.com

One thing that'd be useful to do, perhaps, is polish up the conversion
script I posted in that thread, and make it available to users before
plpython2 disappears.  (As written, it needs both plpython versions
to be available ...)

regards, tom lane

Re: Removing pg_pltemplate and creating "trustable" extensions

[Stephen Frost]

Greetings,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Peter Eisentraut  writes:
> > On 2019-08-21 21:29, Tom Lane wrote:
> >> Patch 0001 below addresses this problem by inventing a concept of
> >> "trustable" (not necessarily trusted) extensions.  An extension that
> >> would normally require superuser permissions (e.g., because it creates
> >> C functions) can now be installed by a non-superuser if (a) it is
> >> marked trustable in the extension's control file, AND (b) it is
> >> listed as trusted in one of two new GUCs, trusted_extensions_dba and
> >> trusted_extensions_anyone.
> 
> > I think this overall direction is good.  I'm not so fond of the interfaces.

I'm not really thrilled with this interface either.

> > Using GUCs to control some of this creates yet another place where 
> > permission information is kept, and with it questions about how to get 
> > to it, how to edit it, or to back it up and restore it, etc.  Also, 
> > list-based parameters are particularly hard to manage by automated 
> > tools.  I think we can do this within the existing permission system, 
> > for example with pre-defined roles (for example, GRANT 
> > pg_create_trusted_extension ...).  Also, CREATE EXTENSION should somehow 
> > be controlled by the CREATE privilege on the containing database, so a 
> > separate setting for database owner vs. regular user might not be 
> > necessary.  Regular users would need both the role membership (given by 
> > the overall superuser) and the privilege within the database (given by 
> > the database owner).

Two things- first, this doesn't actually cover everything that the
proposed GUCs do- specifically, the proposed GUCs give you a way to
limit what specific extensions are allowed to be installed, and by whom.
Moving to a GRANT-based system removes the extension specificity and
leaves with just "is user X allowed to install extensions".  Second,
this approach is requiring that a user who is allowed to create
extensions must also be allowed to create schemas on the database in
question.

> Hm.  In principle I'm okay with the idea of having a predefined role
> or two for extension installation.  I think though that we could not
> easily make that design emulate the current behavior, wherein database
> owners automatically have the ability to install trusted PLs.  The
> superuser would have to take the additional step of granting them a
> role to let them do that.  Maybe that's just fine --- from some
> angles it could be seen as an improvement --- but it is an
> incompatibility.  Anybody have a problem with that?

I'm certainly fine with a little backwards incompatibility breakage to
remove pg_pltemplate.

> Do we need more than one level of extension trust-ability (and more
> than one predefined role to go with that)?  Assuming that we go ahead
> and mark all the safe-looking contrib modules as trustable, granting
> "pg_install_extensions" or whatever we call it would then give a DB
> owner more privilege than just the ability to install trusted PLs.
> But maybe that's fine too.

I also agree with the idea of making PLs be closer to extensions, and
this change would move us in that direction too.

> I agree with the idea of requiring a DB-level privilege as well as
> the overall role.  Is it okay to re-use the CREATE privilege (which
> today only allows for CREATE SCHEMA), or do we need another one?

If we just created another one, wouldn't that remove the need to have a
database role?  I certainly understand that default roles in the
database are useful, but I don't think we should be using them in cases
where a traditional GRANT-based privilege could be used instead, and
this certainly seems like a case where a user could just have "CREATE
EXTENSION" as a privilege GRANT'd to their role, at a database level,
and they would then be able to create (trusted) extensions in that
database.  That would also make it independent of the "CREATE SCHEMA"
privilege that we have now, removing the need to wonder about the above
question regarding combining the two.

This is far from the first time we've talked about allowing privilege
based control over who is allowed to create what kind of objects in the
system.  That kind of fine-grained control over other objects would also
be a good improvement to our privilege system (not everyone needs to be
able to create functions and operators, particularly when those are
actually roles that are logged into by services who shouldn't ever be
creating those kinds of objects even if they, maybe, need to create
tables or similar...).

> I think re-using CREATE is probably all right, since it would only be
> useful for this purpose to users who also have "pg_install_extensions".

With this, you couldn't have a user who is able to create extensions but
not able to create schemas though.  That kind of combining of privileges
together really goes against the general principle of 'least privilege',
unless the action associated with one necessairly requires the ot

Re: Removing pg_pltemplate and creating "trustable" extensions

[Tom Lane]

Stephen Frost  writes:
>> Peter Eisentraut  writes:
>>> Using GUCs to control some of this creates yet another place where 
>>> permission information is kept, and with it questions about how to get 
>>> to it, how to edit it, or to back it up and restore it, etc.  Also, 
>>> list-based parameters are particularly hard to manage by automated 
>>> tools.  I think we can do this within the existing permission system, 
>>> for example with pre-defined roles (for example, GRANT 
>>> pg_create_trusted_extension ...).  Also, CREATE EXTENSION should somehow 
>>> be controlled by the CREATE privilege on the containing database, so a 
>>> separate setting for database owner vs. regular user might not be 
>>> necessary.  Regular users would need both the role membership (given by 
>>> the overall superuser) and the privilege within the database (given by 
>>> the database owner).

> Two things- first, this doesn't actually cover everything that the
> proposed GUCs do- specifically, the proposed GUCs give you a way to
> limit what specific extensions are allowed to be installed, and by whom.
> Moving to a GRANT-based system removes the extension specificity and
> leaves with just "is user X allowed to install extensions".

True.  But do we care?  We did not have that flexibility before, either.
I'd still keep the "trustable" property (probably renamed to "trusted"
for simplicity) for extensions, so in the worst case, an admin could
edit extension control files to add or remove the per-extension flag.

> Second,
> this approach is requiring that a user who is allowed to create
> extensions must also be allowed to create schemas on the database in
> question.

That doesn't seem like a big objection from here.  We could fix it
by making a separate privilege bit, but I doubt that it's worth using
up one of our limited set of spare bits for.

>> I agree with the idea of requiring a DB-level privilege as well as
>> the overall role.  Is it okay to re-use the CREATE privilege (which
>> today only allows for CREATE SCHEMA), or do we need another one?

> If we just created another one, wouldn't that remove the need to have a
> database role?

No, because then being DB owner would be alone be enough to let you
install extensions (since as owner, you could certainly grant yourself
all privileges on the DB, even if this were somehow not the default).
We'd have to mangle GRANT's behavior to avoid that, and I don't think
we should.  Nor do I think that DB ownership ought to be enough
privilege by itself.

>> I think re-using CREATE is probably all right, since it would only be
>> useful for this purpose to users who also have "pg_install_extensions".

> With this, you couldn't have a user who is able to create extensions but
> not able to create schemas though.  That kind of combining of privileges
> together really goes against the general principle of 'least privilege',
> unless the action associated with one necessairly requires the other,
> but I don't believe that's the case here.

A point here is that many extensions involve creating their own schemas
anyway.  Also, the ability to "relocate" an extension to a different
schema is pretty meaningless if you can't create a schema to put it in.

If I thought that there were a use-case for letting someone create
extensions but not schemas, I'd be more eager to invent a new bit.
But I'm having a *really* hard time envisioning a live use-case
for that.  Granting extension-creation ability requires a whole lot
more trust in the grantee than the ability to make new schemas
(which, in themselves, have about zero impact on anybody else).

regards, tom lane

Re: Removing pg_pltemplate and creating "trustable" extensions

[Stephen Frost]

Greetings,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Stephen Frost  writes:
> >> Peter Eisentraut  writes:
> >>> Using GUCs to control some of this creates yet another place where 
> >>> permission information is kept, and with it questions about how to get 
> >>> to it, how to edit it, or to back it up and restore it, etc.  Also, 
> >>> list-based parameters are particularly hard to manage by automated 
> >>> tools.  I think we can do this within the existing permission system, 
> >>> for example with pre-defined roles (for example, GRANT 
> >>> pg_create_trusted_extension ...).  Also, CREATE EXTENSION should somehow 
> >>> be controlled by the CREATE privilege on the containing database, so a 
> >>> separate setting for database owner vs. regular user might not be 
> >>> necessary.  Regular users would need both the role membership (given by 
> >>> the overall superuser) and the privilege within the database (given by 
> >>> the database owner).
> 
> > Two things- first, this doesn't actually cover everything that the
> > proposed GUCs do- specifically, the proposed GUCs give you a way to
> > limit what specific extensions are allowed to be installed, and by whom.
> > Moving to a GRANT-based system removes the extension specificity and
> > leaves with just "is user X allowed to install extensions".
> 
> True.  But do we care?  We did not have that flexibility before, either.

I'm not 100% sure that we do, but I wanted to mention it as a
difference.  Certainly there have previously been suggestions of having
a 'whitelist' similar to what you initially proposed, that are
extensions which non-superusers are allowed to install.

> I'd still keep the "trustable" property (probably renamed to "trusted"
> for simplicity) for extensions, so in the worst case, an admin could
> edit extension control files to add or remove the per-extension flag.

At a high level, I agree with the idea of an extension being able to be
marked as one that's "trusted" or not, but we would also need to come up
with exactly what that means for it to really have value, and I don't
think we've really done that yet.

> > Second,
> > this approach is requiring that a user who is allowed to create
> > extensions must also be allowed to create schemas on the database in
> > question.
> 
> That doesn't seem like a big objection from here.  We could fix it
> by making a separate privilege bit, but I doubt that it's worth using
> up one of our limited set of spare bits for.

I do not agree that we should just shift to using default roles instead
of adding new options to GRANT because of an entirely internal
implementation detail that we could fix (and should, as I've said for
probably 10 years now...).

> >> I agree with the idea of requiring a DB-level privilege as well as
> >> the overall role.  Is it okay to re-use the CREATE privilege (which
> >> today only allows for CREATE SCHEMA), or do we need another one?
> 
> > If we just created another one, wouldn't that remove the need to have a
> > database role?
> 
> No, because then being DB owner would be alone be enough to let you
> install extensions (since as owner, you could certainly grant yourself
> all privileges on the DB, even if this were somehow not the default).
> We'd have to mangle GRANT's behavior to avoid that, and I don't think
> we should.  Nor do I think that DB ownership ought to be enough
> privilege by itself.

Really?  Why do you think that DB ownership shouldn't be enough for
this, for trusted extensions?

I agree that we don't want to mangle GRANT's behavior, at all, for this.

> >> I think re-using CREATE is probably all right, since it would only be
> >> useful for this purpose to users who also have "pg_install_extensions".
> 
> > With this, you couldn't have a user who is able to create extensions but
> > not able to create schemas though.  That kind of combining of privileges
> > together really goes against the general principle of 'least privilege',
> > unless the action associated with one necessairly requires the other,
> > but I don't believe that's the case here.
> 
> A point here is that many extensions involve creating their own schemas
> anyway.  Also, the ability to "relocate" an extension to a different
> schema is pretty meaningless if you can't create a schema to put it in.

What extensions require creating their own schema?  Every single
extension that's in contrib can be installed into the public schema
(concurrently, even) except for two hacks- plpgsql and adminpack, and
those go into pg_catalog for historical reasons more than anything else.

Creating a schema is an option for extensions but it isn't a
requirement.  I agree that you need the ability to create schemas if you
want to relocate one, but that's like needing SELECT to do an UPDATE
without a WHERE clause.  I also don't know that extension relocation is
really something that's commonly done.

> If I thought that there were a use-case for letting someone create
> extensions but not schemas,

Re: Removing pg_pltemplate and creating "trustable" extensions

[Chapman Flack]

On 11/7/19 2:13 PM, Stephen Frost wrote:

>> That doesn't seem like a big objection from here.  We could fix it
>> by making a separate privilege bit, but I doubt that it's worth using
>> up one of our limited set of spare bits for.
> 
> I do not agree that we should just shift to using default roles instead
> of adding new options to GRANT because of an entirely internal

Am I mis-following the conversation in some way? I'm having trouble
seeing this as a question about a privilege bit, because that leads
straight on to the question of what database object carries the acl
item that grants that bit to a role. An extension isn't yet a database
object until after you create it.

So isn't this more a proposal to add another boolean attribute
to pg_authid, along the lines of rolcreatedb or rolbypassrls?


On the other hand, maybe thinking of it as a privilege bit could
lead somewhere interesting. A not-yet-installed extension isn't
a real database object, but it does have a synthesized existence
as a row in the pg_available_extensions view. Maybe that could
have an acl column, where a privilege (why not just CREATE?) could
be granted to one or more roles. Synthesizing that could rely on
some directive in the control file, or in some separate
extension_creators.conf file that would associate extensions with
roles.

That would avoid using a new bit, avoid adding a pg_authid attribute,
and avoid setting in stone a particular predefined role or two or
a single final meaning of 'trusted'. A site could create a few roles
and edit extension_creators.conf to associate extensions with them.

Maybe that's just a more ad-hoc and GUCless way of circling back
to what the original proposal would be doing with GUCs

Regards,
-Chap

Re: Removing pg_pltemplate and creating "trustable" extensions

[Tom Lane]

Stephen Frost  writes:
> * Tom Lane (t...@sss.pgh.pa.us) wrote:
>> Stephen Frost  writes:
>>> Two things- first, this doesn't actually cover everything that the
>>> proposed GUCs do- specifically, the proposed GUCs give you a way to
>>> limit what specific extensions are allowed to be installed, and by whom.
>>> Moving to a GRANT-based system removes the extension specificity and
>>> leaves with just "is user X allowed to install extensions".

>> True.  But do we care?  We did not have that flexibility before, either.

> I'm not 100% sure that we do, but I wanted to mention it as a
> difference.  Certainly there have previously been suggestions of having
> a 'whitelist' similar to what you initially proposed, that are
> extensions which non-superusers are allowed to install.

Right, but I'm not sure that we need multiple layers of that.  Flags
in the extension control files are a clear and understandable mechanism
for that.  I didn't especially like the idea of a GUC-based whitelist
even when I proposed it, and Peter's points against it are compelling
too, so I don't really want to go down that path anymore.  Do you have
another mechanism in mind?

> At a high level, I agree with the idea of an extension being able to be
> marked as one that's "trusted" or not, but we would also need to come up
> with exactly what that means for it to really have value, and I don't
> think we've really done that yet.

Agreed, we'd need to have a policy for what we'd mark.  The policy that
I more or less had in mind was to mark a contrib module as trusted if it
does not provide a mechanism for privilege escalation (such as access to
the filesystem, in the case of adminpack).  Some people might feel that
"contrib module X shouldn't be trusted because I'm not convinced it hasn't
got bugs", but I fear if we start trying to make decisions on that basis,
we'll be spending a whole lot of time arguing hypotheticals.

>> That doesn't seem like a big objection from here.  We could fix it
>> by making a separate privilege bit, but I doubt that it's worth using
>> up one of our limited set of spare bits for.

> I do not agree that we should just shift to using default roles instead
> of adding new options to GRANT because of an entirely internal
> implementation detail that we could fix (and should, as I've said for
> probably 10 years now...).

The default role is not a substitute for the GRANT bit, in my view of
this.  I think that what we're saying with that, or at least what
Peter evidently had in mind, is that we want extension installers to
have *both* privileges from the superuser and privileges from the
specific DB's owner.  We can manage the latter with GRANT, but not the
former.

It's certainly arguable that requiring a superuser-granted role is
enough privilege and we shouldn't bother with having a per-DB
restriction capability.  I'd be more inclined to go that route than
to add the overhead of a brand new ACL bit.

> Really?  Why do you think that DB ownership shouldn't be enough for
> this, for trusted extensions?

DB owners have never been particularly highly privileged in the past.
I think that suddenly saying they can install extensions is moving
the understanding of that privilege level quite a bit.  Although
admittedly, the precedent of trusted PLs would point to allowing them
to install trusted extensions without further ado.  So maybe a
different take on this is "allow installing trusted extensions if you
are DB owner *or* have the pg_install_trusted_extensions role"?

regards, tom lane

Re: Removing pg_pltemplate and creating "trustable" extensions

[Tom Lane]

Chapman Flack  writes:
> So isn't this more a proposal to add another boolean attribute
> to pg_authid, along the lines of rolcreatedb or rolbypassrls?

I think we've mostly concluded that default roles are superior
to pg_authid attributes.  The latter are legacy things rather
than a model to keep extending.

> On the other hand, maybe thinking of it as a privilege bit could
> lead somewhere interesting. A not-yet-installed extension isn't
> a real database object, but it does have a synthesized existence
> as a row in the pg_available_extensions view. Maybe that could
> have an acl column, where a privilege (why not just CREATE?) could
> be granted to one or more roles. Synthesizing that could rely on
> some directive in the control file, or in some separate
> extension_creators.conf file that would associate extensions with
> roles.

Meh ... that seems like building a whole new set of infrastructure
to solve something that we already have a couple of good models
for (i.e., default roles and object-based permissions).  I really
doubt it's worth the trouble to do that.

Although upthread I mentioned the possibility of a database admin
editing extension control files, I think most people would consider
that to be a truly last resort; you generally want those files to
remain as-distributed.  The alternative of a new config file is
slightly less unmaintainable, but only slightly.  There'd be no
way to update it from inside the database, short of writing a lot
of new infrastructure comparable to ALTER SYSTEM, and surely we
don't want to do that.

> Maybe that's just a more ad-hoc and GUCless way of circling back
> to what the original proposal would be doing with GUCs

Yeah, I think if we really need per-extension configurability
of this, we're going to end up with a GUC.  It's just not worth
the trouble to build another mechanism that would support such a
need.  But I'm currently taking the position that we don't need
to support that.

regards, tom lane

Re: define bool in pgtypeslib_extern.h

[Tom Lane]

I wrote:
> I'm inclined to think that we need to make ecpglib.h's bool-related
> definitions exactly match c.h, which will mean that it has to pull in
>  on most platforms, which will mean adding a control symbol
> for that to ecpg_config.h.  I do not think we should export
> HAVE_STDBOOL_H and SIZEOF_BOOL there though; probably better to have
> configure make the choice and export something named like PG_USE_STDBOOL.

Here's a proposed patch that does it like that.

I'm of two minds about whether to back-patch or not.  This shouldn't
really change anything except on platforms where sizeof(_Bool) isn't
one.  We have some reason to think that nobody is actually using
ecpg on such platforms :-(, because if they were, they'd likely have
complained about breakage.  So maybe we should just put this in HEAD
and be done.

regards, tom lane

diff --git a/configure b/configure
index 7312bd7..21ee193 100755
--- a/configure
+++ b/configure
@@ -14729,6 +14729,12 @@ _ACEOF
 
 
 
+if test "$ac_cv_header_stdbool_h" = yes -a "$ac_cv_sizeof_bool" = 1; then
+
+$as_echo "#define PG_USE_STDBOOL 1" >>confdefs.h
+
+fi
+
 
 ##
 ## Functions, global variables
diff --git a/configure.in b/configure.in
index ea83d92..d128e59 100644
--- a/configure.in
+++ b/configure.in
@@ -1590,6 +1590,13 @@ AC_CHECK_SIZEOF([bool], [],
 #include 
 #endif])
 
+dnl We use  if we have it and it declares type bool as having
+dnl size 1.  Otherwise, c.h will fall back to declaring bool as unsigned char.
+if test "$ac_cv_header_stdbool_h" = yes -a "$ac_cv_sizeof_bool" = 1; then
+  AC_DEFINE([PG_USE_STDBOOL], 1,
+[Define to 1 to use  to define type bool.])
+fi
+
 
 ##
 ## Functions, global variables
diff --git a/src/backend/utils/fmgr/dfmgr.c b/src/backend/utils/fmgr/dfmgr.c
index be68478..c8d2cef 100644
--- a/src/backend/utils/fmgr/dfmgr.c
+++ b/src/backend/utils/fmgr/dfmgr.c
@@ -23,7 +23,7 @@
  * On macOS,  insists on including .  If we're not
  * using stdbool, undef bool to undo the damage.
  */
-#ifndef USE_STDBOOL
+#ifndef PG_USE_STDBOOL
 #ifdef bool
 #undef bool
 #endif
diff --git a/src/include/c.h b/src/include/c.h
index c95acd3..5f800a3 100644
--- a/src/include/c.h
+++ b/src/include/c.h
@@ -288,20 +288,21 @@
  * bool
  *		Boolean value, either true or false.
  *
- * Use stdbool.h if available and its bool has size 1.  That's useful for
+ * We use stdbool.h if available and its bool has size 1.  That's useful for
  * better compiler and debugger output and for compatibility with third-party
  * libraries.  But PostgreSQL currently cannot deal with bool of other sizes;
  * there are static assertions around the code to prevent that.
  *
  * For C++ compilers, we assume the compiler has a compatible built-in
  * definition of bool.
+ *
+ * Note: this stanza also appears in src/interfaces/ecpg/include/ecpglib.h.
  */
 
 #ifndef __cplusplus
 
-#if defined(HAVE_STDBOOL_H) && SIZEOF_BOOL == 1
+#ifdef PG_USE_STDBOOL
 #include 
-#define USE_STDBOOL 1
 #else
 
 #ifndef bool
diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in
index 2bf5060..249161c 100644
--- a/src/include/pg_config.h.in
+++ b/src/include/pg_config.h.in
@@ -823,6 +823,9 @@
 /* Define to best printf format archetype, usually gnu_printf if available. */
 #undef PG_PRINTF_ATTRIBUTE
 
+/* Define to 1 to use  to define type bool. */
+#undef PG_USE_STDBOOL
+
 /* PostgreSQL version as a string */
 #undef PG_VERSION
 
diff --git a/src/include/pg_config.h.win32 b/src/include/pg_config.h.win32
index 6b67fb0..6c98ef4 100644
--- a/src/include/pg_config.h.win32
+++ b/src/include/pg_config.h.win32
@@ -624,6 +624,9 @@
(--with-krb-srvnam=NAME) */
 #define PG_KRB_SRVNAM "postgres"
 
+/* Define to 1 to use  to define type bool. */
+#define PG_USE_STDBOOL 1
+
 /* A string containing the version number, platform, and C compiler */
 #define PG_VERSION_STR "Uninitialized version string (win32)"
 
diff --git a/src/interfaces/ecpg/include/ecpg_config.h.in b/src/interfaces/ecpg/include/ecpg_config.h.in
index c185561..17e93c4 100644
--- a/src/interfaces/ecpg/include/ecpg_config.h.in
+++ b/src/interfaces/ecpg/include/ecpg_config.h.in
@@ -10,6 +10,9 @@
 /* Define to 1 if `long long int' works and is 64 bits. */
 #undef HAVE_LONG_LONG_INT_64
 
+/* Define to 1 to use  to define type bool. */
+#undef PG_USE_STDBOOL
+
 /* Define to 1 to build client libraries as thread-safe code.
  *(--enable-thread-safety) */
 #undef ENABLE_THREAD_SAFETY
diff --git a/src/interfaces/ecpg/include/ecpglib.h b/src/interfaces/ecpg/include/ecpglib.h
index de9c76a..5cb31e2 100644
--- a/src/interfaces/ecpg/include/ecpglib.h
+++ b/src/interfaces/ecpg/include/ecpglib.h
@@ -1,6 +1,6 @@
 /*
- * this is a small part of c.h since we don't want to leak all postgres
- * definitions into ecpg programs
+ * Client-visible declarations for ecpglib
+ *
  * src/interfaces/ecpg/include/ecpglib.h
  */
 
@@ -8,30 +8,36 @@
 #define _ECPGLIB_H
 
 #include "libpq-fe.h"
+#include "ecpg_config.h"
 #include "

Re: Does 'instead of delete' trigger support modification of OLD

[Bruce Momjian]

On Thu, Nov  7, 2019 at 11:24:29AM +0200, Eugen Konkov wrote:
> Hello Eugen,
> 
> Thursday, November 7, 2019, 11:20:32 AM, you wrote:
> 
> >> I looked in the CREATE TRIGGER manual page and found this:
> 
> >> https://www.postgresql.org/docs/12/sql-createtrigger.html
> >> If the trigger fires before or instead of the event, the trigger
> >> can skip the operation for the current row, or change the row
> >> being inserted (for INSERT and UPDATE operations only).
> 
> >> I don't see the "(for INSERT and UPDATE operations only)" language in
> >> the main trigger documentation,
> >> https://www.postgresql.org/docs/current/trigger-definition.html.  I have
> >> written the attached patch to fix that.  Does that help?
> 
> > No.   If   we document that PG does not allow to modify OLD at instead
> > of  trigger,  the  we can not implement that. Probably we can put note
> > that  "currently  modification of the trigger row for RETURNING is not
> > implemented"
> 
> sorry, typo. Please read:
> "currently  modification of the trigger row for DELETE RETURNING is 
> notimplemented"

In looking at the existing docs, the bullet above the quoted text says:

For row-level INSERT and UPDATE triggers only, the returned row becomes
 
the row that will be inserted or will replace the row being updated.
This allows the trigger function to modify the row being inserted or
updated.

First, notice "only", which was missing from the later sentence:

For INSERT and UPDATE
operations [only], the trigger may modify the
NEW row before returning it.

which I have now added with my applied patch to all supported releases. 

The major use of modifying NEW is to modify the data that goes into the
database, and its use to modify data seen by later executed triggers, or
by RETURNING, is only a side-effect of its primary purpose.  Therefore,
it is not surprising that, since DELETE does not modify any data, just
removes it, that the modification of OLD to appear in later triggers or
RETURNING is not supported.

> >> As far as allowing DELETE to modify the trigger row for RETURNING, I am
> >> not sure how much work it would take to allow that, but it seems like it
> >> is a valid requite, and if so, I can add it to the TODO list.
> 
> > Yes,  Add please into TODO the feature to "allowing DELETE to modify the 
> > trigger row
> > for  RETURNING".  Becuase, as I have described at first letter, without
> > this the RETURNING rows **does not correspond actually deleted data**
> 
> > Thank you.

I have added a TODO item:

Allow DELETE triggers to modify rows, for use by RETURNING 

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

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: Safeguards against incorrect fd flags for fsync()

[Mark Dilger]

On 10/8/19 11:26 PM, Michael Paquier wrote:

Hi all,

After the set of issues discussed here, it seems to me that it would
be a good thing to have some safeguards against incorrect flags when
opening a fd which would be used for fsync():
https://www.postgresql.org/message-id/16039-196fc97cc05e1...@postgresql.org

Attached is a patch aimed at doing that.  Historically O_RDONLY is 0,
so when looking at a directory we just need to make sure that no write
flags are used.  For files, that's the contrary, a write flag has to
be used.

Thoughts or better ideas?


The code and comments don't clearly indicate what you have said in the 
email, that you are verifying directories are opened read-only and files 
are opened either read-write or write-only.  I'd recommend changing the 
comments a bit to make that clearer.


I would also rearrange the code a little, as it is slightly clearer to read:

if (x)
/* directory stuff */
else
/* file stuff */

than as you have it:

if (!x)
/* file stuff */
else
/* directory stuff */

because it takes slightly less time for somebody reading the code when 
they don't have to think about the negation of x.


I'm a little uncertain about ignoring fstat errors as you do, but left 
that part of the logic alone.  I understand that any fstat error will 
likely be immediately followed by another error when the fsync is 
attempted, but relying on that seems vaguely similar to the security 
vulnerability of checking permissions and then opening a file as two 
separate operations.  Not sure the analogy actually holds for fstat 
before fsync, though.


Attached is a revised version of the patch.  Perhaps you can check what 
I've done and tell me if I've broken it.



--
Mark Dilger
diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c
index fe2bb8f859..1cddd590b8 100644
--- a/src/backend/storage/file/fd.c
+++ b/src/backend/storage/file/fd.c
@@ -330,6 +330,41 @@ static int fsync_parent_path(const char *fname, int 
elevel);
 int
 pg_fsync(int fd)
 {
+#ifdef USE_ASSERT_CHECKING
+   struct stat st;
+
+   /*
+* Some operating system implementations of fsync have requirements 
about
+* the file access modes that were used when their file descriptor 
argument
+* was opened, and these requirements differ depending on whether the 
file
+* descriptor is for a directory.
+*
+* For any file descriptor that may eventually be handed to fsync, we
+* should have opened it with access modes that are compatible with 
fsync
+* on all supported systems, otherwise the code may not be portable, 
even
+* if it runs ok on the current system.
+*
+* We assert here that a descriptor for a file was opened with write
+* permissions (either O_RDWR or O_WRONLY) and for a directory was 
opened
+* without write permissions (O_RDONLY).
+*
+* Ignore any fstat errors.
+*/
+   if (fstat(fd, &st) == 0)
+   {
+   int desc_flags = fcntl(fd, F_GETFL);
+
+   /*
+* O_RDONLY is historically 0, so just make sure that for
+* directories no write flags are used.
+*/
+   if (S_ISDIR(st.st_mode))
+   Assert((desc_flags & (O_RDWR | O_WRONLY)) == 0);
+   else
+   Assert((desc_flags & (O_RDWR | O_WRONLY)) != 0);
+   }
+#endif
+
/* #if is to skip the sync_method test if there's no need for it */
 #if defined(HAVE_FSYNC_WRITETHROUGH) && !defined(FSYNC_WRITETHROUGH_IS_FSYNC)
if (sync_method == SYNC_METHOD_FSYNC_WRITETHROUGH)

Re: log bind parameter values on error

[Alvaro Herrera]

So, if some parameters are large (they can be up to 1 GB-1, remember)
then we can bloat the log file severely.  I think we need to place an
upper limit on the strings that we're going to log -- as inspiration,
callers of ExecBuildValueDescription uses 64 chars per value maximum.
Something like that seems reasonable.  So I think you need to add some
pg_mbcliplen() calls in a couple of places in exec_bind_message.

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

Re: log bind parameter values on error

[Alvaro Herrera]

On 2019-Nov-07, Alvaro Herrera wrote:

> So, if some parameters are large (they can be up to 1 GB-1, remember)
> then we can bloat the log file severely.  I think we need to place an
> upper limit on the strings that we're going to log -- as inspiration,
> callers of ExecBuildValueDescription uses 64 chars per value maximum.
> Something like that seems reasonable.  So I think you need to add some
> pg_mbcliplen() calls in a couple of places in exec_bind_message.

(BTW it looks like plpgsql_param_fetch() is creating ParamExternData
values and not setting textValue for them.)

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

Re: TestLib::command_fails_like enhancement

[Mark Dilger]

On 10/31/19 10:02 AM, Andrew Dunstan wrote:


This small patch authored by my colleague Craig Ringer enhances
Testlib's command_fails_like by allowing the passing of extra keyword
type arguments. The keyword initially recognized is 'extra_ipcrun_opts'.
The value for this keyword needs to be an array, and is passed through
to the call to IPC::Run.


Hi Andrew, a few code review comments:

The POD documentation for this function should be updated to include a 
description of the %kwargs argument list.


Since command_fails_like is patterned on command_like, perhaps you 
should make this change to both of them, even if you only originally 
intend to use the new functionality in command_fails_like.  I'm not sure 
of this, though, since I haven't seen any example usage yet.


I'm vaguely bothered by having %kwargs gobble up the remaining function 
arguments, not because it isn't a perl-ish thing to do, but because none 
of the other functions in this module do anything similar.  The function 
check_mode_recursive takes an optional $ignore_list array reference as 
its last argument.  Perhaps command_fails_like could follow that pattern 
by taking an optional $kwargs hash reference.


--
Mark Dilger

Re: Does 'instead of delete' trigger support modification of OLD

[Bruce Momjian]

On Thu, Nov  7, 2019 at 04:26:55PM -0500, Bruce Momjian wrote:
> On Thu, Nov  7, 2019 at 11:24:29AM +0200, Eugen Konkov wrote:
> > >> As far as allowing DELETE to modify the trigger row for RETURNING, I am
> > >> not sure how much work it would take to allow that, but it seems like it
> > >> is a valid requite, and if so, I can add it to the TODO list.
> > 
> > > Yes,  Add please into TODO the feature to "allowing DELETE to modify the 
> > > trigger row
> > > for  RETURNING".  Becuase, as I have described at first letter, without
> > > this the RETURNING rows **does not correspond actually deleted data**
> > 
> > > Thank you.
> 
> I have added a TODO item:
> 
>   Allow DELETE triggers to modify rows, for use by RETURNING 

Thinking some more on this, I now don't think a TODO makes sense, so I
have removed it.

Triggers are designed to check and modify input data, and since DELETE
has no input data, it makes no sense.  In the attached SQL script, you
can see that only the BEFORE INSERT trigger fires, so there is no way
even with INSERT to change what is passed after the write to RETURNING. 
What you can do is to modify the returning expression, which is what I
have done for the last query --- hopefully that will help you.

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

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +


trigtest.sql
Description: application/sql

SPI refactoring

[Mark Dilger]

Hackers,

As discussed with Tom in [1] and again with Pavel and Alvaro in [2], 
here is a partial WIP refactoring of the SPI interface.  The goal is to 
remove as many of the SPI_ERROR_xxx codes as possible from the 
interface, replacing them with elog(ERROR), without removing the ability 
of callers to check meaningful return codes and make their own decisions 
about what to do next.  The crucial point here is that many of the error 
codes in SPI are "can't happen" or "you made a programmatic mistake" 
type errors that don't require run time remediation, but rather require 
fixing the C code and recompiling.  Those seem better handled as an 
elog(ERROR) to avoid the need for tests against such error codes 
sprinkled throughout the system.


One upshot of the refactoring is that some of the SPI functions that 
previously returned an error code can be changed to return void.  Tom 
suggested a nice way to use macros to provide backward compatibility 
with older third-party software, and I used his suggestion.


I need guidance with SPI_ERROR_ARGUMENT because it is used by functions 
that don't return an integer error code, but rather return a SPIPlanPtr, 
such as SPI_prepare.  Those functions return NULL and set a global 
variable named SPI_result to the error code.  I'd be happy to just 
convert this to throwing an error, too, but it is a greater API break 
than anything implemented in the attached patches so far.  How do others 
feel about it?


If more places like this can be converted to use elog(ERROR), it may be 
possible to convert more functions to return void.



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

[2] 
https://www.postgresql.org/message-id/20191106151112.GA12251%40alvherre.pgsql

--
Mark Dilger
>From 113d42772be2c2abd71fd142cde9240522f143d7 Mon Sep 17 00:00:00 2001
From: Mark Dilger 
Date: Thu, 7 Nov 2019 07:51:06 -0800
Subject: [PATCH v1 1/5] Deprecating unused SPI error codes.

The SPI_ERROR_NOOUTFUNC and SPI_ERROR_CONNECT codes, defined in spi.h,
were no longer used anywhere in the sources except nominally in spi.c
where SPI_result_code_string(int code) was translating them to a cstring
for use in error messages.  But since the system never returns these
error codes, it seems unlikely anybody still needs to be able to convert
them to a string.

Removing these from spi.c, from the docs, and from a code comment in
contrib.  Leaving the definition in spi.h for backwards compatibility of
third-party applications.
---
 contrib/spi/refint.c   |  2 +-
 doc/src/sgml/spi.sgml  | 14 ++
 src/backend/executor/spi.c |  4 
 src/include/executor/spi.h |  4 ++--
 4 files changed, 5 insertions(+), 19 deletions(-)

diff --git a/contrib/spi/refint.c b/contrib/spi/refint.c
index adf0490f85..00253a63e9 100644
--- a/contrib/spi/refint.c
+++ b/contrib/spi/refint.c
@@ -392,7 +392,7 @@ check_foreign_key(PG_FUNCTION_ARGS)
 			char	   *oldval = SPI_getvalue(trigtuple, tupdesc, fnumber);
 			char	   *newval;
 
-			/* this shouldn't happen! SPI_ERROR_NOOUTFUNC ? */
+			/* this shouldn't happen! */
 			if (oldval == NULL)
 /* internal error */
 elog(ERROR, "check_foreign_key: SPI_getvalue returned %s", SPI_result_code_string(SPI_result));
diff --git a/doc/src/sgml/spi.sgml b/doc/src/sgml/spi.sgml
index 9e37fc3a14..d1e6582d96 100644
--- a/doc/src/sgml/spi.sgml
+++ b/doc/src/sgml/spi.sgml
@@ -128,14 +128,6 @@ int SPI_connect_ext(int options)
 

 
-   
-SPI_ERROR_CONNECT
-
- 
-  on error
- 
-
-   
   
  
 
@@ -3231,12 +3223,10 @@ char * SPI_getvalue(HeapTuple row, TupleDesc r
   Return Value
 
   
-   Column value, or NULL if the column is null,
+   Column value, or NULL if the column is null, or
colnumber is out of range
(SPI_result is set to
-   SPI_ERROR_NOATTRIBUTE), or no output function is
-   available (SPI_result is set to
-   SPI_ERROR_NOOUTFUNC).
+   SPI_ERROR_NOATTRIBUTE).
   
  
 
diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index 2c0ae395ba..2bb49e6919 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -1708,8 +1708,6 @@ SPI_result_code_string(int code)
 
 	switch (code)
 	{
-		case SPI_ERROR_CONNECT:
-			return "SPI_ERROR_CONNECT";
 		case SPI_ERROR_COPY:
 			return "SPI_ERROR_COPY";
 		case SPI_ERROR_OPUNKNOWN:
@@ -1724,8 +1722,6 @@ SPI_result_code_string(int code)
 			return "SPI_ERROR_TRANSACTION";
 		case SPI_ERROR_NOATTRIBUTE:
 			return "SPI_ERROR_NOATTRIBUTE";
-		case SPI_ERROR_NOOUTFUNC:
-			return "SPI_ERROR_NOOUTFUNC";
 		case SPI_ERROR_TYPUNKNOWN:
 			return "SPI_ERROR_TYPUNKNOWN";
 		case SPI_ERROR_REL_DUPLICATE:
diff --git a/src/include/executor/spi.h b/src/include/executor/spi.h
index ad69a5ce43..a2ea6dda5f 100644
--- a/src/include/executor/spi.h
+++ b/src/include/executor/spi.h
@@ -36,7 +36,7 @@ typedef struct SPITupleTable
 /* Plans are opaque structs for standard users of SPI */
 typedef struct _SPI_plan *SPIPlanPtr;
 
-#define SPI_ERROR_

Re: log bind parameter values on error

[Andres Freund]

Hi,

On 2019-11-05 12:07:50 +, Alexey Bashtanov wrote:
> > What I'm suggesting is that PortalStart() would build a string
> > representation out of the parameter list (using
> > ParamExternData.textValue if set, calling the output function
> > otherwise), and stash that in the portal.
> > 
> > At the start of all the already existing PG_TRY blocks in pquery.c we
> > push an element onto the error_context_stack that adds the errdetail
> > with the parameters to the error.  Alternatively we could also add them
> > in in the PG_CATCH blocks, but that'd only work for elevel == ERROR
> > (i.e. neither FATAL nor non throwing log levels would be able to enrich
> > the error).
> 
> I'm a bit worried about going this way, as it makes us log
> the query and its parameters too far apart in the code,
> and it's trickier to make sure we never log parameters without the query.

The way you do it you need to do it in numerous places, and I'm pretty
sure you're missing some already. E.g. this will not work to log
parameters for parametrized statements generated on the server side,
e.g. for foreign key queries. I don't think that's the right direction
to go. You can maybe argue that we don't need support for logging server
side generated parameters in the initial version, but the approach
should be compatible with adding that support.


> I think logging the parameters should not be part of error_context_stack,
> but rather a primary part of logging facility itself, like statement.
> That's because whether we want to log parameters depends
> on print_stmt in elog.c. With print_stmt being computed upon edata,
> I'm not sure how to work it out nicely.

I don't think those really are contradictions. You can continue to have
an errdetail_params(), and but call it from the error context callback
set up in the portal code.

That'd also get rid of the two different copies of the logic for
deciding whether to log bind parameters. Right now your approach
e.g. doesn't work for emit_log_hook. Nor is all that clear to me that
we'd never want to send this information to the client.

Even leaving that aside, I'm *STRONGLY* against entangling elog.c with
query execution details like ParamListInfo. We should work to make
elog.c know less about different parts of the system, not more.


> > Thinking about this: I think the current approach doesn't actually
> > handle recursive errors correctly. Even if we fail to emit the error
> > message due to the parameter details requiring a lot of memory, we'd
> > again do so (and again fail) when handling that OOM error, because
> > current_top_portal afaict would still be set.  At the very least this'd
> > need to integrate with the recursion_depth logic in elog.c.

> We handle it the same way as we do it for debug_query_string itself:
> 
>         if (in_error_recursion_trouble())
>         {
>             error_context_stack = NULL;
>             debug_query_string = NULL;
>             current_top_portal = NULL;
>         }
> 

Sorry, had missed that, it wasn't apparent in the diff. I personally
just always use a larger context size to make it easier to recognize...

Greetings,

Andres Freund

Collation versions on Windows (help wanted, apply within)

[Thomas Munro]

Hello hackers,

Here's a draft patch that teaches PostgreSQL how to ask for collation
versions on Windows.  It receives a pair of DWORDs, which, it displays
as hex.  The values probably have an internal structure that is
displayed in a user-friendly way by software like Active Directory and
SQL Server (I'm pretty sure they both track collation versions and
reindex), but I don't know.  It is based on the documentation at:

https://docs.microsoft.com/en-us/windows/win32/win7appqual/nls-sorting-changes

My understanding of our OS and tool chain version strategy on that
platform is limited, but it looks like we only allow ourselves to use
Vista (and later) APIs if the compiler is Visual Studio 2015 (aka
14.0) or later.  So I tested that this builds cleanly on AppVeyor
using that compiler (see attached CI patch).  The regression tests
failed with Windows error code 87 before I added in the check to skip
"C" and "POSIX", so I know the new code is reached.  I don't have an
environment to test it beyond that.

The reason for returning an empty string for "C" and "POSIX" is the
following comment for get_collation_actual_version():

 * A particular provider must always either return a non-NULL string or return
 * NULL (if it doesn't support versions).  It must not return NULL for some
 * collcollate and not NULL for others.

I'm not sure why, or if that really makes sense.

Do any Windows hackers want to help get it into shape?  Some things to
do: test it, verify that the _WIN32_WINNT >= 0x0600 stuff makes sense
(why do we target such ancient Windows releases anyway?), see if there
is way we could use GetNLSVersion() (no "Ex") to make this work on
older Windows system, check if it makes sense to assume that
collcollate is encoded with CP_ACP ("the system default Windows ANSI
code page", used elsewhere in the PG source tree for a similar
purpose, but this seems likely to go wrong for locale names that have
non-ASCII characters, and indeed we see complaints on the lists
involving the word "Bokmål"), and recommend a better way to display
the collation version as text.  I'll add this to the next commitfest
to attract some eyeballs (but note that when cfbot compiles it, it
will be using an older tool chain and Win32 target, so the new code
will be ifdef'd out and regression test success means nothing).

To test that it works, you'd need to look at the contents of
pg_collation to confirm that you see the new version strings, create
an index on a column that explicitly uses a collation that has a
version, update the pg_collation table by hand to have a to a
different value, and then open a new session and to access the index
to check that you get a warning about the version changing.  The
warning can be cleared by using ALTER COLLATION ... REFRESH VERSION.


0001-Add-collation-versions-for-Windows.patch
Description: Binary data


0002-CI.patch
Description: Binary data

Re: Should we make scary sounding, but actually routine, errors less scary?

[Andres Freund]

Hi,

On 2019-11-05 22:00:58 -0500, Chapman Flack wrote:
> On 11/05/19 18:54, Andres Freund wrote:
> > Hi,
> > 
> > There's a few errors that we issue that are, often, much less bad than
> > they sound. The most common cases I immediately can recall are:
> > 
> > 
> > 1) Mentioning crash, once for each backend, when shutting down
> > immediately. Currently the log output for that, with just two sessions
> > connected, is the following:
> 
> While on the topic ... this may be more a property of particular
> packagings of the server, to run under systemd etc., but often there
> is a process during startup trying periodically to open a connection
> to the server to confirm that it has successfully started, and the
> result is a dozen or so log messages that say "FATAL: the server is
> starting" ... which is amusing once you get what it's doing, but a bit
> disconcerting until then.

I think that is best solved by using pg_ctl's logic to look at the
postmaster state file, rather than connecting before the server is
ready. For one connecting requires to actually be able to connect, which
isn't always a given.  If using pg_ctl is problematic for some reason,
it'd imo be better to extract the relevant logic into its own tool.


> Not sure how that could be changed ... maybe a connection-time option
> trial_connection that would suppress the fatal ereport on rejecting
> the connection?

I think that'd be a recipe for hard to debug issues. Imagine somebody
DOSing the server and setting that option - you'd have no way to
actually see what's happening.

Greetings,

Andres Freund

Re: Should we make scary sounding, but actually routine, errors less scary?

[Andres Freund]

Hi,

On 2019-11-06 17:36:09 +0900, Kyotaro Horiguchi wrote:
> At Tue, 5 Nov 2019 15:54:22 -0800, Andres Freund  wrote 
> in 
> > Hi,
> > 
> > There's a few errors that we issue that are, often, much less bad than
> > they sound. The most common cases I immediately can recall are:
> > 
> > 
> > 1) Mentioning crash, once for each backend, when shutting down
> > immediately. Currently the log output for that, with just two sessions
> > connected, is the following:
> > 
> > 2019-11-05 15:09:52.634 PST [9340][] LOG:  0: received immediate 
> > shutdown request
> > 2019-11-05 15:09:52.634 PST [9340][] LOCATION:  pmdie, postmaster.c:2883
> > 2019-11-05 15:09:52.634 PST [23199][4/0] WARNING:  57P02: terminating 
> > connection because of crash of another server process
> > 2019-11-05 15:09:52.634 PST [23199][4/0] DETAIL:  The postmaster has 
> > commanded this server process to roll back the current transaction and 
> > exit, because another server process exited abnormally and possibly 
> > corrupted shared memory.
> > 2019-11-05 15:09:52.634 PST [23199][4/0] HINT:  In a moment you should be 
> > able to reconnect to the database and repeat your command.
> > 2019-11-05 15:09:52.634 PST [23199][4/0] LOCATION:  quickdie, 
> > postgres.c:2734
> > 2019-11-05 15:09:52.634 PST [23187][3/0] WARNING:  57P02: terminating 
> > connection because of crash of another server process
> > 2019-11-05 15:09:52.634 PST [23187][3/0] DETAIL:  The postmaster has 
> > commanded this server process to roll back the current transaction and 
> > exit, because another server process exited abnormally and possibly 
> > corrupted shared memory.
> > 2019-11-05 15:09:52.634 PST [23187][3/0] HINT:  In a moment you should be 
> > able to reconnect to the database and repeat your command.
> > 2019-11-05 15:09:52.634 PST [23187][3/0] LOCATION:  quickdie, 
> > postgres.c:2734
> > 2019-11-05 15:09:52.634 PST [9345][1/0] WARNING:  57P02: terminating 
> > connection because of crash of another server process
> > 2019-11-05 15:09:52.634 PST [9345][1/0] DETAIL:  The postmaster has 
> > commanded this server process to roll back the current transaction and 
> > exit, because another server process exited abnormally and possibly 
> > corrupted shared memory.
> > 2019-11-05 15:09:52.634 PST [9345][1/0] HINT:  In a moment you should be 
> > able to reconnect to the database and repeat your command.
> > 2019-11-05 15:09:52.634 PST [9345][1/0] LOCATION:  quickdie, postgres.c:2734
> > 2019-11-05 15:09:52.644 PST [9340][] LOG:  0: database system is shut 
> > down
> > 2019-11-05 15:09:52.644 PST [9340][] LOCATION:  UnlinkLockFiles, 
> > miscinit.c:859
> > 
> > (23199, 23187 are backends, 9345 is the autovacuum launcher)
> > 
> > I think there's multiple things wrong with this. For one, reading that
> > the server has (no might in there) crashed, is scary, when all that's
> > happening is an intentional shutdown.  But also the sheer log volume is
> > bad - on a busy server there may be a *lot* of these lines.
> > 
> > I think the log volume is bad even for an actual PANIC. I've spent *way*
> > too much time scrolling through pages and pages of the above lines, just
> > to find the one or two lines indicating an actual error.
> >
> > It seems like we ought to be able to somehow
> > 
> > a) Signal that the server has been shut down in immediate mode, rather
> > than actually crashed, and issue a different log message to the user.
> > 
> > b) Stop issuing, to the server log, the same message over and over. We
> > instead just ought to send the message to the client. We however need to
> > be careful that we don't make it harder to debug a SIGQUIT sent directly
> > to backend processes.
> 
> I doubt that different messages for server log and client worth
> doing. Isn't it enough moving the cause description from backend
> message to postmaster one?

I'm not quite following what you're suggesting?


> Addition to that, I don't see such a message on connecting psql. It
> just reports as "server closed the connection unexpectedly" without a
> server message. If I'm not missing something the HINT message is
> useless..

It depends on the state of the connection IIRC, whether you'll get the
error message or not. There's some cases where we might get notified
about the connection having been closed, without actually reading the
error message.  There were some libpq fixes around this not too long
ago, is it possible that you're running a version of psql linked against
an older libpq version (e.g. from the OS)?


I'm not a fan of the above error message libpq generates - it logs this
in plenty cases where there was a network error. Talking about server
crashes when simple network issues may be the reason imo is not a great
idea.




> > 3) When a standby node is shutdown in immediate mode, we issue:
> > 
> > 2019-11-05 15:45:58.722 PST [30321][] LOG:  database system was interrupted 
> > while in recovery at log time 2019-11-05 15:37:43 PST
> > 2019-11-05 15:45:58.

Re: SKIP_LOCKED test causes random buildfarm failures

[Michael Paquier]

On Thu, Nov 07, 2019 at 11:50:25AM -0500, Tom Lane wrote:
> Michael Paquier  writes:
>> Good question.  That's a historical choice, still I have seen cases
>> where those warnings are helpful while not making the logs too
>> verbose to see some congestion in the jobs.
> 
> I kind of feel that NOTICE is more semantically appropriate, but
> perhaps there's an argument for keeping it at WARNING.

Perhaps.  Well, that's the same level as the one used after the
permission checks on the relation vacuumed.

> I do not want to fix this in the main tests by disabling autovacuum,
> because that'd actually reduce the tests' cross-section.  The fact
> that this happens occasionally is a Good Thing for verifying that the
> code paths actually work.  So it seems that there's a consensus on
> adjusting client_min_messages to fix the test output instability ---
> but we need to agree on whether to do s/WARNING/NOTICE/ first, so we
> can know what to set client_min_messages to.

Makes sense. 

> As for the case in the isolation test, shouldn't we also use
> client_min_messages there, rather than prevent the conflict
> from arising?  Or would that test fail in some larger way if
> autovacuum gets in the way?

I think that there is no risk regarding the stability of the output
because we use LOCK before from a first session on the relation to
vacuum in a second session.  So if autovacuum runs in parallel, the
consequence would be a small slow down while waiting on the lock to be
taken.  And per the way the test is ordered, it seems to me that it
makes the most sense to disable autovacuum as it would just get in the
way.  In this case I think that it is actually better to show the
messages as that makes the tests more verbose and we make sure to test
their format, even if we could just rely on the fact that VACUUM
should just be blocking or non-blocking.
--
Michael


signature.asc
Description: PGP signature

Re: heapam_index_build_range_scan's anyvisible

[Michael Paquier]

On Thu, Nov 07, 2019 at 09:25:40AM -0800, Andres Freund wrote:
> Let me take a look this afternoon. Swapped out of my brain right now
> unfortunately.

Thanks for the update.
--
Michael


signature.asc
Description: PGP signature

Re: [PATCH][DOC] Fix for PREPARE TRANSACTION doc and postgres_fdw message.

[Michael Paquier]

On Thu, Nov 07, 2019 at 06:40:36PM +0900, Etsuro Fujita wrote:
> On Thu, Nov 7, 2019 at 5:28 PM Kyotaro Horiguchi
>  wrote:
>> At Thu, 7 Nov 2019 17:20:07 +0900, Etsuro Fujita  
>> wrote in
>>> Only two people complaining about the wording?  Considering as well

That's like..  Half the folks participating to this thread ;)

>>> that we use that wording in the docs and there were no complains about
>>> that IIRC, I don't feel a need to change that, TBH.
>> But the most
>> significant point in the previous mail is using "foreign tables using
>> postgres_fdw" instead of "postgres_fdw foreign tables".
> 
> OK, but as I said above, I don't feel the need to change that.  How
> about leaving it for another patch to improve the wording in that
> message and/or the documentation if we really need to do it.

Fine by me.  If I were to put a number on that, I would be around +-0,
so I don't have an actual objection with your point of view either.
--
Michael


signature.asc
Description: PGP signature

Re: Add SQL function to show total block numbers in the relation

[Michael Paquier]

On Thu, Nov 07, 2019 at 06:01:34PM +0900, Kyotaro Horiguchi wrote:
> Sorry, but I also vote -1 for the new function.

So do I.  If there are no objections, I will mark the patch as
rejected in the CF app.

> If they need it so frequently, a user-defined function is easily
> made up.

Yep.
--
Michael


signature.asc
Description: PGP signature

Re: pg_waldump and PREPARE

[Michael Paquier]

On Tue, Sep 03, 2019 at 10:00:08AM +0900, Fujii Masao wrote:
> Sorry for the long delay... Yes, I will update the patch if necessary.

Fujii-san, are you planning to update this patch then?  I have
switched it as waiting on author.
--
Michael


signature.asc
Description: PGP signature

Re: add a MAC check for TRUNCATE

[Michael Paquier]

On Mon, Sep 30, 2019 at 11:38:05AM -0300, Alvaro Herrera wrote:
> On 2019-Sep-30, Joe Conway wrote:
> 
> > I am not sure I will get to this today. I assume it is ok for me to move
> > it forward e.g. next weekend, or is that not in line with commitfest rules?
> 
> You can commit whatever patch whenever you feel like it.  I will
> probably move this patch to the next commitfest before that, but you can
> mark it committed there as soon as you commit it.

One month later, nothing has happened here.  Joe, are you planning to
look at this patch?

The last patch I found does not apply properly, so please provide a
rebase.  I am switching the patch as waiting on author.
--
Michael


signature.asc
Description: PGP signature

Re: pg_waldump and PREPARE

[Fujii Masao]

On Fri, Nov 8, 2019 at 9:41 AM Michael Paquier  wrote:
>
> On Tue, Sep 03, 2019 at 10:00:08AM +0900, Fujii Masao wrote:
> > Sorry for the long delay... Yes, I will update the patch if necessary.
>
> Fujii-san, are you planning to update this patch then?  I have
> switched it as waiting on author.

No because there has been nothing to update in the latest patch for now
unless I'm missing something. So I'm just waiting for some new review
comments against the latest patch to come :)
Can I switch the status back to "Needs review"?

Regards,

-- 
Fujii Masao

Re: Collation versioning

[Thomas Munro]

On Thu, Nov 7, 2019 at 10:20 PM Julien Rouhaud  wrote:
> Attached 4th patch handles default collation.  I went with an
> ignore_systempin flag in recordMultipleDependencies.

Thanks for working on this!  I haven't looked closely or tested yet,
but this seems reasonable.  Obviously it assumes that the default
provider is really "libc" in disguise for now, but Peter's other patch
will extend that to cover ICU later.

> > > * Andres mentioned off-list that pg_depend rows might get blown away
> > > and recreated in some DDL circumstances.  We need to look into that.
>
> I tried various flavour of DDL but I couldn't wipe out the pg_depend
> rows without having an index rebuild triggered (like changing the
> underlying column datatype).  Do you have any scenario where the index
> rebuild wouldn't be triggered?

Ah, OK, if we only do that when the old index contents will also be
destroyed, that's great news.

> > > * Another is that pg_upgrade won't preserve pg_depend rows, so you'd
> > > need some catalog manipulation (direct or via new DDL) to fix that.
>
> Attached 5th patch add a new "ALTER INDEX idx_name DEPENDS ON
> COLLATION coll_oid VERSION coll_version_text" that can only be
> executed in binary upgrade mode, and teach pg_dump to generate such
> commands (including for indexes created for constraints).

It's nice that you were able to make up a reasonable grammar out of
existing keywords.  I wonder if we should make this user accessible...
it could be useful for expert users.  If so, maybe it should use
collation names, not OIDs?

> One issue
> is that older versions don't have pg_depend information,  so pg_dump
> can't find the dependencies to generate such commands and override the
> version with anything else.  It means that the upgraded cluster will
> show all indexes as depending on the current collation provider
> version.  I'm not sure if that's the best thing to do, or if we should
> change all pg_depend rows to mention "unknown" version or something
> like that.  It would generate so much noise that it's probably not
> worth it.

Right, so this is basically a policy decision: do we assume that all
pre-13 indexes that depend on collations are potentially corrupted, or
assume that they are not?  The "correct" thing to do would be to
assume they are potentially corrupted and complain until the user
reindexes, but I think the pragmatic thing to do would be to assume
that they're not and just let them adopt the current versions, even
though it's a lie.  I lean towards the pragmatic choice; we're trying
to catch future problems, not give the entire user base a load of
extra work to do on their next pg_upgrade for mostly theoretical
reasons.  (That said, given the new glibc versioning, we'll
effectively be giving most of our user base a load of extra work to do
on their next OS upgrade and that'll be a characteristic of PostgreSQL
going forward, once the versioning-for-default-provider patch goes
in.)  Any other opinions?

> I didn't do anything for the spurious warning when running a reindex,
> and kept original approach of pg_depend catalog.

I see three options:

1.  Change all or some of index_open(), relation_open(),
RelationIdGetRelation(), RelationBuildDesc() and
RelationInitIndexAccessInfo() to take some kind of flag so we can say
NO_COLLATION_VERSION_CHECK_PLEASE, and then have ReindexIndex() pass
that flag down when opening it for the purpose of rebuilding it.
2.  Use a global state to suppress these warnings while opening that
index.  Perhaps ReindexIndex() would call RelCacheWarnings(false)
before index_open(), and use PG_FINALLY to make sure it restores
warnings with RelCacheWarnings(true).  (This is a less-code-churn
bad-programming-style version of #1.)
3.  Move the place that we run the collation version check.  Instead
of doing it in RelationInitIndexAccessInfo() so that it runs when the
relation cache first loads the index, put it into a new routine
RelationCheckValidity() and call that from ... I don't know, some
other place that runs whenever we access indexes but not when we
rebuild them.

3's probably a better idea, if you can find a reasonable place to call
it from.  I'm thinking some time around the time the executor acquires
locks, but using a flag in the relcache entry to make sure it doesn't
run the check again if there were no warnings last time (so one
successful version check turns the extra work off for the rest of this
relcache entry's lifetime).  I think it'd be a feature, not a bug, if
it gave you the warning every single time you executed a query using
an index that has a mismatch...  but a practical alternative would be
to check only once per index and that probably makes more sense.