Re: pg_amcheck contrib application

[Tom Lane]

Noah Misch  writes:
> On Sat, Mar 13, 2021 at 01:36:11AM -0500, Tom Lane wrote:
>> I don't mind updating the perl installations on prairiedog and gaur,
>> but Noah might have some difficulty with his AIX flotilla, as I believe
>> he's not sysadmin there.

> The AIX animals have Perl v5.28.1.  hoverfly, in particular, got a big update
> package less than a month ago.  Hence, I doubt it's a question of applying
> routine updates.  The puzzle would be to either (a) compile a 32-bit Perl that
> handles unpack('q') or (b) try a PostgreSQL configuration like "./configure
> ... PROVE='perl64 /usr/bin/prove --'" to run the TAP suites under perl64.
> (For hoverfly, it's enough to run "prove" under $PERL.  mandrill, however,
> needs a 32-bit $PERL for plperl, regardless of what it needs for "prove".)
> Future AIX packagers would face doing the same.

Yeah.  prairiedog and gaur are both frankenstein systems: some of the
software components are years newer than the underlying OS.  So I don't
mind changing them further; in the end they're both in the buildfarm
for reasons of hardware diversity, not because they represent platforms
anyone would run production PG on.  On the other hand, those AIX animals
represent systems that are still considered production grade (no?), so
we have to be willing to adapt to them not vice versa.

> With v5-0001-pg_amcheck-continuing-to-fix-portability-problems.patch being so
> self-contained, something like it is the way to go.

I'm only concerned about whether the all-zero value causes any
significant degradation in test quality.  Probably Peter G. would
have the most informed opinion about that.

regards, tom lane

Re: pg_amcheck contrib application

[Noah Misch]

On Sat, Mar 13, 2021 at 01:36:11AM -0500, Tom Lane wrote:
> Mark Dilger  writes:
> > On Mar 12, 2021, at 10:22 PM, Tom Lane  wrote:
> >> Coping with both endiannesses might be painful.
> 
> > Not too bad if the bigint value is zero, as both the low and high 32bits 
> > will be zero, regardless of endianness.  The question is whether that gives 
> > up too much in terms of what the test is trying to do.  I'm not sure that 
> > it does, but if you'd rather solve this by upgrading perl, that's ok by me. 
> 
> I don't mind updating the perl installations on prairiedog and gaur,
> but Noah might have some difficulty with his AIX flotilla, as I believe
> he's not sysadmin there.

The AIX animals have Perl v5.28.1.  hoverfly, in particular, got a big update
package less than a month ago.  Hence, I doubt it's a question of applying
routine updates.  The puzzle would be to either (a) compile a 32-bit Perl that
handles unpack('q') or (b) try a PostgreSQL configuration like "./configure
... PROVE='perl64 /usr/bin/prove --'" to run the TAP suites under perl64.
(For hoverfly, it's enough to run "prove" under $PERL.  mandrill, however,
needs a 32-bit $PERL for plperl, regardless of what it needs for "prove".)
Future AIX packagers would face doing the same.

With v5-0001-pg_amcheck-continuing-to-fix-portability-problems.patch being so
self-contained, something like it is the way to go.

Re: pg_amcheck contrib application

[Tom Lane]

Mark Dilger  writes:
> On Mar 12, 2021, at 10:36 PM, Tom Lane  wrote:
>> You might think about using some symmetric-but-not-zero value,
>> 0x01010101 or the like.

> I thought about that, but I'm not sure that it proves much more than just 
> using zero.

Perhaps not.  I haven't really looked at any of this code, so I'll defer
to Robert's judgment about whether this represents an interesting testing
issue.

regards, tom lane

Re: pg_amcheck contrib application

[Mark Dilger]

> On Mar 12, 2021, at 10:36 PM, Tom Lane  wrote:
> 
> Mark Dilger  writes:
>> On Mar 12, 2021, at 10:22 PM, Tom Lane  wrote:
>>> Coping with both endiannesses might be painful.
> 
>> Not too bad if the bigint value is zero, as both the low and high 32bits 
>> will be zero, regardless of endianness.  The question is whether that gives 
>> up too much in terms of what the test is trying to do.  I'm not sure that it 
>> does, but if you'd rather solve this by upgrading perl, that's ok by me. 
> 
> I don't mind updating the perl installations on prairiedog and gaur,
> but Noah might have some difficulty with his AIX flotilla, as I believe
> he's not sysadmin there.
> 
> You might think about using some symmetric-but-not-zero value,
> 0x01010101 or the like.

I thought about that, but I'm not sure that it proves much more than just using 
zero.  The test doesn't really do much of interest with this value, and it 
doesn't seem worth complicating the test.  The idea originally was that perl's 
"q" pack code would make reading/writing a number such as 12345678 easy, but 
since it's not easy, this is easy.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: pg_amcheck contrib application

[Tom Lane]

Mark Dilger  writes:
> On Mar 12, 2021, at 10:22 PM, Tom Lane  wrote:
>> Coping with both endiannesses might be painful.

> Not too bad if the bigint value is zero, as both the low and high 32bits will 
> be zero, regardless of endianness.  The question is whether that gives up too 
> much in terms of what the test is trying to do.  I'm not sure that it does, 
> but if you'd rather solve this by upgrading perl, that's ok by me. 

I don't mind updating the perl installations on prairiedog and gaur,
but Noah might have some difficulty with his AIX flotilla, as I believe
he's not sysadmin there.

You might think about using some symmetric-but-not-zero value,
0x01010101 or the like.

regards, tom lane

Re: pg_amcheck contrib application

[Mark Dilger]

> On Mar 12, 2021, at 10:28 PM, Mark Dilger  
> wrote:
> 
> 
> 
>> On Mar 12, 2021, at 10:22 PM, Tom Lane  wrote:
>> 
>> Mark Dilger  writes:
>>> On Mar 12, 2021, at 10:16 PM, Noah Misch  wrote:
 hoverfly does configure with PERL=perl64.  /usr/bin/prove is from the 
 32-bit
 Perl, so I suspect the TAP suites get 32-bit Perl that way.  (There's no
 "prove64".)
>> 
>> Oh, that's annoying.
>> 
 This test should unpack the field as two 32-bit values, not a
 64-bit value, to avoid requiring more from the Perl installation.
>> 
>>> I will post a modified test in a bit that avoids using Q/q.
>> 
>> Coping with both endiannesses might be painful.
> 
> Not too bad if the bigint value is zero, as both the low and high 32bits will 
> be zero, regardless of endianness.  The question is whether that gives up too 
> much in terms of what the test is trying to do.  I'm not sure that it does, 
> but if you'd rather solve this by upgrading perl, that's ok by me. 


I'm not advocating that this be the solution, but if we don't fix up the perl 
end of it, then this test change might be used instead.



v5-0001-pg_amcheck-continuing-to-fix-portability-problems.patch
Description: Binary data


—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: pgbench: option delaying queries till connections establishment?

[Tom Lane]

Thomas Munro  writes:
> On Sat, Mar 13, 2021 at 4:59 PM Tom Lane  wrote:
>> Looks reasonable by eyeball.  If you'd push it, I can launch
>> a gaur run right away.

> Done.

gaur's gotten through "make" and "make check" cleanly.  Unfortunately
I expect it will fail at the pg_amcheck test before it reaches pgbench.
But for the moment it's reasonable to assume we're good here.  Thanks!

regards, tom lane

Re: pg_amcheck contrib application

[Andres Freund]

Hi,

On 2021-03-13 01:22:54 -0500, Tom Lane wrote:
> Mark Dilger  writes:
> > On Mar 12, 2021, at 10:16 PM, Noah Misch  wrote:
> >> hoverfly does configure with PERL=perl64.  /usr/bin/prove is from the 
> >> 32-bit
> >> Perl, so I suspect the TAP suites get 32-bit Perl that way.  (There's no
> >> "prove64".)
> 
> Oh, that's annoying.

I suspect we could solve that by invoking changing our /usr/bin/prove
invocation to instead be PERL /usr/bin/prove? That might be a good thing
independent of this issue, because it's not unreasonable for a user to
expect that we'd actually use the perl installation they configured...

Although I do not know how prove determines the perl installation it's
going to use for the test scripts...

- Andres

Re: pg_amcheck contrib application

[Mark Dilger]

> On Mar 12, 2021, at 10:22 PM, Tom Lane  wrote:
> 
> Mark Dilger  writes:
>> On Mar 12, 2021, at 10:16 PM, Noah Misch  wrote:
>>> hoverfly does configure with PERL=perl64.  /usr/bin/prove is from the 32-bit
>>> Perl, so I suspect the TAP suites get 32-bit Perl that way.  (There's no
>>> "prove64".)
> 
> Oh, that's annoying.
> 
>>> This test should unpack the field as two 32-bit values, not a
>>> 64-bit value, to avoid requiring more from the Perl installation.
> 
>> I will post a modified test in a bit that avoids using Q/q.
> 
> Coping with both endiannesses might be painful.

Not too bad if the bigint value is zero, as both the low and high 32bits will 
be zero, regardless of endianness.  The question is whether that gives up too 
much in terms of what the test is trying to do.  I'm not sure that it does, but 
if you'd rather solve this by upgrading perl, that's ok by me. 

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: pg_amcheck contrib application

[Tom Lane]

Mark Dilger  writes:
> On Mar 12, 2021, at 10:16 PM, Noah Misch  wrote:
>> hoverfly does configure with PERL=perl64.  /usr/bin/prove is from the 32-bit
>> Perl, so I suspect the TAP suites get 32-bit Perl that way.  (There's no
>> "prove64".)

Oh, that's annoying.

>> This test should unpack the field as two 32-bit values, not a
>> 64-bit value, to avoid requiring more from the Perl installation.

> I will post a modified test in a bit that avoids using Q/q.

Coping with both endiannesses might be painful.

regards, tom lane

Re: shared-memory based stats collector

[Andres Freund]

Hi,

On 2021-03-11 19:22:57 -0800, Andres Freund wrote:
> I started changing the patch to address my complaints. I'll try to do
> it as an incremental patch ontop of your 0004, but it might become too
> unwieldy. Not planning to touch other patches for now (and would be
> happy if the first few were committed).  I do think we'll have to
> split 0004 a bit - it's just too large to commit as is, I think.

Far from being done (and the individual commits is just me working, not
something that is intended to survive), but I thought it might be
helpful to post my WIP git tree:
https://github.com/anarazel/postgres/commits/shmstat

I do think that the removal of the the "attach" logic, as well as the
more explicit naming differentiating between the three different hash
tables is a pretty clear improvement. It's way too easy to get turned
around otherwise.


I suspect to make all of this workable we'll have to add a few
preliminary cleanup patches. I'm currently thinking:

1) There's a bunch of functions in places that don't make sense.


/* 
 * Local support functions follow
 * 
 */

[internal stuff]
...
void
pgstat_send_archiver(const char *xlog, bool failed)
...
void
pgstat_send_bgwriter(void)
...
void
pgstat_report_wal(void)
...
bool
pgstat_send_wal(bool force)
...
[internal stuff]
...
void
pgstat_count_slru_page_zeroed(int slru_idx)

...

I think it'd make sense to separately clean that up.


2) src/backend/postmaster/pgstat.c currently contains at least two,
   effectively independent, subsystems. Arguably more:
   a) cumulative stats collection infrastructure: sending data to the
  persistent stats file, and reading from it.
   b) "content aware" cumulative statistics function

   c) "current" activity infrastructure, around PgBackendStatus (which
  basically boils down to pg_stat_activity et al.)
   d) wait events
   e) command progress stuff

   They don't actually have much to do with each other, except being
   related to stats. Even without the shared memory stats, having these
   be in one file makes pretty much no sense. Having them all under one
   common pgstat_* prefix is endlessly confusing too.

   I think before making things differently complicated with this patch,
   we need to clean this up, unfortunately. I think we should initially have
   - src/backend/postmaster/pgstat.c, for a), b) above
   - src/backend/utils/activity/backend_status.c for c)
   - src/backend/utils/activity/wait_events.c for d)
   - src/backend/utils/activity/progress.c for e)

   Not at all sure about the names, but something roughly like this
   would im make sense.


The next thing to note is that after this whole patchseries, having the
remaining functionality in src/backend/postmaster/pgstat.c doesn't make
sense. The things in postmaster/ are related to postmaster
sub-processes, not random pieces of backend infrastructure. Therefore I
am thinking that patch 0004 should be changed so it basically adds all
the changed code to two new files:
- src/backend/utils/activity/stats.c - for the stats keeping
  infrastructure (shared memory hash table, pending table, etc)
- src/backend/utils/activity/stats_report.c - for pgstat_report_*,
  pgstat_count_*, pgstat_update_*, flush_*, i.e. everything that knows
  about specific kinds of stats.

The reason for that split is that imo the two pieces of code are largely
independent. One shouldn't need to understand the way stats are stored
in any sort of detail to add a new stats field and vice versa.


Horiguchi-san, is there a chance you could add a few tests (on master)
that test/document the way stats are kept across "normal" restarts, and
thrown away after crash restarts/immediate restarts and also thrown away
graceful streaming rep shutdowns?


Greetings,

Andres Freund

Re: pg_amcheck contrib application

[Mark Dilger]

> On Mar 12, 2021, at 10:16 PM, Noah Misch  wrote:
> 
> On Sat, Mar 13, 2021 at 01:07:15AM -0500, Tom Lane wrote:
>> I wrote:
 ... btw, prairiedog (which has a rather old Perl) has a
 different complaint:
 Invalid type 'q' in unpack at t/004_verify_heapam.pl line 104.
>> 
>>> Hmm ... "man perlfunc" on that system quoth
>>>   q   A signed quad (64-bit) value.
>>>   Q   An unsigned quad value.
>>> (Quads are available only if your system supports 
>>> 64-bit
>>>  integer values _and_ if Perl has been compiled to 
>>> support those.
>>>  Causes a fatal error otherwise.)
>>> It does not seem unreasonable for us to rely on Perl having that
>>> in 2021, so I'll see about upgrading this perl installation.
>> 
>> Hm, wait a minute: hoverfly is showing the same failure, even though
>> it claims to be running a 64-bit Perl.  Now I'm confused.
> 
> On that machine:
> 
> [nm@power8-aix 7:0 2021-03-13T06:09:08 ~ 0]$ /usr/bin/perl64 -e 'unpack "q", 
> ""'
> [nm@power8-aix 7:0 2021-03-13T06:09:10 ~ 0]$ /usr/bin/perl -e 'unpack "q", ""'
> Invalid type 'q' in unpack at -e line 1.
> 
> hoverfly does configure with PERL=perl64.  /usr/bin/prove is from the 32-bit
> Perl, so I suspect the TAP suites get 32-bit Perl that way.  (There's no
> "prove64".)  This test should unpack the field as two 32-bit values, not a
> 64-bit value, to avoid requiring more from the Perl installation.

I will post a modified test in a bit that avoids using Q/q.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: pg_amcheck contrib application

[Noah Misch]

On Sat, Mar 13, 2021 at 01:07:15AM -0500, Tom Lane wrote:
> I wrote:
> >> ... btw, prairiedog (which has a rather old Perl) has a
> >> different complaint:
> >> Invalid type 'q' in unpack at t/004_verify_heapam.pl line 104.
> 
> > Hmm ... "man perlfunc" on that system quoth
> >q   A signed quad (64-bit) value.
> >Q   An unsigned quad value.
> >  (Quads are available only if your system supports 
> > 64-bit
> >   integer values _and_ if Perl has been compiled to 
> > support those.
> >   Causes a fatal error otherwise.)
> > It does not seem unreasonable for us to rely on Perl having that
> > in 2021, so I'll see about upgrading this perl installation.
> 
> Hm, wait a minute: hoverfly is showing the same failure, even though
> it claims to be running a 64-bit Perl.  Now I'm confused.

On that machine:

[nm@power8-aix 7:0 2021-03-13T06:09:08 ~ 0]$ /usr/bin/perl64 -e 'unpack "q", ""'
[nm@power8-aix 7:0 2021-03-13T06:09:10 ~ 0]$ /usr/bin/perl -e 'unpack "q", ""'
Invalid type 'q' in unpack at -e line 1.

hoverfly does configure with PERL=perl64.  /usr/bin/prove is from the 32-bit
Perl, so I suspect the TAP suites get 32-bit Perl that way.  (There's no
"prove64".)  This test should unpack the field as two 32-bit values, not a
64-bit value, to avoid requiring more from the Perl installation.

Re: pg_amcheck contrib application

[Tom Lane]

I wrote:
>> ... btw, prairiedog (which has a rather old Perl) has a
>> different complaint:
>> Invalid type 'q' in unpack at t/004_verify_heapam.pl line 104.

> Hmm ... "man perlfunc" on that system quoth
>q   A signed quad (64-bit) value.
>Q   An unsigned quad value.
>  (Quads are available only if your system supports 
> 64-bit
>   integer values _and_ if Perl has been compiled to 
> support those.
>   Causes a fatal error otherwise.)
> It does not seem unreasonable for us to rely on Perl having that
> in 2021, so I'll see about upgrading this perl installation.

Hm, wait a minute: hoverfly is showing the same failure, even though
it claims to be running a 64-bit Perl.  Now I'm confused.

regards, tom lane

Re: proposal: schema variables

[Pavel Stehule]

Hi

fresh rebase

Pavel


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

Re: New IndexAM API controlling index vacuum strategies

[Masahiko Sawada]

On Tue, Mar 9, 2021 at 2:22 PM Peter Geoghegan  wrote:
>
> On Tue, Mar 2, 2021 at 8:49 PM Masahiko Sawada  wrote:
> > On Tue, Mar 2, 2021 at 2:34 PM Peter Geoghegan  wrote:
> > > lazy_vacuum_table_and_indexes() should probably not skip index
> > > vacuuming when we're close to exceeding the space allocated for the
> > > LVDeadTuples array. Maybe we should not skip when
> > > vacrelstats->dead_tuples->num_tuples is greater than 50% of
> > > dead_tuples->max_tuples? Of course, this would only need to be
> > > considered when lazy_vacuum_table_and_indexes() is only called once
> > > for the entire VACUUM operation (otherwise we have far too little
> > > maintenance_work_mem/dead_tuples->max_tuples anyway).
> >
> > Doesn't it actually mean we consider how many dead *tuples* we
> > collected during a vacuum? I’m not sure how important the fact we’re
> > close to exceeding the maintenance_work_mem space. Suppose
> > maintenance_work_mem is 64MB, we will not skip both index vacuum and
> > heap vacuum if the number of dead tuples exceeds 5592404 (we can
> > collect 11184809 tuples with 64MB memory). But those tuples could be
> > concentrated in a small number of blocks, for example in a very large
> > table case. It seems to contradict the current strategy that we want
> > to skip vacuum if relatively few blocks are modified. No?
>
> There are competing considerations. I think that we need to be
> sensitive to accumulating "debt" here. The cost of index vacuuming
> grows in a non-linear fashion as the index grows (or as
> maintenance_work_mem is lowered). This is the kind of thing that we
> should try to avoid, I think. I suspect that cases where we can skip
> index vacuuming and heap vacuuming are likely to involve very few dead
> tuples in most cases anyway.
>
> We should not be sensitive to the absolute number of dead tuples when
> it doesn't matter (say because they're concentrated in relatively few
> heap pages). But when we overrun the maintenance_work_mem space, then
> the situation changes; the number of dead tuples clearly matters just
> because we run out of space for the TID array. The heap page level
> skew is not really important once that happens.
>
> That said, maybe there is a better algorithm. 50% was a pretty arbitrary 
> number.

I agreed that when we're close to overrunning the
maintnenance_work_mem space, the situation changes. If we skip it in
even that case, the next vacuum will be likely to use up
maintenance_work_mem, leading to a second index scan. Which is
bad.

If this threshold is aimed to avoid a second index scan due to
overrunning the maintenance_work_mem, using a ratio of
maintenance_work_mem would be a good idea. On the other hand, if it's
to avoid accumulating debt affecting the cost of index vacuuming,
using a ratio of the total heap tuples seems better.

The situation where we need to deal with here is a very large table
that has a lot of dead tuples but those fit in fewer heap pages (less
than 1% of all heap blocks). In this case, it's likely that the number
of dead tuples also is relatively small compared to the total heap
tuples, as you mentioned. If dead tuples fitted in fewer pages but
accounted for most of all heap tuples in the heap, it would be a more
serious situation, there would definitely already be other problems.
So considering those conditions, I agreed to use a ratio of
maintenance_work_mem as a threshold. Maybe we can increase the
constant to 70, 80, or so.

>
> Have you thought more about how the index vacuuming skipping can be
> configured by users? Maybe a new storage param, that works like the
> current SKIP_VACUUM_PAGES_RATIO constant?

Since it’s unclear to me yet that adding a new storage parameter or
GUC parameter for this feature would be useful even for future
improvements in this area, I haven't thought yet about having users
control skipping index vacuuming. I’m okay with a constant value for
the threshold for now.



Regards,

--
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

Re: pg_amcheck contrib application

[Tom Lane]

Mark Dilger  writes:
> Sorry for painting so many farm animals red this evening.

Not to worry.  We go through this sort of fire drill regularly
when somebody pushes a batch of brand new test code.

regards, tom lane

Re: pg_amcheck contrib application

[Tom Lane]

I wrote:
> ... btw, prairiedog (which has a rather old Perl) has a
> different complaint:
> Invalid type 'q' in unpack at t/004_verify_heapam.pl line 104.

Hmm ... "man perlfunc" on that system quoth

   q   A signed quad (64-bit) value.
   Q   An unsigned quad value.
 (Quads are available only if your system supports 
64-bit
  integer values _and_ if Perl has been compiled to 
support those.
  Causes a fatal error otherwise.)

It does not seem unreasonable for us to rely on Perl having that
in 2021, so I'll see about upgrading this perl installation.

(I suppose gaur will need it too, sigh.)

regards, tom lane

Re: Parallel INSERT (INTO ... SELECT ...)

[Amit Kapila]

On Sat, Mar 13, 2021 at 10:08 AM Tom Lane  wrote:
>
> Greg Nancarrow  writes:
> > On Fri, Mar 12, 2021 at 5:00 AM Tom Lane  wrote:
> >> BTW, having special logic for FK triggers in
> >> target_rel_trigger_max_parallel_hazard seems quite loony to me.
> >> Why isn't that handled by setting appropriate proparallel values
> >> for those trigger functions?
>
> > ... and also attached a patch to update the code for this issue.
>
> Hm, what I was expecting to see is that RI_FKey_check_ins and
> RI_FKey_check_upd get marked as proparallel = 'r' and the remainder
> get marked as proparallel = 'u'.
>

oh, I think Greg's patch has just marked functions for which the
current code has parallel-safety checks and I also thought that would
be sufficient.

>  Remember that the default for
> builtin functions is proparallel = 's', but surely that's wrong
> for triggers that can propagate updates to other tables?
>

Okay, probably the others can be marked as unsafe. I think we have not
considered others except for FK-related triggers which we knew are
hazardous for enabling inserts in parallel-mode. The others seem to be
related to update/delete, so we have not done anything, but maybe it
is better to mark them as 'unsafe' now, and later when we support the
update/delete in parallel-mode, we can see if any of those can be
executed in parallel-mode. But OTOH, we can keep them as it is if they
don't impact the current operation we have supported in parallel-mode.

-- 
With Regards,
Amit Kapila.

Re: pg_amcheck contrib application

[Mark Dilger]

> On Mar 12, 2021, at 9:08 PM, Tom Lane  wrote:
> 
> I wrote:
>> Don't almost all of the following tests have the same issue?
> 
> Ah, nevermind, I was looking at an older version of 003_check.pl.
> I concur that 24189277f missed only one here.
> 
> Pushed your fix.
> 
>   regards, tom lane

Thanks!  Was just responding to your other email, but now I don't have to send 
it.

Sorry for painting so many farm animals red this evening.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: pg_amcheck contrib application

[Tom Lane]

I wrote:
> Don't almost all of the following tests have the same issue?

Ah, nevermind, I was looking at an older version of 003_check.pl.
I concur that 24189277f missed only one here.

Pushed your fix.

regards, tom lane

Re: pg_amcheck contrib application

[Tom Lane]

... btw, prairiedog (which has a rather old Perl) has a
different complaint:

Invalid type 'q' in unpack at t/004_verify_heapam.pl line 104.

regards, tom lane

Re: pg_amcheck contrib application

[Tom Lane]

Mark Dilger  writes:
> There is another problem of non-portable option ordering in the tests.

Don't almost all of the following tests have the same issue?

regards, tom lane

Re: pg_amcheck contrib application

[Mark Dilger]

> On Mar 12, 2021, at 5:16 PM, Robert Haas  wrote:
> 
> Gah, tests are so annoying. :-)

There is another problem of non-portable option ordering in the tests.



v4-0001-pg_amcheck-Keep-trying-to-fix-the-tests.patch
Description: Binary data


—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Parallel INSERT (INTO ... SELECT ...)

[Tom Lane]

Greg Nancarrow  writes:
> On Fri, Mar 12, 2021 at 5:00 AM Tom Lane  wrote:
>> BTW, having special logic for FK triggers in
>> target_rel_trigger_max_parallel_hazard seems quite loony to me.
>> Why isn't that handled by setting appropriate proparallel values
>> for those trigger functions?

> ... and also attached a patch to update the code for this issue.

Hm, what I was expecting to see is that RI_FKey_check_ins and
RI_FKey_check_upd get marked as proparallel = 'r' and the remainder
get marked as proparallel = 'u'.  Remember that the default for
builtin functions is proparallel = 's', but surely that's wrong
for triggers that can propagate updates to other tables?

regards, tom lane

Re: [HACKERS] PATCH: Batch/pipelining support for libpq

[Alvaro Herrera]

On 2021-Mar-11, Tom Lane wrote:

> I think the changes in pqParseInput3() are broken.  You should have
> kept the else-structure as-is and inserted the check for "not really
> idle" inside the else-clause that reports an error.  As it stands,
> after successfully processing an asynchronously-received error or
> ParameterStatus message, the added code will cause us to return without
> advancing inStart, creating an infinite loop of reprocessing that message.
> 
> It's possible that we should redefine the way things happen so that if
> we're waiting for another pipeline event, we should hold off processing
> of async error & ParameterStatus; but in that case you should have added
> the pre-emptive return ahead of that if/else structure, where the existing
> "If not IDLE state, just wait ..." test is.

I think I agree that holding off 'E' and 'S' messages when in between
processing results for different queries in a pipeline, so keeping the
original if/else structure is correct.  An error would be correctly
dealt with in the BUSY state immediately afterwards; and the fact that
we pass 'inError' false at that point causes the wrong reaction (namely
that the pipeline is not put in aborted state).

I made a number of other changes: documentation adjustments per comments
from David Johnston, some function renaming as previously noted, and
added test code for PQsendDescribePrepared, PQsendDescribePortal.  Also
rebased on latest changes.  I also absorbed one change that I already
had when I submitted v35, but hadn't done "git add" on (which caused a
compile failure for CF bot).

-- 
Álvaro Herrera   Valdivia, Chile
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 910e9a81ea..7b938c106c 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -3180,6 +3180,33 @@ ExecStatusType PQresultStatus(const PGresult *res);

   
  
+
+ 
+  PGRES_PIPELINE_SYNC
+  
+   
+The PGresult represents a
+synchronization point in pipeline mode, requested by
+.
+This status occurs only when pipeline mode has been selected.
+   
+  
+ 
+
+ 
+  PGRES_PIPELINE_ABORTED
+  
+   
+The PGresult represents a pipeline that has
+received an error from the server.  PQgetResult
+must be called repeatedly, and each time it will return this status code
+until the end of the current pipeline, at which point it will return
+PGRES_PIPELINE_SYNC and normal processing can
+resume.
+   
+  
+ 
+
 
 
 If the result status is PGRES_TUPLES_OK or
@@ -4926,6 +4953,479 @@ int PQflush(PGconn *conn);
 
  
 
+ 
+  Pipeline Mode
+
+  
+   libpq
+   pipeline mode
+  
+
+  
+   pipelining
+   in libpq
+  
+
+  
+   batch mode
+   in libpq
+  
+
+  
+   libpq pipeline mode allows applications to
+   send a query without having to read the result of the previously
+   sent query.  Taking advantage of the pipeline mode, a client will wait
+   less for the server, since multiple queries/results can be
+   sent/received in a single network transaction.
+  
+
+  
+   While pipeline mode provides a significant performance boost, writing
+   clients using the pipeline mode is more complex because it involves
+   managing a queue of pending queries and finding which result
+   corresponds to which query in the queue.
+  
+
+  
+   Pipeline mode also generally consumes more memory on both the client and server,
+   though careful and aggressive management of the send/receive queue can mitigate
+   this.  This applies whether or not the connection is in blocking or non-blocking
+   mode.
+  
+
+  
+   While the pipeline API was introduced in
+   PostgreSQL 14, it is a client-side feature
+   which doesn't require special server support, and works on any server
+   that supports the v3 extended query protocol.
+  
+
+  
+   Using Pipeline Mode
+
+   
+To issue pipelines, the application must switch the connection
+into pipeline mode,
+which is done with .
+ can be used
+to test whether pipeline mode is active.
+In pipeline mode, only asynchronous operations
+are permitted, and COPY is disallowed.
+Using synchronous command execution functions
+such as PQfn,
+PQexec,
+PQexecParams,
+PQprepare,
+PQexecPrepared,
+PQdescribePrepared,
+PQdescribePortal,
+is an error condition.
+Once all dispatched commands have had their results processed, and
+the end pipeline result has been consumed, the application may return
+to non-pipelined mode with .
+   
+
+   
+
+ It is best to use pipeline mode with libpq in
+ non-blocking mode. If used
+ in blocking mode it is possible for a client/server deadlock to occur.
+  
+   
+The client will block trying to send queries to the server, 

Re: pgbench: option delaying queries till connections establishment?

[Thomas Munro]

On Sat, Mar 13, 2021 at 4:59 PM Tom Lane  wrote:
> Thomas Munro  writes:
> > Thanks.  This seems to work for me on a Mac.  I confirmed with nm that
> > we don't define or reference any pthread_XXX symbols with
> > --disable-thread-safety, and we do otherwise, and the pgbench tests
> > pass either way.
>
> Looks reasonable by eyeball.  If you'd push it, I can launch
> a gaur run right away.

Done.

Re: pgbench: option delaying queries till connections establishment?

[Tom Lane]

Thomas Munro  writes:
> Thanks.  This seems to work for me on a Mac.  I confirmed with nm that
> we don't define or reference any pthread_XXX symbols with
> --disable-thread-safety, and we do otherwise, and the pgbench tests
> pass either way.

Looks reasonable by eyeball.  If you'd push it, I can launch
a gaur run right away.

regards, tom lane

Re: pgbench: option delaying queries till connections establishment?

[Thomas Munro]

On Sat, Mar 13, 2021 at 4:09 PM Tom Lane  wrote:
> OK, cool.  I don't think it's hard, just do
>
> if test "$enable_thread_safety" = yes; then
>   AC_REPLACE_FUNCS(pthread_barrier_wait)
> fi
>
> Probably this check should be likewise conditional:
>
> AC_SEARCH_LIBS(pthread_barrier_wait, pthread)

Thanks.  This seems to work for me on a Mac.  I confirmed with nm that
we don't define or reference any pthread_XXX symbols with
--disable-thread-safety, and we do otherwise, and the pgbench tests
pass either way.


0001-Fix-new-pthread-code-to-respect-disable-thread-safet.patch
Description: Binary data

Re: TRUNCATE on foreign table

[Kazutaka Onishi]

To Ibrar,
Thank you for updating the patch!

To Amit,
Thank you for checking the patch, and I have confirmed the failure.
Now I'm trying to fix it.



2021年3月9日(火) 11:54 Amit Langote :

> On Tue, Mar 9, 2021 at 2:24 AM Ibrar Ahmed  wrote:
> > The patch (pgsql14-truncate-on-foreign-table.v2.patch) does not apply
> successfully.
> >
> > http://cfbot.cputube.org/patch_32_2972.log
> >
> > patching file contrib/postgres_fdw/expected/postgres_fdw.out
> > Hunk #2 FAILED at 9179.
> > 1 out of 2 hunks FAILED -- saving rejects to file
> contrib/postgres_fdw/expected/postgres_fdw.out.rej
> >
> > As this is a minor change therefore I have updated the patch. Please
> take a look.
>
> Thanks for updating the patch.  I was able to apply it successfully
> though I notice it doesn't pass make check-world.
>
> Specifically, it fails the src/test/subscription/013_partition.pl
> test.  The problem seems to be that worker.c: apply_handle_truncate()
> hasn't been updated to add entries to relids_extra for partitions
> expanded from a partitioned table, like ExecuteTruncate() does.  That
> leads to relids and relids_extra having different lengths, which trips
> the Assert in ExecuteTruncateGuts().
>
> --
> Amit Langote
> EDB: http://www.enterprisedb.com
>

Re: pgbench: option delaying queries till connections establishment?

[Tom Lane]

Thomas Munro  writes:
> On Sat, Mar 13, 2021 at 3:47 PM Tom Lane  wrote:
>> Was any thought given to being able to opt out of this patchset
>> to support that configure option?

> Oops.  The pgbench code was tested under --disable-thread-safety, but
> it didn't occur to me that the AC_REPLACE_FUNCS search for
> pthread_barrier_wait should also be conditional on that; I will now go
> and try to figure out how to do that.

OK, cool.  I don't think it's hard, just do

if test "$enable_thread_safety" = yes; then
  AC_REPLACE_FUNCS(pthread_barrier_wait)
fi

Probably this check should be likewise conditional:

AC_SEARCH_LIBS(pthread_barrier_wait, pthread)

regards, tom lane

Re: pgbench: option delaying queries till connections establishment?

[Thomas Munro]

On Sat, Mar 13, 2021 at 3:47 PM Tom Lane  wrote:
> Checking the man pages, it seems that this ancient HPUX version
> is using some pre-POSIX API spec in which pthread_cond_init takes a
> pthread_condattr_t rather than a pointer to pthread_condattr_t.
> Similarly for pthread_mutex_init.

Wow.

> While it's likely that we could work around that, it's my
> opinion that we shouldn't have to, because gaur is building with
> --disable-thread-safety.  If that switch has any meaning at all,
> it should be that we don't try to use thread infrastructure.
> Was any thought given to being able to opt out of this patchset
> to support that configure option?

Oops.  The pgbench code was tested under --disable-thread-safety, but
it didn't occur to me that the AC_REPLACE_FUNCS search for
pthread_barrier_wait should also be conditional on that; I will now go
and try to figure out how to do that.

Re: Parallel INSERT (INTO ... SELECT ...)

[Amit Kapila]

On Fri, Mar 12, 2021 at 9:33 AM houzj.f...@fujitsu.com
 wrote:
>
> > On Thu, Mar 11, 2021 at 01:01:42PM +, houzj.f...@fujitsu.com wrote:
> > > > I guess to have the finer granularity we'd have to go with
> > > > enable_parallel_insert, which then would mean possibly having to
> > > > later add enable_parallel_update, should parallel update have
> > > > similar potential overhead in the parallel-safety checks (which to
> > > > me, looks like it could, and parallel delete may not ...)
> > > >
> > > > It's a shame there is no "set" type for GUC options.
> > > > e.g.
> > > > enable_parallel_dml='insert,update'
> > > > Maybe that's going too far.
> >
> > Isn't that just GUC_LIST_INPUT ?
> > I'm not sure why it'd be going to far ?
> >
> > The GUC-setting assign hook can parse the enable_parallel_dml_list value set
> > by the user, and set an internal int/bits enable_parallel_dml variable with 
> > some
> > define/enum values like:
> >
> > GUC_PARALLEL_DML_INSERT 0x01
> > GUC_PARALLEL_DML_DELETE 0x02
> > GUC_PARALLEL_DML_UPDATE 0x04
> >
>
> I think this ideas works, but we still need to consider about the reloption.
> After looking into the reloption, I think postgres do not have a list-like 
> type for reloption.
> And I think it's better that the guc and reloption is consistent.
>

I also think it is better to be consistent here.

> Besides, a list type guc option that only support one valid value 'insert' 
> seems a little weird to me(we only support parallel insert for now).
>
> So, I tend to keep the current style of guc option.
>

+1. I feel at this stage it might not be prudent to predict the
overhead for parallel updates or deletes especially when there doesn't
appear to be an easy way to provide a futuristic guc/reloption and we
don't have any patch on the table which can prove or disprove that
theory. The only thing that we can see that even if parallel
updates/deletes have overhead, it might not be due to similar reasons.
Also, I guess the parallel-copy might need somewhat similar
parallel-safety checking w.r.t partitioned tables and I feel the
current proposed guc/reloption can be used for the same as it is quite
a similar operation.

-- 
With Regards,
Amit Kapila.

Re: A qsort template

[Thomas Munro]

On Fri, Mar 12, 2021 at 7:58 AM Andres Freund  wrote:
> I wish we had the same for bsearch... :)

Glibc already has the definition of the traditional void-based
function in /usr/include/bits/stdlib-bsearch.h, so the generated code
when the compiler can see the comparator definition is already good in
eg lazy_tid_reaped() and eg some nbtree search routines.  We could
probably expose more trivial comparators in headers to get more of
that, and we could perhaps put our own bsearch definition in a header
for other platforms that didn't think of that...

It might be worth doing type-safe macro templates as well, though (as
I already did in an earlier proposal[1]), just to have nice type safe
code though, not sure, I'm thinking about that...

[1] 
https://www.postgresql.org/message-id/flat/CA%2BhUKGLY47Cvu62mFDT53Ya0P95cGggcBN6R6aLpx6%3DGm5j%2B1A%40mail.gmail.com

Re: pgbench: option delaying queries till connections establishment?

[Tom Lane]

Thomas Munro  writes:
> On Mon, Mar 8, 2021 at 3:18 PM Thomas Munro  wrote:
>> David Rowley kindly tested this for me on Windows and told me how to
>> fix one of the macros that had incorrect error checking on that OS.
>> So here's a new version.  I'm planning to commit 0001 and 0002 soon,
>> if there are no objections.  0003 needs some more review.

> I made a few mostly cosmetic changes, pgindented and pushed all these patches.

So, gaur is not too happy with this:

ccache gcc -std=gnu99 -Wall -Wmissing-prototypes -Wpointer-arith 
-Wdeclaration-after-statement -Werror=vla -Wendif-labels 
-Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv 
-fexcess-precision=standard -g -O2 -I../../src/port -DFRONTEND 
-I../../src/include  -D_USE_CTYPE_MACROS -D_XOPEN_SOURCE_EXTENDED  
-I/usr/local/libxml2-2.6.23/include/libxml2 -I/usr/local/ssl-1.0.1e/include  -c 
-o strlcat.o strlcat.c
pthread_barrier_wait.c: In function 'pthread_barrier_init':
pthread_barrier_wait.c:24:2: error: incompatible type for argument 2 of 
'pthread_cond_init'
/usr/include/pthread.h:378:5: note: expected 'pthread_condattr_t' but argument 
is of type 'void *'
pthread_barrier_wait.c:26:2: error: incompatible type for argument 2 of 
'pthread_mutex_init'
/usr/include/pthread.h:354:5: note: expected 'pthread_mutexattr_t' but argument 
is of type 'void *'
make[2]: *** [pthread_barrier_wait.o] Error 1

Checking the man pages, it seems that this ancient HPUX version
is using some pre-POSIX API spec in which pthread_cond_init takes a
pthread_condattr_t rather than a pointer to pthread_condattr_t.
Similarly for pthread_mutex_init.

While it's likely that we could work around that, it's my
opinion that we shouldn't have to, because gaur is building with
--disable-thread-safety.  If that switch has any meaning at all,
it should be that we don't try to use thread infrastructure.
Was any thought given to being able to opt out of this patchset
to support that configure option?

regards, tom lane

Re: EXPLAIN/EXPLAIN ANALYZE REFRESH MATERIALIZED VIEW

[Japin Li]

On Mon, 08 Mar 2021 at 12:28, Bharath Rupireddy 
 wrote:
> On Sun, Mar 7, 2021 at 10:13 PM Zhihong Yu  wrote:
>> Hi,
>>
>> +* EXPLAIN ANALYZE CREATE TABLE AS or REFRESH MATERIALIZED VIEW
>> +* WITH NO DATA is weird.
>>
>> Maybe it is clearer to spell out WITH NO DATA for both statements, instead 
>> of sharing it.
>
> Done that way.
>
>> -   if (!stmt->skipData)
>> +   if (!stmt->skipData && !explainInfo)
>> ...
>> +   else if (explainInfo)
>>
>> It would be cleaner to put the 'if (explainInfo)' as the first check. That 
>> way, the check for skipData can be simplified.
>
> Changed.
>
> Thanks for review comments. Attaching v7 patch set with changes only
> in 0002 patch. Please have a look.
>

The v7 patch looks good to me, and there is no other advice, so I change
the status to "Ready for Committer".

-- 
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

Re: Different compression methods for FPI

[Justin Pryzby]

On Fri, Mar 12, 2021 at 01:45:47AM -0600, Justin Pryzby wrote:
> On Sat, Mar 06, 2021 at 12:29:14PM +0500, Andrey Borodin wrote:
> > > 1 марта 2021 г., в 10:03, Justin Pryzby  написал(а):
> > 
> > Justin, Michael, thanks for comments!
> > 
> > As far as I understood TODO list for the patch looks as follows:
> 
> Your patch can be simplified some, and then the only ifdef are in two short
> functions.  Moving the compression calls to another function/file is hardly
> worth it, and anyone that implements a generic compression API could refactor
> easily, if it's a win.  So I don't want to impose the burden on your small
> patch of setting up the compression API for everyone else's patches.  Since
> this is non-streaming compression, the calls are trivial.
> 
> One advantage of a generic API is that it's a good place to handle things like
> compression options, like zstd:9 or zstd:3,rsyncable (I am not suggesting this
> syntax).
> 
> Today, I re-sent an Dillip's patch with a change to use pkg-config for liblz4,
> and it now also compiles on mac, so I used those changes to configure.ac 
> (using
> pkg-config) and src/tools/msvc/Solution.pm, and changed HAVE_LIBLZ4 to 
> USE_LZ4.

Updated patch with a minor fix to configure.ac to avoid warnings on OSX.
And 2ndary patches from another thread to allow passing recovery tests.
Renamed to WAL_COMPRESSION_*
Split LZ4 support to a separate patch and support zstd.  These show that
changes needed for a new compression method have been minimized, although not
yet moved to a separate, abstracted compression/decompression function.

On Mon, Mar 01, 2021 at 01:57:12PM +0900, Michael Paquier wrote:
> > Your patch looks fine, but I wonder if we should first implement a generic
> > compression API.  Then, the callers don't need to have a bunch of #ifdef.
> > If someone calls zs_create() for an algorithm which isn't enabled at compile
> > time, it throws the error at a lower level.
> 
> Yeah, the patch feels incomplete with its footprint in xloginsert.c
> for something that could be available in src/common/ like pglz,
> particularly knowing that you will need to have this information 
> available to frontend tools, no?

Michael: what frontend tools did you mean ?
pg_rewind?  This may actually be okay as-is, since it uses symlinks:

$ ls -l src/bin/pg_rewind/xlogreader.c
lrwxrwxrwx 1 pryzbyj pryzbyj 48 Mar 12 17:48 src/bin/pg_rewind/xlogreader.c -> 
../../../src/backend/access/transam/xlogreader.c

> Not much a fan either of assuming that it is just fine to add one byte
> to XLogRecordBlockImageHeader for the compression_method field.

What do you mean?  Are you concerned about alignment, or the extra width, or??

These two patches are a prerequisite for this patch to progress:
 * Run 011_crash_recovery.pl with wal_level=minimal
 * Make sure published XIDs are persistent

I don't know if anyone will consider this patch for v14 - if not, it should be
set to v15 and revisit in a month.  

-- 
Justin
>From ac1cf5ed6a22aade34424d6461bbb83ffaba28ba Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi 
Date: Mon, 8 Mar 2021 15:32:30 +0900
Subject: [PATCH 1/8] Run 011_crash_recovery.pl with wal_level=minimal

The test doesn't need that feature and pg_current_xact_id() is better
exercised by turning off the feature.

Copied from: https://www.postgresql.org/message-id/20210308.173242.463790587797836129.horikyota.ntt%40gmail.com
---
 src/test/recovery/t/011_crash_recovery.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/test/recovery/t/011_crash_recovery.pl b/src/test/recovery/t/011_crash_recovery.pl
index 10cd98f70a..690655dda2 100644
--- a/src/test/recovery/t/011_crash_recovery.pl
+++ b/src/test/recovery/t/011_crash_recovery.pl
@@ -11,7 +11,7 @@ use Config;
 plan tests => 3;
 
 my $node = get_new_node('primary');
-$node->init(allows_streaming => 1);
+$node->init();
 $node->start;
 
 my ($stdin, $stdout, $stderr) = ('', '', '');
-- 
2.17.0

>From b6021740e9a593db2d57db7c394c66eb4cf0253f Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi 
Date: Mon, 8 Mar 2021 15:43:01 +0900
Subject: [PATCH 2/8] Make sure published XIDs are persistent

pg_xact_status() premises that XIDs obtained by
pg_current_xact_id(_if_assigned)() are persistent beyond a crash. But
XIDs are not guaranteed to go beyond WAL buffers before commit and
thus XIDs may vanish if server crashes before commit. This patch
guarantees the XID shown by the functions to be flushed out to disk.

Copied from: https://www.postgresql.org/message-id/20210308.173242.463790587797836129.horikyota.ntt%40gmail.com
---
 src/backend/access/transam/xact.c | 55 +--
 src/backend/access/transam/xlog.c |  2 +-
 src/backend/utils/adt/xid8funcs.c | 12 ++-
 src/include/access/xact.h |  3 +-
 4 files changed, 59 insertions(+), 13 deletions(-)

diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 6395a9b240..38e978d238 100644
--- 

Re: pg_amcheck contrib application

[Robert Haas]

On Fri, Mar 12, 2021 at 8:04 PM Mark Dilger
 wrote:
> The problem with IPC::Run appears to be real, though I might just need to 
> wait longer for the farm animals to prove me wrong about that.  But there is 
> a similar symptom caused by an unrelated problem, one entirely my fault and 
> spotted by Robert.  Here is a patch:

OK, I committed this too, along with the one I hadn't committed yet
from your previous email. Gah, tests are so annoying. :-)

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: pg_amcheck contrib application

[Mark Dilger]

> On Mar 12, 2021, at 3:24 PM, Mark Dilger  wrote:
> 
> and the second deals with an apparent problem with IPC::Run shell expanding 
> an asterisk on some platforms but not others.  That second one, if true, 
> seems like a problem with scope beyond the pg_amcheck project, as 
> TestLib::command_checks_all uses IPC::Run, and it would be desirable to have 
> consistent behavior across platforms.

The problem with IPC::Run appears to be real, though I might just need to wait 
longer for the farm animals to prove me wrong about that.  But there is a 
similar symptom caused by an unrelated problem, one entirely my fault and 
spotted by Robert.  Here is a patch:



v3-0001-Avoid-use-of-non-portable-option-ordering-in-comm.patch
Description: Binary data

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Reduce timing overhead of EXPLAIN ANALYZE using rdtsc?

[Thomas Munro]

On Sat, Jun 13, 2020 at 11:28 AM Andres Freund  wrote:
> [PATCH v1 1/2] WIP: Change instr_time to just store nanoseconds, that's 
> cheaper.

Makes a lot of sense.  If we do this, I'll need to update pgbench,
which just did something similar locally.  If I'd been paying
attention to this thread I might not have committed that piece of the
recent pgbench changes, but it's trivial stuff and I'll be happy to
tidy that up when the time comes.

> [PATCH v1 2/2] WIP: Use cpu reference cycles, via rdtsc, to measure time for 
> instrumentation.

> Some of the time is spent doing function calls, dividing into struct
> timespec, etc. But most of it just the rdtscp instruction:
>  65.30 │1  63:   rdtscp

> The reason for that is largely that rdtscp waits until all prior
> instructions have finished (but it allows later instructions to already
> start). Multiple times for each tuple.

Yeah, after reading a bit about this, I agree that there is no reason
to think that the stalling version makes the answer better in any way.
It might make sense if you use it once at the beginning of a large
computation, but it makes no sense if you sprinkle it around inside
blocks that will run multiple times.  It destroys your
instructions-per-cycle while, turning your fancy super scalar Pentium
into a 486.  It does raise some interesting questions about what
exactly you're measuring, though: I don't know enough to have a good
grip on how far out of order the TSC could be read!

> There's also other issues with using rdtsc directly: On older CPUs, in
> particular older multi-socket systems, the tsc will not be synchronized
> in detail across cores. There's bits that'd let us check whether tsc is
> suitable or not.  The more current issue of that is that things like
> virtual machines being migrated can lead to rdtsc suddenly returning a
> different value / the frequency differening. But that is supposed to be
> solved these days, by having virtualization technologies set frequency
> multipliers and offsets which then cause rdtsc[p] to return something
> meaningful, even after migration.

Googling tells me that Nehalem (2008) introduced "invariant TSC"
(clock rate independent) and also socket synchronisation at the same
time, so systems without it are already pretty long in the tooth.

A quick peek at an AMD manual[1] tells me that a similar change
happened in 15H/Bulldozer/Piledriver/Steamroller/Excavator (2011),
identified with the same CPUID test.

My first reaction is that it seems like TSC would be the least of your
worries if you're measuring a VM that's currently migrating between
hosts, but maybe the idea is just that you have to make sure you don't
assume it can't ever go backwards or something like that...

Google Benchmark has some clues about how to spell this on MSVC, what
some instructions might be to research on ARM, etc.

[1] https://www.amd.com/system/files/TechDocs/47414_15h_sw_opt_guide.pdf
(page 373)
[2] https://github.com/google/benchmark/blob/master/src/cycleclock.h

Re: GROUP BY DISTINCT

[Tomas Vondra]

Hi Vik,

The patch seems quite ready, I have just two comments.

1) Shouldn't this add another  for DISTINCT, somewhere in the
documentation? Now the index points just to the SELECT DISTINCT part.

2) The part in gram.y that wraps/unwraps the boolean flag as an integer,
in order to stash it in the group lists is rather ugly, IMHO. It forces
all the places handling the list to be aware of this (there are not
many, but still ...). And there are no other places doing (bool) intVal
so it's not like there's a precedent for this.

I think the clean solution is to make group_clause produce a struct with
two fields, and just use that. Not sure how invasive that will be
outside gram.y, though.


Also, the all_or_distinct vs. distinct_or_all seems a bit error-prone. I
wonder if we can come up with some clearer names, describing the context
of those types.


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: pg_amcheck contrib application

[Mark Dilger]

> On Mar 12, 2021, at 11:24 AM, Robert Haas  wrote:
> 
> On Fri, Mar 12, 2021 at 2:05 PM  wrote:
>> I think there is a formatting glitch in lines like:
>> 
>> 2/4 relations (50%) 187977/187978 pages (99%), (testdb   
>>   )
>> 
>> I suppose that last part should show up trimmed as '(testdb)', right?
> 
> Actually I think this is intentional. The idea is that as the line is
> rewritten we don't want the close-paren to move around. It's the same
> thing pg_basebackup does with its progress reporting.
> 
> Now that is not to say that some other behavior might not be better,
> just that Mark was copying something that already exists, probably
> because he knows that I'm finnicky about consistency

I think Erik's test case only checked one database, which might be why it 
looked odd to him.  But consider:

  pg_amcheck -d foo -d bar -d myreallylongdatabasename -d myshortername -d baz 
--progress

The tool will respect your database ordering, and check foo, then bar, etc.  If 
you have --jobs greater than one, it will start checking some relations in bar 
before finishing all relations in foo, but with a fudge factor, pg_amcheck can 
report that the processing has moved on to database bar, etc.

You wouldn't want the parens to jump around when the long database names get 
processed.  So it keeps the parens in the same location, space pads shorter 
database names, and truncates overlong database names.


—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: pg_amcheck contrib application

[Mark Dilger]

> On Mar 12, 2021, at 2:55 PM, Robert Haas  wrote:
> 
> On Fri, Mar 12, 2021 at 5:24 PM Mark Dilger
>  wrote:
>> This does nothing to change the verbiage from contrib/amcheck, but it should 
>> address the problems discussed here in pg_amcheck's regression tests.
> 
> Committed.

Thanks.

There are two more, attached here.  The first deals with error message text 
which differs between farm animals, and the second deals with an apparent 
problem with IPC::Run shell expanding an asterisk on some platforms but not 
others.  That second one, if true, seems like a problem with scope beyond the 
pg_amcheck project, as TestLib::command_checks_all uses IPC::Run, and it would 
be desirable to have consistent behavior across platforms.




v2-0001-Fixing-pg_amcheck-regression-test-portability-iss.patch
Description: Binary data


v2-0002-Working-around-apparent-difficulty-in-IPC-Run.patch
Description: Binary data


—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Justin Pryzby]

On Fri, Mar 12, 2021 at 11:32:27PM +0200, Mark Rofail wrote:
> I have retested the patch on a windows build and it passes the regression
> tests thanks to Justin's recommendations. Hopefully, it will pass CI too.
> 
> Changelog:
>  - v7 (compatible with current master 2021-3-12,
> commit 02b5940dbea17d07a1dbcba3cbe113cc8b70f228)
> * re-add failing regression test with fixes
> * rebase patch

This still fails for CI (windows) and me (linux):

 SELECT ftest1 FROM FKTABLEFORARRAYGIN WHERE ftest1 @>> 5;
-   ftest1
--
- {5}
- {3,5,2,5}
- {3,5,4,1,3}
- {5,1}
- {3,4,5,3}
-(5 rows)
+ ftest1 
+
+(0 rows)

You added enable_seqscan=off, and EXPLAIN to show that it uses an bitmap index
scan, but do you know why it failed ?

I guess the failure is in the first patch, but isn't caught by test cases until
the 2nd patch.

-- 
Justin

Re: pg_amcheck contrib application

[Robert Haas]

On Fri, Mar 12, 2021 at 5:24 PM Mark Dilger
 wrote:
> This does nothing to change the verbiage from contrib/amcheck, but it should 
> address the problems discussed here in pg_amcheck's regression tests.

Committed.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: zstd compression for pg_dump

[Tomas Vondra]

On 1/4/21 11:17 AM, Daniil Zakhlystov wrote:
> Hi!
> 
>> On Jan 4, 2021, at 11:04 AM, Andrey Borodin  wrote:
>>
>> Daniil, is levels definition compatible with libpq compression patch?
>> +typedef struct Compress {
>> +CompressionAlgorithmalg;
>> +int level;
>> +/* Is a nondefault level set ?  This is useful since different 
>> compression
>> + * methods have different "default" levels.  For now we assume the 
>> levels
>> + * are all integer, though.
>> +*/
>> +boollevel_set;
>> +} Compress;
> 
> Similarly to this patch, it is also possible to define the compression level 
> at the initialization stage in libpq compression patch.
> 
> The difference is that in libpq compression patch the default compression 
> level always equal to 1, independently of the chosen compression algorithm.
> 
>> On Jan 4, 2021, at 11:04 AM, Andrey Borodin  wrote:
>>
>> Libpq compression encountered some problems with memory consumption which 
>> required some extra config efforts.
> 
> 
>> On Jan 4, 2021, at 12:06 PM, Justin Pryzby  wrote:
>>
>> RAM use is not significantly different from zlib, except that zstd --long 
>> adds
>> more memory.
> 
> Regarding ZSTD memory usage:
> 
> Recently I’ve made a couple of tests of libpq compression with different 
> ZLIB/ZSTD compression levels which shown that compressing/decompressing ZSTD 
> w/ high compression levels 
> require to allocate more virtual (Commited_AS) memory, which may be exploited 
> by malicious clients:
> 
> https://www.postgresql.org/message-id/62527092-16BD-479F-B503-FA527AF3B0C2%40yandex-team.ru
> 
> We can avoid high memory usage by limiting the max window size to 8MB. This 
> should effectively disable the support of compression levels above 19:
> https://www.postgresql.org/message-id/6A45DFAA-1682-4EF2-B835-C5F46615EC49%40yandex-team.ru
> 
> So maybe it is worthwhile to use similar restrictions in this patch.
>

I think there's a big difference between those two patches. In the libpq
case, the danger is that the client requests the server to compress the
data in a way that requires a lot of memory. I.e. the memory is consumed
on the server.

With this pg_dump patch, the compression is done by the pg_dump process,
not the server. So if the attacker configures the compression in a way
that requires a lot of memory, so what? He'll just allocate memory on
the client machine, where he could also just run a custom binary that
does a huge malloc().

So I don't think we need to worry about this too much.


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: pg_amcheck contrib application

[Mark Dilger]

> On Mar 12, 2021, at 1:43 PM, Robert Haas  wrote:
> 
> On Fri, Mar 12, 2021 at 2:31 PM Robert Haas  wrote:
>> I'll commit something shortly to address these.
> 
> There are some interesting failures in the test cases on the
> buildfarm. One of the tests ($offnum == 13) corrupts the TOAST pointer
> with a garbage value, expecting to get the message "final toast chunk
> number 0 differs from expected value 6". But on florican and maybe
> other systems we instead get "final toast chunk number 0 differs from
> expected value 5". That's because the value of TOAST_MAX_CHUNK_SIZE
> depends on MAXIMUM_ALIGNOF. I think that on 4-byte alignment systems
> it works out to 2000 and on 8-byte alignment systems it works out to
> 1996, and the value being stored is 1 bytes, hence the problem.
> The place where the calculation goes different seems to be in
> MaximumBytesPerTuple(), where it uses MAXALIGN_DOWN() on a value that,
> according to my calculations, will be 2038 on all platforms, but the
> output of MAXALIGN_DOWN() will be 2032 or 2036 depending on the
> platform. I think the solution to this is just to change the message
> to match \d+ chunks instead of exactly 6. We should do that right away
> to avoid having the buildfarm barf.
> 
> But, I also notice a couple of other things I think could be improved here:
> 
> 1. amcheck is really reporting the complete absence of any TOAST rows
> here due to a corrupted va_valueid. It could pick a better phrasing of
> that message than "final toast chunk number 0 differs from expected
> value XXX". I mean, there is no chunk 0. There are no chunks at all.
> 
> 2. Using S as the perl unpack code for the varlena header is
> not ideal, because it's really 2 1-byte fields followed by 4 4-byte
> fields. So I think you should be using CCllLL, for two unsigned bytes
> and then two signed 4-byte quantities and then two unsigned 4-byte
> quantities. I think if you did that you'd be overwriting the
> va_valueid with the *same* garbage value on every platform, which
> would be better than different ones. Perhaps when we improve the
> message as suggested in (1) this will become a live issue, since we
> might choose to say something like "no TOAST entries for value %u".
> 
> -- 
> Robert Haas
> EDB: http://www.enterprisedb.com


This does nothing to change the verbiage from contrib/amcheck, but it should 
address the problems discussed here in pg_amcheck's regression tests.



v1-0001-Fixing-portability-issues-in-pg_amcheck-regressio.patch
Description: Binary data

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Background writer and checkpointer in crash recovery

[Thomas Munro]

On Wed, Feb 3, 2021 at 11:11 AM Robert Haas  wrote:
> I think the way it works right now is stupid and the proposed change
> is going in the right direction. We have ample evidence already that
> handing off fsyncs to a background process is a good idea, and there's
> no reason why that shouldn't be beneficial during crash recovery just
> as it is at other times. But even if it somehow failed to improve
> performance during recovery, there's another good reason to do this,
> which is that it would make the code simpler. Having the pendingOps
> stuff in the startup process in some recovery situations and in the
> checkpointer in other recovery situations makes this harder to reason
> about. As Tom said, the system state where bgwriter and checkpointer
> are not running is an uncommon one, and is probably more likely to
> have (or grow) bugs than the state where they are running.

Yeah, it's a good argument.

> The rat's-nest of logic introduced by the comment "Perform a
> checkpoint to update all our recovery activity to disk." inside
> StartupXLOG() could really do with some simplification. Right now we
> have three cases: CreateEndOfRecoveryRecord(), RequestCheckpoint(),
> and CreateCheckpoint(). Maybe with this change we could get it down to
> just two, since RequestCheckpoint() already knows what to do about
> !IsUnderPostmaster.

True.  Done in this version.

Here's a rebase of this patch + Simon's patch to report on stats.

I also have a sketch patch to provide a GUC that turns off the
end-of-recovery checkpoint as mentioned earlier, attached (sharing
mainly because this is one of the stack of patches that Jakub was
testing for his baseback/PITR workloads and he might want to test some
more), but I'm not proposing that for PG14.  That idea is tangled up
with the "relfile tombstone" stuff I wrote about elsewhere[1], but I
haven't finished studying the full arsenal of footguns in that area
(it's something like: if we don't wait for end-of-recovery checkpoint
before allowing connections, then we'd better start creating
tombstones in recovery unless the WAL level is high enough to avoid
data eating hazards with unlogged changes and a double crash).

[1] https://commitfest.postgresql.org/33/3030/
From 7a82c9e622ea45c981d05dff08c1d4279ec3c73b Mon Sep 17 00:00:00 2001
From: Thomas Munro 
Date: Wed, 26 Aug 2020 16:34:33 +1200
Subject: [PATCH v2 1/3] Run checkpointer and bgworker in crash recovery.

Relieve the startup process of some writeback and checkpointing work
during crash recovery, making it more like replication recovery.  This
wasn't done back in commit cdd46c76 out of caution, but now it seems
more conservative to have the crash recovery environment work the same
as the more commonly exercised streaming replication environment.

In order to have a bgwriter running, you also need a checkpointer.
While you could give startup and bgwriter their own backend private
pending ops table, it's nicer to merger their requests in one place.

Reviewed-by: Tom Lane 
Reviewed-by: Simon Riggs 
Reviewed-by: Robert Haas 
Tested-by:  Jakub Wartak 
Discussion: https://postgr.es/m/CA%2BhUKGJ8NRsqgkZEnsnRc2MFROBV-jCnacbYvtpptK2A9YYp9Q%40mail.gmail.com
---
 src/backend/access/transam/xlog.c | 32 +--
 src/backend/postmaster/bgwriter.c |  3 ---
 src/backend/postmaster/checkpointer.c |  3 ---
 src/backend/postmaster/postmaster.c   | 17 ++
 src/backend/storage/sync/sync.c   | 30 +++--
 src/include/storage/sync.h|  1 -
 6 files changed, 21 insertions(+), 65 deletions(-)

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index f4d1ce5dea..6e8e6cf1e4 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -880,9 +880,6 @@ bool		reachedConsistency = false;
 
 static bool InRedo = false;
 
-/* Have we launched bgwriter during recovery? */
-static bool bgwriterLaunched = false;
-
 /* For WALInsertLockAcquire/Release functions */
 static int	MyLockNo = 0;
 static bool holdingAllLocks = false;
@@ -7268,25 +7265,14 @@ StartupXLOG(void)
 		/* Also ensure XLogReceiptTime has a sane value */
 		XLogReceiptTime = GetCurrentTimestamp();
 
+		PublishStartupProcessInformation();
+
 		/*
 		 * Let postmaster know we've started redo now, so that it can launch
-		 * checkpointer to perform restartpoints.  We don't bother during
-		 * crash recovery as restartpoints can only be performed during
-		 * archive recovery.  And we'd like to keep crash recovery simple, to
-		 * avoid introducing bugs that could affect you when recovering after
-		 * crash.
-		 *
-		 * After this point, we can no longer assume that we're the only
-		 * process in addition to postmaster!  Also, fsync requests are
-		 * subsequently to be handled by the checkpointer, not locally.
+		 * the archiver if necessary.
 		 */
-		if (ArchiveRecoveryRequested && IsUnderPostmaster)
-		{
-			PublishStartupProcessInformation();
-			

Re: VACUUM (DISABLE_PAGE_SKIPPING on)

[Tomas Vondra]

On 1/28/21 2:33 PM, Simon Riggs wrote:
> On Thu, 28 Jan 2021 at 12:53, Masahiko Sawada  wrote:
> 
>> This entry has been "Waiting on Author" status and the patch has not
>> been updated since Nov 30. Are you still planning to work on this?
> 
> Yes, new patch version tomorrow. Thanks for the nudge and the review.
> 

So, is it tomorrow already? ;-)

regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: pg_amcheck contrib application

[Peter Geoghegan]

On Fri, Mar 12, 2021 at 1:43 PM Robert Haas  wrote:
> There are some interesting failures in the test cases on the
> buildfarm.

I wonder if Andrew Dunstan (now CC'd) could configure his crake
buildfarm member to run pg_amcheck with the most expensive and
thorough options on the master branch (plus all new major version
branches going forward).

That would give us some degree of amcheck test coverage in the back
branches right away. It might even detect cross-version
inconsistencies. Or even pg_upgrade bugs.

-- 
Peter Geoghegan

Re: pg_amcheck contrib application

[Robert Haas]

On Fri, Mar 12, 2021 at 2:31 PM Robert Haas  wrote:
> I'll commit something shortly to address these.

There are some interesting failures in the test cases on the
buildfarm. One of the tests ($offnum == 13) corrupts the TOAST pointer
with a garbage value, expecting to get the message "final toast chunk
number 0 differs from expected value 6". But on florican and maybe
other systems we instead get "final toast chunk number 0 differs from
expected value 5". That's because the value of TOAST_MAX_CHUNK_SIZE
depends on MAXIMUM_ALIGNOF. I think that on 4-byte alignment systems
it works out to 2000 and on 8-byte alignment systems it works out to
1996, and the value being stored is 1 bytes, hence the problem.
The place where the calculation goes different seems to be in
MaximumBytesPerTuple(), where it uses MAXALIGN_DOWN() on a value that,
according to my calculations, will be 2038 on all platforms, but the
output of MAXALIGN_DOWN() will be 2032 or 2036 depending on the
platform. I think the solution to this is just to change the message
to match \d+ chunks instead of exactly 6. We should do that right away
to avoid having the buildfarm barf.

But, I also notice a couple of other things I think could be improved here:

1. amcheck is really reporting the complete absence of any TOAST rows
here due to a corrupted va_valueid. It could pick a better phrasing of
that message than "final toast chunk number 0 differs from expected
value XXX". I mean, there is no chunk 0. There are no chunks at all.

2. Using S as the perl unpack code for the varlena header is
not ideal, because it's really 2 1-byte fields followed by 4 4-byte
fields. So I think you should be using CCllLL, for two unsigned bytes
and then two signed 4-byte quantities and then two unsigned 4-byte
quantities. I think if you did that you'd be overwriting the
va_valueid with the *same* garbage value on every platform, which
would be better than different ones. Perhaps when we improve the
message as suggested in (1) this will become a live issue, since we
might choose to say something like "no TOAST entries for value %u".

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: documentation fix for SET ROLE

[Bossart, Nathan]

On 3/12/21, 11:14 AM, "Joe Conway"  wrote:
> On 3/12/21 1:16 PM, Bossart, Nathan wrote:
>> My main goal of this thread is to get the RESET ROLE documentation
>> fixed.  I don't have a terribly strong opinion on documenting these
>> special uses of "role".  I lean in favor of adding it, but I wouldn't
>> be strongly opposed to simply leaving it out for now.  But if we're
>> going to add it, I think we might as well add it everywhere.
>
>
> Looking back at the commit history it seems to me that this only works
> accidentally. Perhaps it would be best to fix RESET ROLE and be done with it.

That seems reasonable to me.

Nathan

Re: [patch] [doc] Minor variable related cleanup and rewording of plpgsql docs

[Tom Lane]

"David G. Johnston"  writes:
> I do agree that the delineation of "returns records or not" is not ideal
> here.  SELECT, then INSERT/UPDATE/DELETE (due to their shared RETURNING
> dynamic), then "DML commands", then "DMS exceptions" (these last two
> ideally leveraging the conceptual work noted above).  That said, I do not
> think this is such a big issue as to warrant that much of a rewrite.

I took a stab at doing that, just to see what it might look like.
I thought it comes out pretty well, really -- see what you think.

(This still uses the terminology "optimizable statement", but I'm open
to replacing that with something else.)

> In the following I'm confused as to why "column reference" is specified
> since those are not substituted:
> "Parameters will only be substituted in places where a parameter or
> column reference is syntactically allowed."

The meaning of "column reference" there is, I think, a reference to
a column of a table being read by a query.  In the counterexample
of "INSERT INTO mytable (col) ...", "col" cannot be replaced by a
data value.  But in "INSERT INTO mytable (col) SELECT foo FROM bar",
"foo" is a candidate for replacement, even though it's likely meant
as a reference to bar.foo.

> I'm not married to my explicit calling out of identifiers not being
> substitutable but that does tend to be what people try to do.

The problem I had with it was that it didn't help clarify this
distinction.  I'm certainly open to changes that do clarify that.

regards, tom lane

diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
index 9242c54329..15117c78cb 100644
--- a/doc/src/sgml/plpgsql.sgml
+++ b/doc/src/sgml/plpgsql.sgml
@@ -894,7 +894,7 @@ SELECT expression
 
  to the main SQL engine.  While forming the SELECT command,
  any occurrences of PL/pgSQL variable names
- are replaced by parameters, as discussed in detail in
+ are replaced by query parameters, as discussed in detail in
  .
  This allows the query plan for the SELECT to
  be prepared just once and then reused for subsequent
@@ -946,8 +946,7 @@ IF count(*)  0 FROM my_table THEN ...
 PL/pgSQL.
 Anything not recognized as one of these statement types is presumed
 to be an SQL command and is sent to the main database engine to execute,
-as described in 
-and .
+as described in .

 

@@ -993,31 +992,78 @@ complex_array[n].realpart = 12.3;
 

 
-   
-Executing a Command with No Result
+   
+Executing SQL Commands
 
 
- For any SQL command that does not return rows, for example
- INSERT without a RETURNING clause, you can
- execute the command within a PL/pgSQL function
- just by writing the command.
+ In general, any SQL command that does not return rows can be executed
+ within a PL/pgSQL function just by writing
+ the command.  For example, you could create and fill a table by writing
+
+CREATE TABLE mytable (id int primary key, data text);
+INSERT INTO mytable VALUES (1,'one'), (2,'two');
+
+
+
+
+ If the command does return rows (for example SELECT,
+ or INSERT/UPDATE/DELETE
+ with RETURNING), there are two ways to proceed.
+ When the command will return at most one row, or you only care about
+ the first row of output, write the command as usual but add
+ an INTO clause to capture the output, as described
+ in .
+ To process all of the output rows, write the command as the data
+ source for a FOR loop, as described in
+ .
 
 
 
- Any PL/pgSQL variable name appearing
- in the command text is treated as a parameter, and then the
+ Usually it is not sufficient to just execute statically-defined SQL
+ commands.  Typically you'll want a command to use varying data values,
+ or even to vary in more fundamental ways such as by using different
+ table names at different times.  Again, there are two ways to proceed,
+ depending on the particular command you need to execute.
+
+
+
+ PL/pgSQL variable values can be
+ automatically inserted into optimizable SQL commands, which
+ are SELECT, INSERT,
+ UPDATE, DELETE, and certain
+ utility commands that incorporate one of these, such
+ as EXPLAIN and CREATE TABLE ... AS
+ SELECT.  In these commands,
+ any PL/pgSQL variable name appearing
+ in the command text is replaced by a query parameter, and then the
  current value of the variable is provided as the parameter value
  at run time.  This is exactly like the processing described earlier
  for expressions; for details see .
 
 
 
- When executing a SQL command in this way,
+ When executing an optimizable SQL command in this way,
  PL/pgSQL may cache and re-use the execution
  plan for the command, as discussed in
  .
 
 
+
+ Non-optimizable SQL commands (also called utility commands) are not
+ capable of accepting 

Re: [patch] [doc] Minor variable related cleanup and rewording of plpgsql docs

[David G. Johnston]

On Fri, Mar 12, 2021 at 1:36 PM Tom Lane  wrote:

> Pavel Stehule  writes:
> > pá 12. 3. 2021 v 21:08 odesílatel Tom Lane  napsal:
> >> I attach a v3 that I like better, although there's room to disagree
> >> about that.
>
> > I am not sure if people can understand the "optimizable command" term.
> More
> > common categories are DML, DDL and SELECT. Maybe it is easier to say. DDL
> > statements don't support parametrizations, and then the variables cannot
> be
> > used there.
>
> Yeah, but DML/DDL is a pretty squishy separation as well, besides
> which it'd mislead people for cases such as CREATE TABLE AS SELECT.
> (Admittedly, I didn't mention that in my version either, but if you
> think in terms of whether the optimizer will be applied then you
> will draw the right conclusion.)
>

Related to an earlier email though, "CREATE TABLE AS SELECT" gets optimized
but "COPY (SELECT) TO" doesn't...

DML/DDL has the merit of being chapters 5 and 6 in the documentation (with
7 being SELECT).

I do agree that the delineation of "returns records or not" is not ideal
here.  SELECT, then INSERT/UPDATE/DELETE (due to their shared RETURNING
dynamic), then "DML commands", then "DMS exceptions" (these last two
ideally leveraging the conceptual work noted above).  That said, I do not
think this is such a big issue as to warrant that much of a rewrite.  But
in lieu of that, and based upon responses given on the mailing lists,
"utility commands" seems preferable to optimizable commands.  Defining,
either by name or by behavior, what utility commands are is needed though,
ideally outside of this chapter.  Then a paragraph in the "no result"
section should list explicitly those utility commands that are an
exception, since they have an attached SELECT statement that does get
optimized.

Maybe in Chapter 4, with some forward references, some of this can be
covered and the exceptions to the rule (like CREATE TABLE AS) can be
mentioned.

To address your point about "utility commands", lacking an external
definition to link to, I would change it to be "everything except
INSERT/UPDATE/DELETE, which are described below, as well as EXPLAIN and
SELECT which are described in the next section".  From there I like my
proposed flow into INSERT/UPDATE/DELETE w/o RETURNING, then from there the
RETURNING pointing forward to these being SELECT-like in behavior.

Adding a note about using EXECUTE works for me.

Calling EXPLAIN a utility command seems incorrect given that it behaves
just like a query.  If it quacks like a duck...

What other row set returning commands are you considering as being utility?


> Maybe there's no way out but to specifically list the statement types
> we can insert query parameters in.
>

In the following I'm confused as to why "column reference" is specified
since those are not substituted:

"Parameters will only be substituted in places where a parameter or
column reference is syntactically allowed."

I'm not married to my explicit calling out of identifiers not being
substitutable but that does tend to be what people try to do.

I'm good with the Pl/SQL wording proposal.

David J.

Re: pgbench - add pseudo-random permutation function

[Alvaro Herrera]

On 2021-Mar-13, Thomas Munro wrote:

> That doesn't sound like a bad option to me, if it makes this much
> simpler.  The main modern system without it seems to be MSVC.  The
> Linux, BSD, Apple, illumos, AIX systems using Clang/GCC with
> Intel/AMD/ARM/PowerPC CPUs have it, and the Windows systems using open
> source compilers have it.

Hmm.  Can we go a bit further, and say that if you don't have 128-bit
ints, then you can use pr_perm but only to a maximum of 32-bit ints?
Then you can do the calculations in 64-bit ints.  That's much less bad
than desupporting the feature altogether.

-- 
Álvaro Herrera39°49'30"S 73°17'W
"No necesitamos banderas
 No reconocemos fronteras"  (Jorge González)

Re: pgbench - add pseudo-random permutation function

[Thomas Munro]

On Mon, Mar 8, 2021 at 11:50 PM Fabien COELHO  wrote:
> > I may have time to become familiar or at least semi-comfortable with all
> > that weird math in it by then.
>
> Yep.
>
> Generating a parametric good-quality low-cost (but not
> cryptographically-secure) pseudo-random permutations on arbitrary sizes
> (not juste power of two sizes) is not a trivial task, I had to be quite
> creative to achieve it, hence the "weird" maths. I had a lot of bad
> not-really-working ideas before the current status of the patch.
>
> The code could be simplified if we assume that PG_INT128_TYPE will be
> available on all relevant architectures, and accept the feature not to be
> available if not.

That doesn't sound like a bad option to me, if it makes this much
simpler.  The main modern system without it seems to be MSVC.  The
Linux, BSD, Apple, illumos, AIX systems using Clang/GCC with
Intel/AMD/ARM/PowerPC CPUs have it, and the Windows systems using open
source compilers have it.

Re: [patch] [doc] Minor variable related cleanup and rewording of plpgsql docs

[Pavel Stehule]

pá 12. 3. 2021 v 21:36 odesílatel Tom Lane  napsal:

> Pavel Stehule  writes:
> > pá 12. 3. 2021 v 21:08 odesílatel Tom Lane  napsal:
> >> I attach a v3 that I like better, although there's room to disagree
> >> about that.
>
> > I am not sure if people can understand the "optimizable command" term.
> More
> > common categories are DML, DDL and SELECT. Maybe it is easier to say. DDL
> > statements don't support parametrizations, and then the variables cannot
> be
> > used there.
>
> Yeah, but DML/DDL is a pretty squishy separation as well, besides
> which it'd mislead people for cases such as CREATE TABLE AS SELECT.
> (Admittedly, I didn't mention that in my version either, but if you
> think in terms of whether the optimizer will be applied then you
> will draw the right conclusion.)
>

Can it be better to use word planner instead of optimizer? An optimization
is too common a word, and unfortunately a lot of people have no idea what
optimization in SQL means.

It can be pretty hard, because the people that have problems here don't
know what is a plan or what is an optimization.


> Maybe there's no way out but to specifically list the statement types
> we can insert query parameters in.
>

can be


> regards, tom lane
>

Re: [patch] [doc] Minor variable related cleanup and rewording of plpgsql docs

[Tom Lane]

Pavel Stehule  writes:
> pá 12. 3. 2021 v 21:08 odesílatel Tom Lane  napsal:
>> I attach a v3 that I like better, although there's room to disagree
>> about that.

> I am not sure if people can understand the "optimizable command" term. More
> common categories are DML, DDL and SELECT. Maybe it is easier to say. DDL
> statements don't support parametrizations, and then the variables cannot be
> used there.

Yeah, but DML/DDL is a pretty squishy separation as well, besides
which it'd mislead people for cases such as CREATE TABLE AS SELECT.
(Admittedly, I didn't mention that in my version either, but if you
think in terms of whether the optimizer will be applied then you
will draw the right conclusion.)

Maybe there's no way out but to specifically list the statement types
we can insert query parameters in.

regards, tom lane

Re: [patch] [doc] Minor variable related cleanup and rewording of plpgsql docs

[Pavel Stehule]

Hi

pá 12. 3. 2021 v 21:08 odesílatel Tom Lane  napsal:

> I looked over the v2 patch.  Parts of it seem like improvements but
> other parts definitely don't.  In particular, I thought you introduced
> a great deal of confusion in 43.5.2 (Executing a Command with No Result).
> The statement that you can write a non-result-returning SQL command as-is
> is true in general, and ought not be confused with the question of whether
> you can insert variable values into it.  Also, starting with a spongy
> definition of "utility command" and then contrasting with that does not
> seem to me to add clarity.
>
> I attach a v3 that I like better, although there's room to disagree
> about that.  I've always felt that the separation between 43.5.2 and
> 43.5.3 was rather artificial --- it's okay I guess for describing
> how to handle command output, but we end up with considerable
> duplication when it comes to describing how to insert values into a
> command.  It's tempting to try re-splitting it to separate optimizable
> from non-optimizable statements; but maybe that'd just end with
> different duplication.
>

I am not sure if people can understand the "optimizable command" term. More
common categories are DML, DDL and SELECT. Maybe it is easier to say. DDL
statements don't support parametrizations, and then the variables cannot be
used there.



> regards, tom lane
>
>

Re: [patch] [doc] Minor variable related cleanup and rewording of plpgsql docs

[Tom Lane]

I looked over the v2 patch.  Parts of it seem like improvements but
other parts definitely don't.  In particular, I thought you introduced
a great deal of confusion in 43.5.2 (Executing a Command with No Result).
The statement that you can write a non-result-returning SQL command as-is
is true in general, and ought not be confused with the question of whether
you can insert variable values into it.  Also, starting with a spongy
definition of "utility command" and then contrasting with that does not
seem to me to add clarity.

I attach a v3 that I like better, although there's room to disagree
about that.  I've always felt that the separation between 43.5.2 and
43.5.3 was rather artificial --- it's okay I guess for describing
how to handle command output, but we end up with considerable
duplication when it comes to describing how to insert values into a
command.  It's tempting to try re-splitting it to separate optimizable
from non-optimizable statements; but maybe that'd just end with
different duplication.

regards, tom lane

diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
index 9242c54329..aa868b4191 100644
--- a/doc/src/sgml/plpgsql.sgml
+++ b/doc/src/sgml/plpgsql.sgml
@@ -894,7 +894,7 @@ SELECT expression
 
  to the main SQL engine.  While forming the SELECT command,
  any occurrences of PL/pgSQL variable names
- are replaced by parameters, as discussed in detail in
+ are replaced by query parameters, as discussed in detail in
  .
  This allows the query plan for the SELECT to
  be prepared just once and then reused for subsequent
@@ -1004,20 +1004,32 @@ complex_array[n].realpart = 12.3;
 
 
 
- Any PL/pgSQL variable name appearing
- in the command text is treated as a parameter, and then the
+ In optimizable SQL commands (INSERT,
+ UPDATE, and DELETE),
+ any PL/pgSQL variable name appearing
+ in the command text is replaced by a query parameter, and then the
  current value of the variable is provided as the parameter value
  at run time.  This is exactly like the processing described earlier
  for expressions; for details see .
 
 
 
- When executing a SQL command in this way,
+ When executing an optimizable SQL command in this way,
  PL/pgSQL may cache and re-use the execution
  plan for the command, as discussed in
  .
 
 
+
+ Non-optimizable SQL commands (also called utility commands) are not
+ capable of accepting query parameters.  So automatic substitution
+ of PL/pgSQL variables does not work in such
+ commands.  To include non-constant text in a utility command executed
+ from PL/pgSQL, you must build the utility
+ command as a string and then EXECUTE it, as
+ discussed in .
+
+
 
  Sometimes it is useful to evaluate an expression or SELECT
  query but discard the result, for example when calling a function
@@ -1095,11 +1107,11 @@ DELETE ... RETURNING expressions INTO STRIC
  record/row fields.
  PL/pgSQL variables will be
  substituted into the rest of the query, and the plan is cached,
- just as described above for commands that do not return rows.
+ just as described above for optimizable commands that do not return rows.
  This works for SELECT,
  INSERT/UPDATE/DELETE with
- RETURNING, and utility commands that return row-set
- results (such as EXPLAIN).
+ RETURNING, and certain utility commands
+ that return row sets, such as EXPLAIN.
  Except for the INTO clause, the SQL command is the same
  as it would be written outside PL/pgSQL.
 
@@ -2567,7 +2579,7 @@ $$ LANGUAGE plpgsql;
 
 
 
- PL/pgSQL variables are substituted into the query text,
+ PL/pgSQL variables are replaced by query parameters,
  and the query plan is cached for possible re-use, as discussed in
  detail in  and
  .
@@ -4643,7 +4655,7 @@ CREATE EVENT TRIGGER snitch ON ddl_command_start EXECUTE FUNCTION snitch();
 SQL statements and expressions within a PL/pgSQL function
 can refer to variables and parameters of the function.  Behind the scenes,
 PL/pgSQL substitutes query parameters for such references.
-Parameters will only be substituted in places where a parameter or
+Query parameters will only be substituted in places where a parameter or
 column reference is syntactically allowed.  As an extreme case, consider
 this example of poor programming style:
 
@@ -4657,13 +4669,6 @@ INSERT INTO foo (foo) VALUES (foo);
 variable.

 
-   
-
- PostgreSQL versions before 9.0 would try
- to substitute the variable in all three cases, leading to syntax errors.
-
-   
-

 Since the names of variables are syntactically no different from the names
 of table columns, there can be ambiguity in statements that also refer to
@@ -5314,11 +5319,12 @@ HINT:  Make sure the query returns the exact list of 

Re: pg_amcheck contrib application

[Robert Haas]

On Fri, Mar 12, 2021 at 1:35 PM Peter Geoghegan  wrote:
> On Fri, Mar 12, 2021 at 10:32 AM Peter Geoghegan  wrote:
> > Thank you both, Mark and Robert. This is excellent work!

Thanks.

> FYI I see these compiler warnings just now:
>
> pg_amcheck.c:1653:4: warning: ISO C90 forbids mixed declarations and
> code [-Wdeclaration-after-statement]
>  1653 |DatabaseInfo *dat = (DatabaseInfo *)
> pg_malloc0(sizeof(DatabaseInfo));
>   |^~~~
> pg_amcheck.c: In function ‘compile_relation_list_one_db’:
> pg_amcheck.c:2060:9: warning: variable ‘is_btree’ set but not used
> [-Wunused-but-set-variable]
>  2060 |   bool  is_btree = false;
>   | ^~~~
>
> Looks like this 'is_btree' variable should be PG_USED_FOR_ASSERTS_ONLY.

I'll commit something shortly to address these.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: pg_amcheck contrib application

[Robert Haas]

On Fri, Mar 12, 2021 at 2:05 PM  wrote:
> I think there is a formatting glitch in lines like:
>
> 2/4 relations (50%) 187977/187978 pages (99%), (testdb
>  )
>
> I suppose that last part should show up trimmed as '(testdb)', right?

Actually I think this is intentional. The idea is that as the line is
rewritten we don't want the close-paren to move around. It's the same
thing pg_basebackup does with its progress reporting.

Now that is not to say that some other behavior might not be better,
just that Mark was copying something that already exists, probably
because he knows that I'm finnicky about consistency

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: documentation fix for SET ROLE

[Joe Conway]

On 3/12/21 1:16 PM, Bossart, Nathan wrote:

On 3/12/21, 6:35 AM, "Laurenz Albe"  wrote:

On Fri, 2021-03-12 at 10:16 +0100, I wrote:

After sleeping on it, I have come to think that it is excessive to write
so much documentation for a feature that is that unimportant.

It takes some effort to come up with a good use case for it.

I think we can add a few lines to ALTER ROLE, perhaps ALTER DATABASE
(although I don't see what sense it could make to set that on the database 
level),
and briefly explain the difference between RESET ROLE and SET ROLE NONE.

I think adding too much detail will harm - anyone who needs to know the
exact truth can resort to the implementation.

I'll try to come up with a proposal later.


Attached is my idea of the documentation change.

I think that ALTER DATABASE ... SET ROLE can remain undocumented, because
I cannot imagine that it could be useful.

I am unsure if specifying "role" in a libpq connect string might be
worth documenting.  Can you think of a use case?


My main goal of this thread is to get the RESET ROLE documentation
fixed.  I don't have a terribly strong opinion on documenting these
special uses of "role".  I lean in favor of adding it, but I wouldn't
be strongly opposed to simply leaving it out for now.  But if we're
going to add it, I think we might as well add it everywhere.



Looking back at the commit history it seems to me that this only works 
accidentally. Perhaps it would be best to fix RESET ROLE and be done with it.


Joe

--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

Re: pg_amcheck contrib application

[er]

> On 2021.03.12. 19:10 Robert Haas  wrote:
> 
>  
> On Fri, Mar 12, 2021 at 11:41 AM Mark Dilger
>  wrote:
> > In this next patch, your documentation patch has been applied, and the 
> > whole project has been relocated from contrib/pg_amcheck to 
> > src/bin/pg_amcheck.
> 
> Committed that way with some small adjustments. Let's see what the
> buildfarm thinks.
> 

Hi,

An output-formatting error, I think:

I ran pg_amcheck against a 1.5 GB table:

-- pg_amcheck --progress --on-error-stop --heapallindexed -vt myjsonfile100k

pg_amcheck: including database: "testdb"
pg_amcheck: in database "testdb": using amcheck version "1.3" in schema "public"
0/4 relations (0%)  0/187978 pages (0%) 
 
pg_amcheck: checking heap table "testdb"."public"."myjsonfile100k"
pg_amcheck: checking btree index "testdb"."public"."myjsonfile100k_pkey"
2/4 relations (50%) 187977/187978 pages (99%), (testdb  
   )
pg_amcheck: checking btree index "testdb"."pg_toast"."pg_toast_26110_index"
3/4 relations (75%) 187978/187978 pages (100%), (testdb 
)
pg_amcheck: checking heap table "testdb"."pg_toast"."pg_toast_26110"
4/4 relations (100%) 187978/187978 pages (100%) 


I think there is a formatting glitch in lines like:

2/4 relations (50%) 187977/187978 pages (99%), (testdb  
   )

I suppose that last part should show up trimmed as '(testdb)', right?

Thanks,

Erik Rijkers

Re: pg_amcheck contrib application

[Peter Geoghegan]

On Fri, Mar 12, 2021 at 10:32 AM Peter Geoghegan  wrote:
> Thank you both, Mark and Robert. This is excellent work!

FYI I see these compiler warnings just now:

pg_amcheck.c:1653:4: warning: ISO C90 forbids mixed declarations and
code [-Wdeclaration-after-statement]
 1653 |DatabaseInfo *dat = (DatabaseInfo *)
pg_malloc0(sizeof(DatabaseInfo));
  |^~~~
pg_amcheck.c: In function ‘compile_relation_list_one_db’:
pg_amcheck.c:2060:9: warning: variable ‘is_btree’ set but not used
[-Wunused-but-set-variable]
 2060 |   bool  is_btree = false;
  | ^~~~

Looks like this 'is_btree' variable should be PG_USED_FOR_ASSERTS_ONLY.

-- 
Peter Geoghegan

Re: pg_amcheck contrib application

[Peter Geoghegan]

On Fri, Mar 12, 2021 at 10:10 AM Robert Haas  wrote:
> Committed that way with some small adjustments. Let's see what the
> buildfarm thinks.

Thank you both, Mark and Robert. This is excellent work!

-- 
Peter Geoghegan

Re: documentation fix for SET ROLE

[Bossart, Nathan]

On 3/12/21, 6:35 AM, "Laurenz Albe"  wrote:
> On Fri, 2021-03-12 at 10:16 +0100, I wrote:
>> After sleeping on it, I have come to think that it is excessive to write
>> so much documentation for a feature that is that unimportant.
>>
>> It takes some effort to come up with a good use case for it.
>>
>> I think we can add a few lines to ALTER ROLE, perhaps ALTER DATABASE
>> (although I don't see what sense it could make to set that on the database 
>> level),
>> and briefly explain the difference between RESET ROLE and SET ROLE NONE.
>>
>> I think adding too much detail will harm - anyone who needs to know the
>> exact truth can resort to the implementation.
>>
>> I'll try to come up with a proposal later.
>
> Attached is my idea of the documentation change.
>
> I think that ALTER DATABASE ... SET ROLE can remain undocumented, because
> I cannot imagine that it could be useful.
>
> I am unsure if specifying "role" in a libpq connect string might be
> worth documenting.  Can you think of a use case?

My main goal of this thread is to get the RESET ROLE documentation
fixed.  I don't have a terribly strong opinion on documenting these
special uses of "role".  I lean in favor of adding it, but I wouldn't
be strongly opposed to simply leaving it out for now.  But if we're
going to add it, I think we might as well add it everywhere.

Nathan

Re: Confusing behavior of psql's \e

[Tom Lane]

Laurenz Albe  writes:
> Done like that in the attached patch version 4.

I pushed the race-condition-fixing part of this, since that's an
unarguable bug fix and hence seems OK to back-patch.  (I added a
check on change of file size, because why not.)

Attached is the rest, just to keep the cfbot happy.

I don't think this is quite committable yet.  The division of
labor between do_edit() and its callers seems to need more
thought: in particular, I see that \ef now fails to print
"No changes" when I would expect it to.  But the real question
is whether there is any non-error condition in which do_edit
should not set the query_buffer, either to the edit result
or empty.  If we're going to improve the header comment for
do_edit, I would expect it to specify what happens to the
query_buf, and the fact that I can't write a concise spec
leads me to think that a bit more design effort is needed.

Also, somewhat cosmetic, but: I feel like the hunk that is
setting discard_on_quit in exec_command_edit is assuming
more than it ought to about what copy_previous_query will do.
Perhaps it's worth making copy_previous_query return a bool
indicating whether it copied previous_buf, and then
exec_command_edit becomes

discard_on_quit = copy_previous_query(query_buf, previous_buf);

regards, tom lane

diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 13c1edfa4d..0bf69174ff 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -1970,7 +1970,9 @@ testdb=
 
 
 
-The new contents of the query buffer are then re-parsed according to
+If you edit a file or the previous query, and you quit the editor without
+modifying the file, the query buffer is cleared.
+Otherwise, the new contents of the query buffer are re-parsed according to
 the normal rules of psql, treating the
 whole buffer as a single line.  Any complete queries are immediately
 executed; that is, if the query buffer contains or ends with a
@@ -2039,7 +2041,8 @@ Tue Oct 26 21:40:57 CEST 1999
  in the form of a CREATE OR REPLACE FUNCTION or
  CREATE OR REPLACE PROCEDURE command.
  Editing is done in the same way as for \edit.
- After the editor exits, the updated command is executed immediately
+ If you quit the editor without saving, the statement is discarded.
+ If you save and exit the editor, the updated command is executed immediately
  if you added a semicolon to it.  Otherwise it is redisplayed;
  type semicolon or \g to send it, or \r
  to cancel.
@@ -2115,7 +2118,8 @@ Tue Oct 26 21:40:57 CEST 1999
  This command fetches and edits the definition of the named view,
  in the form of a CREATE OR REPLACE VIEW command.
  Editing is done in the same way as for \edit.
- After the editor exits, the updated command is executed immediately
+ If you quit the editor without saving, the statement is discarded.
+ If you save and exit the editor, the updated command is executed immediately
  if you added a semicolon to it.  Otherwise it is redisplayed;
  type semicolon or \g to send it, or \r
  to cancel.
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 838f74..3b97cd51dc 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -152,7 +152,7 @@ static void copy_previous_query(PQExpBuffer query_buf, PQExpBuffer previous_buf)
 static bool do_connect(enum trivalue reuse_previous_specification,
 	   char *dbname, char *user, char *host, char *port);
 static bool do_edit(const char *filename_arg, PQExpBuffer query_buf,
-	int lineno, bool *edited);
+	int lineno, bool *edited, bool discard_on_quit);
 static bool do_shell(const char *command);
 static bool do_watch(PQExpBuffer query_buf, double sleep);
 static bool lookup_object_oid(EditableObjectType obj_type, const char *desc,
@@ -1004,6 +1004,13 @@ exec_command_edit(PsqlScanState scan_state, bool active_branch,
 			}
 			if (status != PSQL_CMD_ERROR)
 			{
+/*
+ * If the query buffer is empty, we'll edit the previous
+ * query. But in that case, we don't want to keep that if the
+ * editor is quit.
+ */
+bool		discard_on_quit = (query_buf->len == 0);
+
 expand_tilde();
 if (fname)
 	canonicalize_path(fname);
@@ -1011,7 +1018,7 @@ exec_command_edit(PsqlScanState scan_state, bool active_branch,
 /* If query_buf is empty, recall previous query for editing */
 copy_previous_query(query_buf, previous_buf);
 
-if (do_edit(fname, query_buf, lineno, NULL))
+if (do_edit(fname, query_buf, lineno, NULL, discard_on_quit))
 	status = PSQL_CMD_NEWEDIT;
 else
 	status = PSQL_CMD_ERROR;
@@ -1134,11 +1141,9 @@ exec_command_ef_ev(PsqlScanState scan_state, bool active_branch,
 		{
 			bool		edited = false;
 
-			if (!do_edit(NULL, 

Re: pg_amcheck contrib application

[Robert Haas]

On Fri, Mar 12, 2021 at 11:41 AM Mark Dilger
 wrote:
> In this next patch, your documentation patch has been applied, and the whole 
> project has been relocated from contrib/pg_amcheck to src/bin/pg_amcheck.

Committed that way with some small adjustments. Let's see what the
buildfarm thinks.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: [POC] verifying UTF-8 using SIMD instructions

[John Naylor]

On Fri, Mar 12, 2021 at 9:14 AM Amit Khandekar 
wrote:
>
> On my Arm64 VM :
>
> HEAD :
>  mixed | ascii
> ---+---
>   1091 |   628
> (1 row)
>
> PATCHED :
>  mixed | ascii
> ---+---
>681 |   119

Thanks for testing! Good, the speedup is about as much as I can hope for
using plain C. In the next patch I'll go ahead and squash in the ascii fast
path, using 16-byte stride, unless there are objections. I claim we can
live with the regression Heikki found on an old 32-bit Arm platform since
it doesn't seem to be true of Arm in general.

> I guess, if at all we use the equivalent Arm NEON intrinsics, the
> "mixed" figures will be close to the "ascii" figures, going by your
> figures on x86.

I would assume so.

> I was not thinking about auto-vectorizing the code in
> pg_validate_utf8_sse42(). Rather, I was considering auto-vectorization
> inside the individual helper functions that you wrote, such as
> _mm_setr_epi8(), shift_right(), bitwise_and(), prev1(), splat(),

If the PhD holders who came up with this algorithm thought it possible to
do it that way, I'm sure they would have. In reality, simdjson has
different files for SSE4, AVX, AVX512, NEON, and Altivec. We can
incorporate any of those as needed. That's a PG15 project, though, and I'm
not volunteering.

--
John Naylor
EDB: http://www.enterprisedb.com

Re: documentation fix for SET ROLE

[David G. Johnston]

On Fri, Mar 12, 2021 at 7:35 AM Laurenz Albe 
wrote:

> On Fri, 2021-03-12 at 10:16 +0100, I wrote:
> > After sleeping on it, I have come to think that it is excessive to write
> > so much documentation for a feature that is that unimportant.
> >
> > It takes some effort to come up with a good use case for it.
> >
> > I think we can add a few lines to ALTER ROLE, perhaps ALTER DATABASE
> > (although I don't see what sense it could make to set that on the
> database level),
> > and briefly explain the difference between RESET ROLE and SET ROLE NONE.
> >
> > I think adding too much detail will harm - anyone who needs to know the
> > exact truth can resort to the implementation.
> >
> > I'll try to come up with a proposal later.
>
> Attached is my idea of the documentation change.
>
> I think that ALTER DATABASE ... SET ROLE can remain undocumented, because
> I cannot imagine that it could be useful.
>
> I am unsure if specifying "role" in a libpq connect string might be
> worth documenting.  Can you think of a use case?
>

Does our imagination really matter here?  It works and is just as "useful"
as "ALTER ROLE" and so should be documented if we document ALTER ROLE.

I agree that ALTER DATABASE seems entirely useless and even
counter-productive...but I would still document if only because we document
ALTER ROLE and they should be kept similar.

Haven't formed an opinion on the merits of the two patches.

David J.

Re: documentation fix for SET ROLE

[Laurenz Albe]

On Fri, 2021-03-12 at 10:16 +0100, I wrote:
> After sleeping on it, I have come to think that it is excessive to write
> so much documentation for a feature that is that unimportant.
> 
> It takes some effort to come up with a good use case for it.
> 
> I think we can add a few lines to ALTER ROLE, perhaps ALTER DATABASE
> (although I don't see what sense it could make to set that on the database 
> level),
> and briefly explain the difference between RESET ROLE and SET ROLE NONE.
> 
> I think adding too much detail will harm - anyone who needs to know the
> exact truth can resort to the implementation.
> 
> I'll try to come up with a proposal later.

Attached is my idea of the documentation change.

I think that ALTER DATABASE ... SET ROLE can remain undocumented, because
I cannot imagine that it could be useful.

I am unsure if specifying "role" in a libpq connect string might be
worth documenting.  Can you think of a use case?

Yours,
Laurenz Albe
From 5daa6c2a874898506fda316fe651f107dbed34d5 Mon Sep 17 00:00:00 2001
From: Laurenz Albe 
Date: Fri, 12 Mar 2021 15:32:01 +0100
Subject: [PATCH] Document ALTER ROLE ... SET ROLE

This was previously undocumented, like ALTER DATABASE ... SET ROLE or
specifying "role=some_role" in a libpq connect string, but it might
have some practical use cases.
---
 doc/src/sgml/ref/alter_role.sgml | 15 +--
 doc/src/sgml/ref/set_role.sgml   |  8 ++--
 2 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/doc/src/sgml/ref/alter_role.sgml b/doc/src/sgml/ref/alter_role.sgml
index 5aa5648ae7..a377fc303b 100644
--- a/doc/src/sgml/ref/alter_role.sgml
+++ b/doc/src/sgml/ref/alter_role.sgml
@@ -40,6 +40,7 @@ ALTER ROLE name RENAME TO role_specification | ALL } [ IN DATABASE database_name ] SET configuration_parameter { TO | = } { value | DEFAULT }
 ALTER ROLE { role_specification | ALL } [ IN DATABASE database_name ] SET configuration_parameter FROM CURRENT
+ALTER ROLE { role_specification | ALL } [ IN DATABASE database_name ] SET ROLE other_role
 ALTER ROLE { role_specification | ALL } [ IN DATABASE database_name ] RESET configuration_parameter
 ALTER ROLE { role_specification | ALL } [ IN DATABASE database_name ] RESET ALL
 
@@ -91,7 +92,7 @@ ALTER ROLE { role_specification | A
 
   
The remaining variants change a role's session default for a configuration
-   variable, either for all databases or, when the IN
+   variable or the role assumed, either for all databases or, when the IN
DATABASE clause is specified, only for sessions in the named
database.  If ALL is specified instead of a role name,
this changes the setting for all roles.  Using ALL
@@ -104,7 +105,7 @@ ALTER ROLE { role_specification | A
starts a new session, the specified value becomes the session
default, overriding whatever setting is present in
postgresql.conf or has been received from the postgres
-   command line. This only happens at login time; executing
+   command line.  This only happens at login time; executing
SET ROLE or
SET SESSION AUTHORIZATION does not cause new
configuration values to be set.
@@ -234,6 +235,16 @@ ALTER ROLE { role_specification | A

   
  
+
+ 
+   other_role
+   
+ 
+   The name of a role that contains the modified role as a member.
+   This identity is automatically assumed when the connection is established.
+ 
+   
+ 
 
  
 
diff --git a/doc/src/sgml/ref/set_role.sgml b/doc/src/sgml/ref/set_role.sgml
index 739f2c5cdf..4ce4873fe8 100644
--- a/doc/src/sgml/ref/set_role.sgml
+++ b/doc/src/sgml/ref/set_role.sgml
@@ -53,8 +53,12 @@ RESET ROLE
   
 
   
-   The NONE and RESET forms reset the current
-   user identifier to be the current session user identifier.
+   SET ROLE NONE sets the current user identifier to the
+   session user identifier, as returned by session_user.
+   RESET ROLE sets the current user identifier to the
+   connection-time setting, which may be different from the session user
+   identifier if a different role has been set with
+   ALTER ROLE.
These forms can be executed by any user.
   
  
-- 
2.26.2

Re: shared-memory based stats collector

[Fujii Masao]

On 2021/03/12 17:24, Kyotaro Horiguchi wrote:

At Fri, 12 Mar 2021 15:13:15 +0900, Fujii Masao  
wrote in

On 2021/03/12 13:49, Kyotaro Horiguchi wrote:

I noticed that I accidentally removed the launch-suppression feature
that is to avoid frequent relaunching.  That mechanism is needed on
the postmaster side. I added PgArchIsSuppressed() to do the same check
with the old pgarch_start() and make it called as a part of
PgArchStartupAllowed().


You're right! At least for me the function name PgArchIsSuppressed()
sounds not good to me. What about something like PgArchCanRestart()?


The reason for the name was some positive-meaning names came up with
me are confusing with PgArchStartupAllowed().  The name
PgArchCanRestart suggests that it's usable only when
restarting. However, the function requires to be called also when the
first launch since last_pgarch_start_time needs to be updated every
time archiver is launched.

Anyway in the attached the name is changed wihtout changing its usage.


Thanks! If we come up with better name, let's rename the function later.



# I don't like it uses "can" as "allowed" so much. The archiver
# actually can restart but is just inhibited to restart.


This is not fault of this patch. But last_pgarch_start_time should be
initialized with zero?


Right. I noticed that but forgot to fix it.


+   if ((curtime - last_pgarch_start_time) < PGARCH_RESTART_INTERVAL)
+   return true;

Why did you remove the cast to unsigned int there?


The cast converts small negative values to large numbers, the code
looks like intending to allow archiver launched when curtime goes
behind than last_pgarch_start_time. That is the case the on-memory
data is corrupt. I'm not sure it's worth worrying and in the first
place if we want to care of the case we should explicitly compare the
operands of the subtraction.  I did that in the attached.


That's an idea. But the similar calculation using that cast is used in
other places (e.g., in pgarch_MainLoop()), so I'm thinking that it's
better not to change that...




And the last_pgarch_start_time is accessed only in the function. I
moved it to inside the function.


OK.





+   /*
+ * Advertise our latch that backends can use to wake us up while
we're
+* sleeping.
+*/
+   PgArch->pgprocno = MyProc->pgprocno;

The comment should be updated?


Hmm. What is advertised is our pgprocno.. Fixed.


OK.

Thanks for updating the patch! I applied some minor changes into your patch.
Attached is the updated version of the patch. I'm thinking to commit this 
version.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index c35045faa1..db4b4e460c 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -935,6 +935,7 @@ postgres   27093  0.0  0.0  30096  2752 ?Ss   11:34 
  0:00 postgres: ser
logical replication worker,
parallel worker, background 
writer,
client backend, checkpointer,
+   archiver,
startup, walreceiver,
walsender and walwriter.
In addition, background workers registered by extensions may have
diff --git a/src/backend/access/transam/xlogarchive.c 
b/src/backend/access/transam/xlogarchive.c
index 1c5a4f8b5a..26b023e754 100644
--- a/src/backend/access/transam/xlogarchive.c
+++ b/src/backend/access/transam/xlogarchive.c
@@ -25,11 +25,11 @@
 #include "common/archive.h"
 #include "miscadmin.h"
 #include "postmaster/startup.h"
+#include "postmaster/pgarch.h"
 #include "replication/walsender.h"
 #include "storage/fd.h"
 #include "storage/ipc.h"
 #include "storage/lwlock.h"
-#include "storage/pmsignal.h"
 
 /*
  * Attempt to retrieve the specified file from off-line archival storage.
@@ -491,7 +491,7 @@ XLogArchiveNotify(const char *xlog)
 
/* Notify archiver that it's got something to do */
if (IsUnderPostmaster)
-   SendPostmasterSignal(PMSIGNAL_WAKEN_ARCHIVER);
+   PgArchWakeup();
 }
 
 /*
diff --git a/src/backend/bootstrap/bootstrap.c 
b/src/backend/bootstrap/bootstrap.c
index 6f615e6622..41da0c5059 100644
--- a/src/backend/bootstrap/bootstrap.c
+++ b/src/backend/bootstrap/bootstrap.c
@@ -317,6 +317,9 @@ AuxiliaryProcessMain(int argc, char *argv[])
case StartupProcess:
MyBackendType = B_STARTUP;
break;
+   case ArchiverProcess:
+   MyBackendType = B_ARCHIVER;
+   break;
case BgWriterProcess:
MyBackendType = B_BG_WRITER;
break;
@@ -437,30 +440,29 @@ AuxiliaryProcessMain(int argc, char *argv[])
proc_exit(1);   /* should never return */
 
case StartupProcess:
-   /* don't set signals, startup process has its 

Re: pg_amcheck contrib application

[Robert Haas]

On Fri, Mar 12, 2021 at 12:00 AM Mark Dilger
 wrote:
> Your proposal is used in this next version of the patch, along with a 
> resolution to the solution to the -D option handling, discussed before, and a 
> change to make --schema and --exclude-schema options accept "database.schema" 
> patterns as well as "schema" patterns.  It previously only interpreted the 
> parameter as a schema without treating embedded dots as separators, but that 
> seems strangely inconsistent with the way all the other pattern options work, 
> so I made it consistent.  (I think the previous behavior was defensible, but 
> harder to explain and perhaps less intuitive.)

Well, OK. In that case I guess we need to patch the docs a little
more. Here's a patch documentation that revised behavior, and also
tidying up a few other things I noticed along the way.

Since nobody is saying we *shouldn't* move this to src/bin, I think
you may as well go put it there per Peter's suggestion.

Then I think it's time to get this committed and move on to the next thing.

-- 
Robert Haas
EDB: http://www.enterprisedb.com


more-doc-hacking.patch
Description: Binary data

Re: Self-join optimisation

[Hywel Carver]

Hi, thanks for your replies. I've tested the patch and it works for me in
the cases where I'd use it.

> And explain why your application is doing queries like this, and why it
can't be changed to changed to not generate such queries.

Reading the thread, it looks like some of the requests for this feature are
coming from people using ORMs that generate bad queries. That's not been my
experience - I've always been able to find a way to construct the right
query through the ORM or just write correct SQL. When I've wanted this
feature has always been in relation to combining views.

For example, I was recently helping out a company that runs a booking
system for leisure activities, and their database has a view for how many
staff are available on a given day to supervise a given activity (e.g.
surfing), and a view for how much equipment is available on a given day
(e.g. how many surfboards). They also have a view for the equipment
requirements for a given activity (e.g. some boat trips require a minimum
of 2 boats and 4 oars). When they want to make bookings, they have to
combine data from these views, and the tables that create them.

It would definitely be possible to write one view that had all of this data
in (and maintain the other views too, which are needed elsewhere in the
site). And it could be made wide to have all of the rows from the source
tables. But it would, to me, feel like much better code to keep the
separate decomposed views and join them together for the booking query.
Right now, that query's performance suffers in a way that this feature
would fix. So the current choices are: accept worse performance with
decomposed views, write one very large and performant view but duplicate
some of the logic, or use their ORM to generate the SQL that they'd
normally put in a view.

On Thu, Mar 11, 2021 at 10:50 PM Tomas Vondra 
wrote:

> On 3/11/21 3:39 PM, Hywel Carver wrote:
> > Great! It looks like it's been in commitfest status for a few years. Is
> > there anything someone like me (outside the pgsql-hackers community) can
> > do to help it get reviewed this time around?
> >
>
> Well, you could do a review, or at least test it with the queries your
> application is actually running. And explain why your application is
> doing queries like this, and why it can't be changed to changed to not
> generate such queries.
>
> The first couple of messages from the patch thread [1] (particularly the
> messages from May 2018) are a good explanation why patches like this are
> tricky to get through.
>
> The basic assumption is that such queries are a bit silly, and it'd be
> probably easier to modify the application not to generate them instead
> of undoing the harm in the database planner. The problem is this makes
> the planner more expensive for everyone, including people who carefully
> write "good" queries.
>
>
> And we don't want to do that, so we need to find a way to make this
> optimization very cheap (essentially "free" if not applicable), but
> that's difficult because there may be cases where the self-joins are
> intentional, and undoing them would make the query slower. And doing
> good decision requires enough information, but this decision needs to
> happen quite early in the planning, when we have very little info.
>
> So while it seems like a simple optimization, it's actually quite tricky
> to get right.
>
> (Of course, there are cases where you may get such queries even if you
> try writing good SQL, say when joining views etc.)
>
> regards
>
> [1]
>
> https://www.postgresql.org/message-id/flat/64486b0b-0404-e39e-322d-080115490...@postgrespro.ru
>
> --
> Tomas Vondra
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

Re: [POC] verifying UTF-8 using SIMD instructions

[Amit Khandekar]

On Tue, 9 Mar 2021 at 17:14, John Naylor  wrote:
> On Tue, Mar 9, 2021 at 5:00 AM Amit Khandekar  wrote:
> > Just a quick question before I move on to review the patch ... The
> > improvement looks like it is only meant for x86 platforms.
>
> Actually it's meant to be faster for all platforms, since the C fallback is 
> quite a bit different from HEAD. I've found it to be faster on ppc64le. An 
> earlier version of the patch was a loser on 32-bit Arm because of alignment 
> issues, but if you could run the test script attached to [1] on 64-bit Arm, 
> I'd be curious to see how it does on 0002, and whether 0003 and 0004 make 
> things better or worse. If there is trouble building on non-x86 platforms, 
> I'd want to fix that also.

On my Arm64 VM :

HEAD :
 mixed | ascii
---+---
  1091 |   628
(1 row)

PATCHED :
 mixed | ascii
---+---
   681 |   119

So the fallback function does show improvements on Arm64.

I guess, if at all we use the equivalent Arm NEON intrinsics, the
"mixed" figures will be close to the "ascii" figures, going by your
figures on x86.

> > Can this be
> > done in a portable way by arranging for auto-vectorization ? Something
> > like commit 88709176236caf. This way it would benefit other platforms
> > as well.
>
> I'm fairly certain that the author of a compiler capable of doing that in 
> this case would be eligible for some kind of AI prize. :-)

:)

I was not thinking about auto-vectorizing the code in
pg_validate_utf8_sse42(). Rather, I was considering auto-vectorization
inside the individual helper functions that you wrote, such as
_mm_setr_epi8(), shift_right(), bitwise_and(), prev1(), splat(),
saturating_sub() etc. I myself am not sure whether it is feasible to
write code that auto-vectorizes all these function definitions.
saturating_sub() seems hard, but I could see the gcc docs mentioning
support for generating such instructions for a particular code loop.
But for the index lookup function() it seems impossible to generate
the  needed index lookup intrinsics. We can have platform-specific
function definitions for such exceptional cases.

I am considering this only because that would make the exact code work
on other platforms like arm64 and ppc, and won't have to have
platform-specific files. But I understand that it is easier said than
done. We will have to process the loop in pg_validate_utf8_sse42() in
128-bit chunks, and pass each chunk to individual functions, which
could mean extra work and extra copy in extracting the chunk data and
passing it around, which may make things drastically slow. You are
passing around the chunks using __m128i type, so perhaps it means
passing around just a reference to the simd registers. Not sure.


-- 
Thanks,
-Amit Khandekar
Huawei Technologies

Re: ?????? unrecognized configuration parameter "plpgsql.check_asserts"

[Walker]

thanks Julien  Pavel


it's crystal clear now. thanks again for your kindly help


thanks
walker




--Original--
From:   
 "Julien Rouhaud"   
 

Re: non-HOT update not looking at FSM for large tuple update

[Matthias van de Meent]

On Thu, 11 Mar 2021 at 16:16, John Naylor  wrote:
>
> On Thu, Mar 11, 2021 at 9:46 AM Matthias van de Meent 
>  wrote:
>
> > Regarding the 2% slack logic, could we change it to use increments of
> > line pointers instead? That makes it more clear what problem this
> > solution is trying to work around; that is to say line pointers not
> > (all) being truncated away. The currently subtracted value accounts
>
> That makes sense.
>
> > ...
> > - if (len + saveFreeSpace > MaxHeapTupleSize)
> > + if (len + saveFreeSpace > maxPaddedFsmRequest)
> > ...
> > - targetFreeSpace = Max(len, MaxHeapTupleSize - (MaxHeapTupleSize * 2 / 
> > 100));
> > + targetFreeSpace = Max(len, maxPaddedFsmRequest);
> > ...
>
> If we have that convenient constant, it seems equivalent (I think) and a bit 
> more clear to write it this way, but I'm not wedded to it:
>
> if (len + saveFreeSpace > MaxHeapTupleSize &&
> len <= maxPaddedFsmRequest)
> {
> ...
> targetFreeSpace = maxPaddedFsmRequest;
> }

+ else if (len > maxPaddedFsmRequest
+ {
+   /* request len amount of space; it might still fit on
not-quite-empty pages */
+   targetFreeSpace = len;
+ }

If this case isn't added, the lower else branch will fail to find
fitting pages for len > maxPaddedFsmRequest tuples; potentially
extending the relation when there is actually still enough space
available.

> else
> targetFreeSpace = len + saveFreeSpace;

> Also, should I write a regression test for it? The test case is already 
> available, just no obvious place to put it.

I think it would be difficult to write tests that exhibit the correct
behaviour on BLCKSZ != 8196. On the other hand, I see there are some
tests that explicitly call out that they expect BLCKSZ to be 8192, so
that has not really been a hard block before.

The previous code I sent had initial INSERT + DELETE + VACUUM. These
statements can be replaced with `INSERT INTO t_failure (b) VALUES
(repeat('1', 95)); VACUUM;` for simplicity. The vacuum is still needed
to populate the FSM for the new page.

With regards,

Matthias van de Meent

Re: MultiXact\SLRU buffers configuration

[Andrey Borodin]

> 11 марта 2021 г., в 20:50, Gilles Darold  написал(а):
> 
> 
> The patch doesn't apply anymore in master cause of error: patch failed: 
> src/backend/utils/init/globals.c:150
> 
> 
> 
> An other remark about this patch is that it should be mentionned in the 
> documentation (doc/src/sgml/config.sgml) that the new configuration variables 
> need a server restart, for example by adding "This parameter can only be set 
> at server start." like for shared_buffers. Patch on postgresql.conf mention 
> it.
> 
> And some typo to be fixed:
> 
> 
> 
> s/Tipically/Typically/
> 
> s/asincronous/asyncronous/
> 
> s/confugured/configured/
> 
> s/substrnsactions/substransactions/
> 
> 

Thanks, Gilles! Fixed.

Best regards, Andrey Borodin.


v10-0001-Make-all-SLRU-buffer-sizes-configurable.patch
Description: Binary data

Re: Do we support upgrade of logical replication?

[Amit Kapila]

On Wed, Mar 10, 2021 at 4:03 PM vignesh C  wrote:
>
> Hi,
>
> I was reviewing logical decoding of two-phase transactions feature,
> while reviewing the feature I was checking if there is any impact on
> publisher/subscriber upgrade.
>
> I checked the existing pg_upgrade behaviour with logical replication.
> I made a logical replication data instance with publisher and
> subscriber with subscriber subscribed to a table.  Then I tried
> upgrading publisher and subscriber individually. After upgrade I
> noticed the following:
>
> 1) Pg_logical/mappings files were not copied in the upgraded data folder:
> --
> Pg_logical contents in old data folder:
> publisher/pg_logical/replorigin_checkpoint
> publisher/pg_logical/mappings:
> map-32cb-4df-0_1767088-225-225
> publisher/pg_logical/snapshots:
> 0-1643650.snap
>
> New upgraded data folder:
> publisher1/pg_logical/replorigin_checkpoint
> publisher1/pg_logical/mappings:
> publisher1/pg_logical/snapshots:
>
> 2) Replication slots were not copied:
> select * from pg_replication_slots;
> slot_name | plugin | slot_type | datoid | database | temporary |
> active | active_pid | xmin | catalog_xmin | restart_lsn |
> confirmed_flush_lsn | wal_status | safe_wal_size | t
> wo_phase
> ---++---++--+---+++--+--+-+-++---+--
> -
> (0 rows)
>
> 3) The subscriber is in disabled mode in the upgraded data:
> select * from pg_subscription;
>   oid  | subdbid | subname | subowner | subenabled | subbinary |
> substream | subtwophase |   subconninfo|
> subslotname | subsynccommit | subpublicati
> ons
> ---+-+-+--++---+---+-+--+-+---+-
> 
> 16404 |   16401 | mysub   |   10 | f  | f | f
>| f   | host=localhost port=5432 dbname=postgres | mysub
>| off   | {mypub}
> (1 row)
>
> 4) The pg_subscription_rel contents also were not copied:
> select * from pg_subscription_rel;
>  srsubid | srrelid | srsubstate | srsublsn
> -+-++--
> (0 rows)
>
> 5) While logical decoding of transactions, the decoded changes will be
> serialized based on logical_decoding_work_mem configuration. Even
> these files were not copied during upgrade.
>
> Do we support upgrading of logical replication, if it is supported
> could someone point me to the document link on how to upgrade logical
> replication?
>

As far as I can understand, the main reason we don't copy all these
things is that we can't retain the slots after upgrade. I think the
reason for the same is that we reset WAL during upgrade and slots
might point to some old WAL location. Now, one might think that we can
try to copy WAL as well to allow slots to work after upgrade but the
WAL format can change in newer version so that might be tricky.

So users need to probably recreate the slots and then perform the
tablesync again via Alter Subscription ... Refresh Publication. Also,
that might require truncating the previous data. I am also not very
sure about the procedure but maybe someone else can correct me or add
more to it.

-- 
With Regards,
Amit Kapila.

Re: 回复： unrecognized configuration parameter "plpgsql.check_asserts"

[Julien Rouhaud]

On Fri, Mar 12, 2021 at 08:12:53PM +0800, Walker wrote:
> To get rid of --enable-cassert while configuring, debug_assertions is shown 
> as off. In this case, ASSERT statement also can't be used, right?

No, those are two different things.  plpgsql ASSERT are only controlled by
plpgsql.check_asserts configuration option, whether the server were compiled
with or without --enable-cassert.

Re: unrecognized configuration parameter "plpgsql.check_asserts"

[Pavel Stehule]

pá 12. 3. 2021 v 13:13 odesílatel Walker  napsal:

> Hi, Pavel
>
> Thanks for your comments.
>
> To get rid of --enable-cassert while configuring, debug_assertions is
> shown as off. In this case, ASSERT statement also can't be used, right?
>

no - debug assertions and plpgsql assertions are two absolutely independent
features.

--enable-cessart enables C assertions, and then needs postgres's source
code compilation. It is designed and used by Postgres's core developers.

plpgsql's ASSERT is user space feature, and can be enabled or disabled how
it is necessary by plpgsql.check_assert.


> when enable --enable-cassert, can we use plpgsql.check_asserts to control
> ASSERT statement, and how?
>
> thanks
> walker
>
> -- 原始邮件 --
> *发件人:* "Pavel Stehule" ;
> *发送时间:* 2021年3月12日(星期五) 晚上7:11
> *收件人:* "Walker";
> *抄送:* "pgsql-hackers";
> *主题:* Re: unrecognized configuration parameter "plpgsql.check_asserts"
>
>
>
> pá 12. 3. 2021 v 11:54 odesílatel Walker  napsal:
>
>> Hi, hackers
>>
>> Due to configure with parameter --enable-cassert, the debug_assertions is
>> on by default, as follows:
>> postgres=# show debug_assertions;
>> debug_assertions
>> ---
>> on
>>
>> Because of pgbench performance testing, I need to disable the assert
>> function. Following the doc below, I tried to set plpgsql.check_asserts to
>> off to disable assert function.
>> https://www.postgresql.org/docs/13/plpgsql-errors-and-messages.html
>>
>> However, it prompts the following error, not sure if I missed something,
>> any thoughts about it?
>> postgres=# alter system set plpgsql.check_asserts = off;
>> EORROR: unrecognized configuration parameter "plpgsql.check_asserts"
>>
>
> you cannot disable debug_assertions. It is possible just by configure, and
> make
>
> plpgsql.check_asserts controls evaluation of plpgsql statement ASSERT
>
> Pavel
>
>
>
>> env:
>> PG: 13.2
>> OS: redhat 7.4 3.10.0-693.17.1.e17.x86_64
>> configure parameter: --enable-coverage --enable-tap-tests
>> --enable-cassert --enable-debug --enable-nls --with-perl --with-python
>> --with-tcl --with-openssl --with-ldap --with-libxml --with-libxslt
>> --with-uuid=e2fs --with-segsize=10 --with-wal-blocksize=16 --with-llvm
>> LLVM_CONFIG=xxx CLANG=xxx
>>
>>
>> thanks
>> walker
>>
>>
>>

?????? unrecognized configuration parameter "plpgsql.check_asserts"

[Walker]

Hi, Pavel


Thanks for your comments.


To get rid of --enable-cassert while configuring, debug_assertions is shown as 
off. In this case, ASSERT statement also can't be used, right?


when enable --enable-cassert, can we use plpgsql.check_asserts to control 
ASSERT statement, and how?



thanks
walker


----
??: 
   "Pavel Stehule"  
  
https://www.postgresql.org/docs/13/plpgsql-errors-and-messages.html


However, it prompts the following error, not sure if I missed something, any 
thoughts about it?
postgres=# alter system set plpgsql.check_asserts = off;
EORROR:unrecognized configuration parameter "plpgsql.check_asserts"


you cannot disable debug_assertions. It is possible just by configure, and make 



plpgsql.check_asserts controls evaluation of plpgsql statement ASSERT


Pavel



 



env:
PG: 13.2
OS: redhat 7.4 3.10.0-693.17.1.e17.x86_64
configure parameter: --enable-coverage --enable-tap-tests --enable-cassert 
--enable-debug --enable-nls --with-perl --with-python --with-tcl --with-openssl 
--with-ldap --with-libxml --with-libxslt --with-uuid=e2fs --with-segsize=10 
--with-wal-blocksize=16 --with-llvm LLVM_CONFIG=xxx CLANG=xxx




thanks
walker

Re: Parallel INSERT (INTO ... SELECT ...)

[Amit Kapila]

On Fri, Mar 12, 2021 at 1:33 PM houzj.f...@fujitsu.com
 wrote:
>
> > > The problem is that target_rel_trigger_max_parallel_hazard and its
> > > caller think they can use a relcache TriggerDesc field across other
> > > cache accesses, which they can't because the relcache doesn't
> > > guarantee that that won't move.
> > >
> > > One approach would be to add logic to RelationClearRelation similar to
> > > what it does for tupdescs, rules, etc, to avoid moving them when their
> > > contents haven't changed.  But given that we've not needed that for
> > > the past several decades, I'm disinclined to add the overhead.  I
> > > think this code ought to be adjusted to not make its own copy of the
> > > trigdesc pointer, but instead fetch it out of the relcache struct each
> > > time it is accessed.  There's no real reason why
> > > target_rel_trigger_max_parallel_hazard shouldn't be passed the
> > > (stable) Relation pointer instead of just the trigdesc pointer.
> > >
> >
> > I have attached a patch to fix the issue, based on your suggestion (tested 
> > with
> > CLOBBER_CACHE_ALWAYS defined).
> >
> > > BTW, having special logic for FK triggers in
> > > target_rel_trigger_max_parallel_hazard seems quite loony to me.
> > > Why isn't that handled by setting appropriate proparallel values for
> > > those trigger functions?
> > >
> >
> > ... and also attached a patch to update the code for this issue.
> >
> > (2nd patch relies on application of the 1st patch)
> >
> > Thanks again for pointing out these problems.
>
> I have tested the triggerdesc bugfix patch with CLOBBER_CACHE_ALWAYS flag.
> It passed the testset where is fail in buildfarm (foreign_key, foreign_data).
>

Thanks for the patch and review. It looks good to me as well and
passes the tests (foreign_key, foreign_data) with CLOBBER_CACHE_ALWAYS
flag.

I'll review the second patch of Greg.

-- 
With Regards,
Amit Kapila.

Re: [Patch] Optimize dropping of relation buffers using dlist

[Amit Kapila]

On Fri, Mar 12, 2021 at 12:07 PM tsunakawa.ta...@fujitsu.com
 wrote:
>
> From: Kyotaro Horiguchi 
> > About the patch, it would be better to change the type of
> > BUF_DROP_FULL_SCAN_THRESHOLD to uint64, even though the current
> > value
> > doesn't harm.
>
> OK, attached, to be prepared for the distant future when NBuffers becomes 
> 64-bit.
>

Thanks for the patch. Pushed!

-- 
With Regards,
Amit Kapila.

Re: unrecognized configuration parameter "plpgsql.check_asserts"

[Pavel Stehule]

pá 12. 3. 2021 v 11:54 odesílatel Walker  napsal:

> Hi, hackers
>
> Due to configure with parameter --enable-cassert, the debug_assertions is
> on by default, as follows:
> postgres=# show debug_assertions;
> debug_assertions
> ---
> on
>
> Because of pgbench performance testing, I need to disable the assert
> function. Following the doc below, I tried to set plpgsql.check_asserts to
> off to disable assert function.
> https://www.postgresql.org/docs/13/plpgsql-errors-and-messages.html
>
> However, it prompts the following error, not sure if I missed something,
> any thoughts about it?
> postgres=# alter system set plpgsql.check_asserts = off;
> EORROR: unrecognized configuration parameter "plpgsql.check_asserts"
>

you cannot disable debug_assertions. It is possible just by configure, and
make

plpgsql.check_asserts controls evaluation of plpgsql statement ASSERT

Pavel



> env:
> PG: 13.2
> OS: redhat 7.4 3.10.0-693.17.1.e17.x86_64
> configure parameter: --enable-coverage --enable-tap-tests --enable-cassert
> --enable-debug --enable-nls --with-perl --with-python --with-tcl
> --with-openssl --with-ldap --with-libxml --with-libxslt --with-uuid=e2fs
> --with-segsize=10 --with-wal-blocksize=16 --with-llvm LLVM_CONFIG=xxx
> CLANG=xxx
>
>
> thanks
> walker
>
>
>

Re: Allow batched insert during cross-partition updates

[gkokolatos]

‐‐‐ Original Message ‐‐‐
On Friday, March 12, 2021 3:45 AM, Amit Langote  wrote:

> On Thu, Mar 11, 2021 at 8:36 PM gkokola...@pm.me wrote:
>
> > On Thursday, March 11, 2021 9:42 AM, Amit Langote amitlangot...@gmail.com 
> > wrote:
> >
> > > On Wed, Mar 10, 2021 at 9:30 PM Georgios gkokola...@protonmail.com wrote:
> > >
> > > > I continued looking a bit at the patch, yet I am either failing to see 
> > > > fix or I am
> > > > looking at the wrong thing. Please find attached a small repro of what 
> > > > my expectetions
> > > > were.
> > > > As you can see in the repro, I would expect the
> > > > UPDATE local_root_remote_partitions SET a = 2;
> > > > to move the tuples to remote_partition_2 on the same transaction.
> > > > However this is not the case, with or without the patch.
> > > > Is my expectation of this patch wrong?
> > >
> > > I think yes. We currently don't have the feature you are looking for
> > > -- moving tuples from one remote partition to another remote
> > > partition. This patch is not for adding that feature.
> >
> > Thank you for correcting me.
> >
> > > What we do support however is moving rows from a local partition to a
> > > remote partition and that involves performing an INSERT on the latter.
> > > This patch is for teaching those INSERTs to use batched mode if
> > > allowed, which is currently prohibited. So with this patch, if an
> > > UPDATE moves 10 rows from a local partition to a remote partition,
> > > then they will be inserted with a single INSERT command containing all
> > > 10 rows, instead of 10 separate INSERT commands.
> >
> > So, if I understand correctly then in my previously attached repro I
> > should have written instead:
> >
> > CREATE TABLE local_root_remote_partitions (a int) PARTITION BY LIST ( a 
> > );
> > CREATE TABLE
> > local_root_local_partition_1
> > PARTITION OF
> > local_root_remote_partitions FOR VALUES IN (1);
> >
> > CREATE FOREIGN TABLE
> > local_root_remote_partition_2
> > PARTITION OF
> > local_root_remote_partitions FOR VALUES IN (2)
> > SERVER
> > remote_server
> > OPTIONS (
> > table_name 'remote_partition_2',
> > batch_size '10'
> > );
> >
> > INSERT INTO local_root_remote_partitions VALUES (1), (1);
> > -- Everything should be on local_root_local_partition_1 and on the same 
> > transaction
> > SELECT ctid, xmin, xmax, cmax, tableoid::regclass, a FROM 
> > local_root_remote_partitions;
> >
> > UPDATE local_root_remote_partitions SET a = 2;
> > -- Everything should be on remote_partition_2 and on the same 
> > transaction
> > SELECT ctid, xmin, xmax, cmax, tableoid::regclass, a FROM 
> > local_root_remote_partitions;
> >
> >
> > I am guessing that I am still wrong because the UPDATE operation above will
> > fail due to the restrictions imposed in postgresBeginForeignInsert regarding
> > UPDATES.
>
> Yeah, for the move to work without hitting the restriction you
> mention, you will need to write the UPDATE query such that
> local_root_remote_partition_2 is not updated. For example, as
> follows:
>
> UPDATE local_root_remote_partitions SET a = 2 WHERE a <> 2;
>
> With this query, the remote partition is not one of the result
> relations to be updated, so is able to escape that restriction.

Excellent. Thank you for the explanation and patience.

>
> > Would it be too much to ask for the addition of a test case that will
> > demonstrate the change of behaviour found in patch.
>
> Hmm, I don't think there's a way to display whether the INSERT done on
> the remote partition as a part of an (tuple-moving) UPDATE used
> batching or not. That's because that INSERT's state is hidden from
> EXPLAIN. Maybe we should change EXPLAIN to make it show such hidden
> INSERT's state (especially its batch size) under the original UPDATE
> node, but I am not sure.

Yeah, there does not seem to be a way for explain to do show that information
with the current code.

>
> By the way, the test case added by commit 927f453a94106 does exercise
> the code added by this patch, but as I said in the last paragraph, we
> can't verify that by inspecting EXPLAIN output.

I never doubted that. However, there is a difference. The current patch
changes the query to be executed in the remote from:

   INSERT INTO  VALUES ($1);
to:
   INSERT INTO  VALUES ($1), ($2) ... ($n);

When this patch gets in, it would be very helpful to know that subsequent
code changes will not cause regressions. So I was wondering if there is
a way to craft a test case that would break for the code in 927f453a94106
yet succeed with the current patch.

I attach version 2 of my small reproduction. I am under the impression that
in this version, examining the value of cmin in the remote table should
give an indication of whether the remote received a multiple insert queries
with a single value, or a single insert query with multiple values.

Or is this a wrong assumption of mine?

unrecognized configuration parameter "plpgsql.check_asserts"

[Walker]

Hi, hackers


Due to configure with parameter --enable-cassert, the debug_assertions is on by 
default, as follows:
postgres=# show debug_assertions;
debug_assertions
---
on


Because of pgbench performance testing, I need to disable the assert function. 
Following the doc below, I tried to set plpgsql.check_asserts to off to disable 
assert function.
https://www.postgresql.org/docs/13/plpgsql-errors-and-messages.html


However, it prompts the following error, not sure if I missed something, any 
thoughts about it?
postgres=# alter system set plpgsql.check_asserts = off;
EORROR:unrecognized configuration parameter "plpgsql.check_asserts"


env:
PG: 13.2
OS: redhat 7.4 3.10.0-693.17.1.e17.x86_64
configure parameter: --enable-coverage --enable-tap-tests --enable-cassert 
--enable-debug --enable-nls --with-perl --with-python --with-tcl --with-openssl 
--with-ldap --with-libxml --with-libxslt --with-uuid=e2fs --with-segsize=10 
--with-wal-blocksize=16 --with-llvm LLVM_CONFIG=xxx CLANG=xxx




thanks
walker

Re: [HACKERS] Custom compression methods

[Justin Pryzby]

I think these names need to be more specific.

+typedef enum CompressionId
+{
+PGLZ_COMPRESSION_ID = 0,
+LZ4_COMPRESSION_ID = 1
+

CompressionId, PGLZ_COMPRESSION_ID, LZ4_COMPRESSION_ID are also being used by
Andrey's WAL compression patch.  I suggested he use a prefix, but your patch is
also of limited scope (TOAST).

-- 
Justin

Re: pgbench - add pseudo-random permutation function

[Fabien COELHO]

Hello Dean,


The implementation looks plausible too, though it adds quite a large
amount of new code.


A significant part of this new code the the multiply-modulo 
implementation, which can be dropped if we assume that the target has 
int128 available, and accept that the feature is not available otherwise.

Also, there are quite a lot of comments which add to the code length.

The main thing that concerns me is justifying the code. With this kind 
of thing, it's all too easy to overlook corner cases and end up with 
trivial sequences in certain special cases. I'd feel better about that 
if we were implementing a standard algorithm with known pedigree.


Yep. I did not find anything convincing with the requirements: generate a 
permutation, can be parametric, low constant cost, good quality, work on 
arbitrary sizes…



Thinking about the use case for this, it seems that it's basically
designed to turn a set of non-uniform random numbers (produced by
random_exponential() et al.) into another set of non-uniform random
numbers, where the non-uniformity is scattered so that the more/less
common values aren't all clumped together.


Yes.


I'm wondering if that's something that can't be done more simply by
passing the non-uniform random numbers through the uniform random
number generator to scatter them uniformly across some range -- e.g.,
given an integer n, return the n'th value from the sequence produced
by random(), starting from some initial seed -- i.e., implement
nth_random(lb, ub, seed, n). That would actually be pretty
straightforward to implement using O(log(n)) time to execute (see the
attached python example), though it wouldn't generate a permutation,
so it'd need a bit of thought to see if it met the requirements.


Indeed, this violates two requirements: constant cost & permutation.

--
Fabien.

Re: [HACKERS] Custom compression methods

[Dilip Kumar]

On Fri, Mar 12, 2021 at 2:12 PM Dilip Kumar  wrote:
>
> On Fri, Mar 12, 2021 at 10:45 AM Justin Pryzby  wrote:
> >
> > On Thu, Mar 11, 2021 at 12:25:26PM -0600, Justin Pryzby wrote:
> > > On Wed, Mar 10, 2021 at 08:28:58PM -0600, Justin Pryzby wrote:
> > > > This includes a patch to use pkgconfig, in an attempt to build on mac, 
> > > > which
> > > > currently fails like:
> > > >
> > > > https://cirrus-ci.com/task/5993712963551232?command=build#L126
> > > > checking for LZ4_compress in -llz4... no
> > > > configure: error: library 'lz4' is required for LZ4 support
> > >
> > > This includes a 2nd attempt to use pkg-config to build on mac.
> > >
> > > If this doesn't work, we should ask for help from a mac user who wants to 
> > > take
> > > on a hopefully-quick project.
> >
> > The 2nd attempt passed ./configure on mac (and BSD after Thomas installed
> > pkg-config), but I eventually realized that LZ4 was effectively disabled,
> > because we set HAVE_LZ4, but the code tested instead WITH_LIBLZ4.
>
> With this patch, I see USE_LZ4 is never defined in my centos
> machine(even --with-lz4), however it was working fine without the 0005
> patch.  I will have a look why it is behaving like this so I will not
> include these changes until I figure out what is going on.

Just realized I was still checking for HAVE_LIBLZ4 not USE_LZ4, sorry
for the noise its working fine.  And thanks for making it work for
mac.

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

Re: [HACKERS] logical decoding of two-phase transactions

[vignesh C]

On Fri, Mar 12, 2021 at 2:29 PM Peter Smith  wrote:
>
> On Fri, Mar 12, 2021 at 4:07 PM vignesh C  wrote:
>
> Hi Vignesh,
>
> Thanks for the review comments.
>
> But can you please resend it with each feedback enumerated as 1. 2.
> 3., or have some other clear separation for each comment.
>
> (Because everything is mushed together I am not 100% sure if your
> comment text applies to the code above or below it)

1) I felt twophase_given can be a local variable, it need not be added
as a function parameter as it is not used outside the function.
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -67,7 +67,8 @@ parse_subscription_options(List *options,
   char **synchronous_commit,
   bool *refresh,
   bool *binary_given,
bool *binary,
-  bool
*streaming_given, bool *streaming)
+  bool
*streaming_given, bool *streaming,
+  bool
*twophase_given, bool *twophase)

The corresponding changes should be done here too:
@@ -358,6 +402,8 @@ CreateSubscription(CreateSubscriptionStmt *stmt,
bool isTopLevel)
boolcopy_data;
boolstreaming;
boolstreaming_given;
+   booltwophase;
+   booltwophase_given;
char   *synchronous_commit;
char   *conninfo;
char   *slotname;
@@ -382,7 +428,8 @@ CreateSubscription(CreateSubscriptionStmt *stmt,
bool isTopLevel)
   _commit,
   NULL,
 /* no "refresh" */

_given, ,
-
_given, );
+
_given, ,
+
_given, );

2) I think this is not possible as we don't allow changing twophase
option, should this be an assert.
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -2930,6 +2930,7 @@ maybe_reread_subscription(void)
strcmp(newsub->slotname, MySubscription->slotname) != 0 ||
newsub->binary != MySubscription->binary ||
newsub->stream != MySubscription->stream ||
+   newsub->twophase != MySubscription->twophase ||
!equal(newsub->publications, MySubscription->publications))

3) We have the following check in parse_subscription_options:
if (twophase && *twophase_given && *twophase)
{
if (streaming && *streaming_given && *streaming)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("%s and %s are mutually exclusive options",
"two_phase = true", "streaming = true")));
}

Should we have a similar check in parse_output_parameters?
@@ -252,6 +254,16 @@ parse_output_parameters(List *options, uint32
*protocol_version,

*enable_streaming = defGetBoolean(defel);
}
+   else if (strcmp(defel->defname, "two_phase") == 0)
+   {
+   if (twophase_given)
+   ereport(ERROR,
+   (errcode(ERRCODE_SYNTAX_ERROR),
+errmsg("conflicting
or redundant options")));
+   twophase_given = true;
+
+   *enable_twophase = defGetBoolean(defel);
+   }

Regard,
Vignesh

RE: Parallel INSERT (INTO ... SELECT ...)

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

> On Fri, Mar 12, 2021 at 6:10 AM Justin Pryzby  
> wrote:
> > Note also this CF entry
> > https://commitfest.postgresql.org/32/2987/
> > | Allow setting parallel_workers on partitioned tables
> 
> +/*
> + * PartitionedOptions
> + * Contents of rd_options for partitioned tables
> + */
> +typedef struct PartitionedOptions
> +{
> +   int32   vl_len_;/* varlena header (do not touch directly!) */
> +   boolparallel_insert_enabled;/* enables planner's use
> of parallel insert */
> +} PartitionedOptions;
> 
> houzj, could you please consider naming the struct PartitionedTableRdOptions
> as the patch for adding parallel_workers option to partitioned tables does?

Thanks for reminding.
I agreed that " PartitionedTableRdOptions " is better.

Attaching new version patch with this change.

Best regards,
houzj


v27-0003-Parallel-SELECT-for-INSERT-INTO-.-SELECT-advanced-tests.patch
Description:  v27-0003-Parallel-SELECT-for-INSERT-INTO-.-SELECT-advanced-tests.patch


v27-0002-Add-new-GUC-option-enable_parallel_insert-boolean.patch
Description:  v27-0002-Add-new-GUC-option-enable_parallel_insert-boolean.patch

Re: documentation fix for SET ROLE

[Laurenz Albe]

On Thu, 2021-03-11 at 22:30 +, Bossart, Nathan wrote:
> On 3/11/21, 12:11 PM, "David G. Johnston"  wrote:
> > The minor bit of documentation pseudo-redundancy doesn’t bother me if I 
> > accept
> >  they are there own separate thing.  The fact that set role and set session
> >  authorization are entirely distinct top-level commands in our 
> > documentation,
> >  as opposed to bundled in with plain set, is a much more convincing example
> >  for treating them uniquely and not just additional GUCs.
> 
> I see your point.  What do you think about something like the attached
> patch?

After sleeping on it, I have come to think that it is excessive to write
so much documentation for a feature that is that unimportant.

It takes some effort to come up with a good use case for it.

I think we can add a few lines to ALTER ROLE, perhaps ALTER DATABASE
(although I don't see what sense it could make to set that on the database 
level),
and briefly explain the difference between RESET ROLE and SET ROLE NONE.

I think adding too much detail will harm - anyone who needs to know the
exact truth can resort to the implementation.

I'll try to come up with a proposal later.

Yours,
Laurenz Albe

Re: Removing unneeded self joins

[Hywel Carver]

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

I've built and tested this, and it seems to function correctly to me. One 
question I have is whether the added "IS NOT NULL" filters can be omitted when 
they're unnecessary. Some of the resulting plans included an "IS NOT NULL" 
filter on a non-nullable column. To be clear, this is still an improvement (to 
me) without that.

Here's the simple test script I ran, on master ("before") and with the latest 
patch applied ("after").

CREATE TABLE users (id BIGINT PRIMARY KEY, nullable_int BIGINT UNIQUE, 
some_non_nullable_int BIGINT NOT NULL);
CREATE VIEW only_some_users AS (SELECT * FROM users WHERE id < 10);
CREATE VIEW some_other_users AS (SELECT * FROM users WHERE id > 3);

EXPLAIN SELECT *
FROM users u1
INNER JOIN users u2 
  ON u1.id = u2.id;
-- before does HASH JOIN
-- after does seq scan with "id IS NOT NULL" condition

EXPLAIN SELECT *
FROM only_some_users
INNER JOIN some_other_users
  ON only_some_users.id = some_other_users.id;
-- before does HASH JOIN
-- after does no JOIN, instead does scan, with an extra "id IS NOT NULL 
condition" (in addition to id < 10, id > 3)

EXPLAIN SELECT *
FROM users u1
INNER JOIN users u2 
  ON u1.nullable_int = u2.nullable_int;
-- before does HASH JOIN
-- after does scan with (nullable_int IS NOT NULL) filter

EXPLAIN SELECT *
FROM users u1
INNER JOIN users u2 
  ON u1.id = u2.nullable_int;
-- before does HASH JOIN
-- after correctly unchanged

EXPLAIN SELECT *
FROM users u1
INNER JOIN users u2 
  ON u1.id = u2.some_non_nullable_int
INNER JOIN users u3 
  ON u2.some_non_nullable_int = u3.id;
-- before does 2x HASH JOIN
-- now does 1x HASH JOIN, with a sequential scan over users filtered by id IS 
NOT NULL

Re: [HACKERS] logical decoding of two-phase transactions

[Peter Smith]

On Fri, Mar 12, 2021 at 4:07 PM vignesh C  wrote:

Hi Vignesh,

Thanks for the review comments.

But can you please resend it with each feedback enumerated as 1. 2.
3., or have some other clear separation for each comment.

(Because everything is mushed together I am not 100% sure if your
comment text applies to the code above or below it)

TIA.


Kind Regards,
Peter Smith.
Fujitsu Australia

Re: HotStandbyActive() issue in postgres

[Fujii Masao]

On 2021/03/12 11:14, Hao Wu wrote:

Hi hackers,

When we enable hot standby, HotStandbyActive() returns true on hot standby.
Then, we promote the hot standby, the SHM variable 
`XLogCtl->SharedHotStandbyActive`
remains true. So, HotStandbyActive() still returns true until the next call of
`XLOGShmemInit()` even if the data node was promoted.
`XLogWalRcvSendHSFeedback()` is the only caller of HotStandbyActive,
it's probably not covered by the test cases.

Is it the expected behavior or a bug in postgres? Probably a bug.
I haven't much knowledge of hot-standby, a simple fix might be
to set XLogCtl->SharedHotStandbyActive to false when
the recovery process almost finishes. See the attachment.


So if walreceiver is only user of HotStandbyActive(), which means that there is 
no user of it after recovery finishes because walreceiver exits at the end of 
recovery? If this understanding is right, ISTM that HotStandbyActive() doesn't 
need to return false after recovery finishes because there is no user of it. No?

Or you're implementing something that uses HotStandbyActive(), so want it to 
return false after the recovery?

Regards,

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

Re: [HACKERS] Custom compression methods

[Dilip Kumar]

On Fri, Mar 12, 2021 at 10:45 AM Justin Pryzby  wrote:
>
> On Thu, Mar 11, 2021 at 12:25:26PM -0600, Justin Pryzby wrote:
> > On Wed, Mar 10, 2021 at 08:28:58PM -0600, Justin Pryzby wrote:
> > > This includes a patch to use pkgconfig, in an attempt to build on mac, 
> > > which
> > > currently fails like:
> > >
> > > https://cirrus-ci.com/task/5993712963551232?command=build#L126
> > > checking for LZ4_compress in -llz4... no
> > > configure: error: library 'lz4' is required for LZ4 support
> >
> > This includes a 2nd attempt to use pkg-config to build on mac.
> >
> > If this doesn't work, we should ask for help from a mac user who wants to 
> > take
> > on a hopefully-quick project.
>
> The 2nd attempt passed ./configure on mac (and BSD after Thomas installed
> pkg-config), but I eventually realized that LZ4 was effectively disabled,
> because we set HAVE_LZ4, but the code tested instead WITH_LIBLZ4.

With this patch, I see USE_LZ4 is never defined in my centos
machine(even --with-lz4), however it was working fine without the 0005
patch.  I will have a look why it is behaving like this so I will not
include these changes until I figure out what is going on.

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

Re: shared-memory based stats collector

[Kyotaro Horiguchi]

At Fri, 12 Mar 2021 15:13:15 +0900, Fujii Masao  
wrote in 
> On 2021/03/12 13:49, Kyotaro Horiguchi wrote:
> > I noticed that I accidentally removed the launch-suppression feature
> > that is to avoid frequent relaunching.  That mechanism is needed on
> > the postmaster side. I added PgArchIsSuppressed() to do the same check
> > with the old pgarch_start() and make it called as a part of
> > PgArchStartupAllowed().
> 
> You're right! At least for me the function name PgArchIsSuppressed()
> sounds not good to me. What about something like PgArchCanRestart()?

The reason for the name was some positive-meaning names came up with
me are confusing with PgArchStartupAllowed().  The name
PgArchCanRestart suggests that it's usable only when
restarting. However, the function requires to be called also when the
first launch since last_pgarch_start_time needs to be updated every
time archiver is launched.

Anyway in the attached the name is changed wihtout changing its usage.

# I don't like it uses "can" as "allowed" so much. The archiver
# actually can restart but is just inhibited to restart.

> This is not fault of this patch. But last_pgarch_start_time should be
> initialized with zero?

Right. I noticed that but forgot to fix it.

> + if ((curtime - last_pgarch_start_time) < PGARCH_RESTART_INTERVAL)
> + return true;
> 
> Why did you remove the cast to unsigned int there?

The cast converts small negative values to large numbers, the code
looks like intending to allow archiver launched when curtime goes
behind than last_pgarch_start_time. That is the case the on-memory
data is corrupt. I'm not sure it's worth worrying and in the first
place if we want to care of the case we should explicitly compare the
operands of the subtraction.  I did that in the attached.

And the last_pgarch_start_time is accessed only in the function. I
moved it to inside the function.

> + /*
> + * Advertise our latch that backends can use to wake us up while
> we're
> +  * sleeping.
> +  */
> + PgArch->pgprocno = MyProc->pgprocno;
> 
> The comment should be updated?

Hmm. What is advertised is our pgprocno.. Fixed.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
>From f8880d60ad17351ba2131f4a2b0e0810f1eb7ed7 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi 
Date: Thu, 11 Mar 2021 18:01:30 +0900
Subject: [PATCH v55 3/3] Make archiver process an auxiliary process

This makes it possible to archiver process uses ipc functions and let
us monitor archiver process status.
---
 doc/src/sgml/monitoring.sgml |   1 +
 src/backend/access/transam/xlogarchive.c |   4 +-
 src/backend/bootstrap/bootstrap.c|  22 ++-
 src/backend/postmaster/pgarch.c  | 240 ++-
 src/backend/postmaster/postmaster.c  |  81 
 src/backend/storage/ipc/ipci.c   |   2 +
 src/include/miscadmin.h  |   2 +
 src/include/postmaster/pgarch.h  |  14 +-
 src/include/storage/pmsignal.h   |   1 -
 src/include/storage/proc.h   |   8 +-
 src/tools/pgindent/typedefs.list |   1 +
 11 files changed, 164 insertions(+), 212 deletions(-)

diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 1ba813bbb9..a96455bdb2 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -935,6 +935,7 @@ postgres   27093  0.0  0.0  30096  2752 ?Ss   11:34   0:00 postgres: ser
logical replication worker,
parallel worker, background writer,
client backend, checkpointer,
+   archiver,
startup, walreceiver,
walsender and walwriter.
In addition, background workers registered by extensions may have
diff --git a/src/backend/access/transam/xlogarchive.c b/src/backend/access/transam/xlogarchive.c
index 1c5a4f8b5a..26b023e754 100644
--- a/src/backend/access/transam/xlogarchive.c
+++ b/src/backend/access/transam/xlogarchive.c
@@ -25,11 +25,11 @@
 #include "common/archive.h"
 #include "miscadmin.h"
 #include "postmaster/startup.h"
+#include "postmaster/pgarch.h"
 #include "replication/walsender.h"
 #include "storage/fd.h"
 #include "storage/ipc.h"
 #include "storage/lwlock.h"
-#include "storage/pmsignal.h"
 
 /*
  * Attempt to retrieve the specified file from off-line archival storage.
@@ -491,7 +491,7 @@ XLogArchiveNotify(const char *xlog)
 
 	/* Notify archiver that it's got something to do */
 	if (IsUnderPostmaster)
-		SendPostmasterSignal(PMSIGNAL_WAKEN_ARCHIVER);
+		PgArchWakeup();
 }
 
 /*
diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.c
index 6f615e6622..41da0c5059 100644
--- a/src/backend/bootstrap/bootstrap.c
+++ b/src/backend/bootstrap/bootstrap.c
@@ -317,6 +317,9 @@ AuxiliaryProcessMain(int argc, char *argv[])
 		case StartupProcess:
 			MyBackendType = B_STARTUP;
 			break;
+		case ArchiverProcess:
+			MyBackendType = B_ARCHIVER;
+			break;
 		case BgWriterProcess:
 			MyBackendType = B_BG_WRITER;
 			

Re: [PATCH] Disable bgworkers during servers start in pg_upgrade

[Julien Rouhaud]

On Wed, Jan 27, 2021 at 02:41:32PM +0100, Jehan-Guillaume de Rorthais wrote:
> 
> On Wed, 27 Jan 2021 11:25:11 +0100
> Denis Laxalde  wrote:
> 
> > Andres Freund a écrit :
> 
> > > I wonder if we could
> > > 
> > > a) set default_transaction_read_only to true, and explicitly change it
> > >in the sessions that need that.
> 
> According to Denis' tests discussed off-list, it works fine in regard with the
> powa bgworker, albeit some complaints in logs. However, I wonder how fragile 
> it
> could be as bgworker could easily manipulate either the GUC or "BEGIN READ
> WRITE". I realize this is really uncommon practices, but bgworker code from
> third parties might be surprising.

Given that having any writes happening at the wrong moment on the old cluster
can end up corrupting the new cluster, and that the corruption might not be
immediately visible we should try to put as many safeguards as possible.

so +1 for the default_transaction_read_only as done in Denis' patch at minimum,
but not only.

AFAICT it should be easy to prevent all background worker from being launched
by adding a check on IsBinaryUpgrade at the beginning of
bgworker_should_start_now().  It won't prevent modules from being loaded, so
this approach should be problematic.

> > > b) when in binary upgrade mode / -b, error out on all wal writes in
> > >sessions that don't explicitly set a session-level GUC to allow
> > >writes.
> 
> It feels safer because more specific to the subject. But I wonder if the
> benefice worth adding some (limited?) complexity and a small/quick conditional
> block in a very hot code path for a very limited use case.

I don't think that it would add that much complexity or overhead as there's
already all the infrastructure to prevent WAL writes in certain condition.  It
should be enough to add an additional test in XLogInsertAllowed() with some new
variable that is set when starting in binary upgrade mode, and a new function
to disable it that will be emitted by pg_dump / pg_dumpall in binary upgrade
mode.

> What about c) where the bgworker are not loaded by default during binary 
> upgrade
> mode / -b unless they explicitly set a bgw_flags (BGWORKER_BINARY_UPGRADE ?)
> when they are required during pg_upgrade?

As mentioned above +1 for not launching the bgworkers.  Does anyone can think
of a reason why some bgworker would really be needed during pg_upgrade, either
on the source or the target cluster?