Re: Idea: Avoid JOINs by using path expressions to follow FKs

[Pavel Stehule]

út 30. 3. 2021 v 8:52 odesílatel Julien Rouhaud  napsal:

> On Tue, Mar 30, 2021 at 08:03:09AM +0200, Pavel Stehule wrote:
> >
> > On second hand, it can be very nice to have some special strict mode in
> > Postgres - maybe slower, not compatible, that disallow some dangerous or
> > unsafe queries. But it is possible to solve in extensions, but nobody did
> > it. Something like plpgsql_check for SQL - who will write sql_check?
>
> The #1 cause of problems is probably unqualified outer references, and
> unfortunately I don't think it's really possible to detect that in an
> extension, as the required information is only available in the raw
> parsetree.
>

the raw parsetree is available  I think. I didn't check it. But it can be
easy to attach or attach a copy to Query structure. Maybe there is no
necessary hook. But it can be a good reason for implementing a post parsing
hook.

It should be easy to check if all joins are related to foreign key
constraints.

Re: Flaky vacuum truncate test in reloptions.sql

[Michael Paquier]

On Tue, Mar 30, 2021 at 01:58:50AM +0300, Arseny Sher wrote:
> Intimate reading of lazy_scan_heap says that the failure indeed might
> happen; if ConditionalLockBufferForCleanup couldn't lock the buffer and
> either the buffer doesn't need freezing or vacuum is not aggressive, we
> don't insist on close inspection of the page contents and count it as
> nonempty according to lazy_check_needs_freeze. It means the page is
> regarded as such even if it contains only garbage (but occupied) ItemIds,
> which is the case of the test. And of course this allegedly nonempty
> page prevents the truncation. Obvious competitors for the page are
> bgwriter/checkpointer; the chances of a simultaneous attack are small
> but they exist.

Yep, this is the same problem as the one discussed for c2dc1a7, where
a concurrent checkpoint may cause a page to be skipped, breaking the
test.

> I'm a bit puzzled that I've ever seen this only when running regression
> tests under our multimaster. While multimaster contains a fair amount of
> C code, I don't see how any of it can interfere with the vacuuming
> business here. I can't say I did my best to create the repoduction
> though -- the explanation above seems to be enough.

Why not just using DISABLE_PAGE_SKIPPING instead here?
--
Michael


signature.asc
Description: PGP signature

Re: standby recovery fails (tablespace related) (tentative patch and discussion)

[Paul Guo]

On 2021/3/27, 10:23 PM, "Alvaro Herrera"  wrote:

>Hmm, can you post a rebased set, where the points under discussion
>   are marked in XXX comments explaining what the issue is?  This thread is
>long and old ago that it's pretty hard to navigate the whole thing in
>order to find out exactly what is being questioned.

OK. Attached are the rebased version that includes the change I discussed
in my previous reply. Also added POD documentation change for RecursiveCopy,
and modified the patch to use the backup_options introduced in
081876d75ea15c3bd2ee5ba64a794fd8ea46d794 for tablespace mapping.

>I think 0004 can be pushed without further ado, since it's a clear and
>simple fix.  0001 needs a comment about the new parameter in
>RecursiveCopy's POD documentation.

Yeah, 0004 is no any risky. One concern seemed to be the compatibility of some
WAL dump/analysis tools(?). I have no idea about this. But if we do not backport
0004 we do not seem to need to worry about this.

>As I understand, this is a backpatchable bug-fix.

Yes.

Thanks.



v11-0001-Support-node-initialization-from-backup-with-tab.patch
Description:  v11-0001-Support-node-initialization-from-backup-with-tab.patch


v11-0002-Tests-to-replay-create-database-operation-on-sta.patch
Description:  v11-0002-Tests-to-replay-create-database-operation-on-sta.patch


v11-0003-Fix-replay-of-create-database-records-on-standby.patch
Description:  v11-0003-Fix-replay-of-create-database-records-on-standby.patch


v11-0004-Fix-database-create-drop-wal-description.patch
Description: v11-0004-Fix-database-create-drop-wal-description.patch

Re: Idea: Avoid JOINs by using path expressions to follow FKs

[Julien Rouhaud]

On Tue, Mar 30, 2021 at 09:02:39AM +0200, Pavel Stehule wrote:
> út 30. 3. 2021 v 8:52 odesílatel Julien Rouhaud  napsal:
> 
> > On Tue, Mar 30, 2021 at 08:03:09AM +0200, Pavel Stehule wrote:
> > >
> > > On second hand, it can be very nice to have some special strict mode in
> > > Postgres - maybe slower, not compatible, that disallow some dangerous or
> > > unsafe queries. But it is possible to solve in extensions, but nobody did
> > > it. Something like plpgsql_check for SQL - who will write sql_check?
> >
> > The #1 cause of problems is probably unqualified outer references, and
> > unfortunately I don't think it's really possible to detect that in an
> > extension, as the required information is only available in the raw
> > parsetree.
> >
> 
> the raw parsetree is available  I think. I didn't check it. But it can be
> easy to attach or attach a copy to Query structure. Maybe there is no
> necessary hook. But it can be a good reason for implementing a post parsing
> hook.

It's not available in any existing hook.  And even if it was you would have to
import most of transformTopLevelStmt() and all underlying functions to be able
to detect this case I think.  This should be best done in core postgres.

> It should be easy to check if all joins are related to foreign key
> constraints.

Yes, and also if the referenced columns are covered by indexes for instance.
My concern is mostly that you won't be able to cover the unqualified outer
references, which can lead to wrong query results rather than just slow
queries.

Re: Idea: Avoid JOINs by using path expressions to follow FKs

[Pavel Stehule]

út 30. 3. 2021 v 9:28 odesílatel Julien Rouhaud  napsal:

> On Tue, Mar 30, 2021 at 09:02:39AM +0200, Pavel Stehule wrote:
> > út 30. 3. 2021 v 8:52 odesílatel Julien Rouhaud 
> napsal:
> >
> > > On Tue, Mar 30, 2021 at 08:03:09AM +0200, Pavel Stehule wrote:
> > > >
> > > > On second hand, it can be very nice to have some special strict mode
> in
> > > > Postgres - maybe slower, not compatible, that disallow some
> dangerous or
> > > > unsafe queries. But it is possible to solve in extensions, but
> nobody did
> > > > it. Something like plpgsql_check for SQL - who will write sql_check?
> > >
> > > The #1 cause of problems is probably unqualified outer references, and
> > > unfortunately I don't think it's really possible to detect that in an
> > > extension, as the required information is only available in the raw
> > > parsetree.
> > >
> >
> > the raw parsetree is available  I think. I didn't check it. But it can be
> > easy to attach or attach a copy to Query structure. Maybe there is no
> > necessary hook. But it can be a good reason for implementing a post
> parsing
> > hook.
>
> It's not available in any existing hook.  And even if it was you would
> have to
> import most of transformTopLevelStmt() and all underlying functions to be
> able
> to detect this case I think.  This should be best done in core postgres.
>
> > It should be easy to check if all joins are related to foreign key
> > constraints.
>
> Yes, and also if the referenced columns are covered by indexes for
> instance.
> My concern is mostly that you won't be able to cover the unqualified outer
> references, which can lead to wrong query results rather than just slow
> queries.
>

it can be fixed

Pavel

Re: [PATCH] add concurrent_abort callback for output plugin

[Ajin Cherian]

On Tue, Mar 30, 2021 at 5:30 PM Markus Wanner <
markus.wan...@enterprisedb.com> wrote:

> Hello Ajin,
>
> On 30.03.21 06:48, Ajin Cherian wrote:
> > For now, I've created a patch that addresses the problem reported using
> > the existing callbacks.
>
> Thanks.
>
> > Do have a look if this fixes the problem reported.
>
> Yes, this replaces the PREPARE I would do from the concurrent_abort
> callback in a direct call to rb->prepare.  However, it misses the most
> important part: documentation.  Because this clearly is a surprising
> behavior for a transaction that's not fully decoded and guaranteed to
> get aborted.
>
>
Where do you suggest this be documented? From an externally visible point
of view, I dont see much of a surprise.
A transaction that was prepared and rolled back can be decoded to be
prepared and rolled back with incomplete changes.
Are you suggesting more comments in code?

regards,
Ajin Cherian
Fujitsu Australia

Re: Idea: Avoid JOINs by using path expressions to follow FKs

[Joel Jacobson]

On Tue, Mar 30, 2021, at 08:03, Pavel Stehule wrote:
> Maybe there were no technical problems.  Just this technology was coming at a 
> bad time.  The people who needed (wanted) OOP access to data got the 
> Hibernate, and there was no necessity to do this work on SQL level. In this 
> time, there was possibility to use GUI for databases, and in this time there 
> were a lot of graphic query designers.

Thanks for giving this perspective. It seems like a likely explanation. In the 
ORM camp, SQL is merely a low-level language compilation target, not a language 
humans primarily write code in.

> I don't like the idea of foreign key constraint names - it doesn't look 
> comfortable to me.  I don't say it is a bad idea, but it is not SQL, and I am 
> not sure if it needs more or less work than explicitly to write PK=FK.

I agree, it's not very comfortable. Maybe we can think of ways to improve the 
comfort?

Here are two such ideas:
 
Idea #1
===

Initial semi-automated script-assisted renaming of existing foreign keys.

In my experiences, multiple foreign keys per primary table is quite common,
but not multiple foreign keys referencing the same foreign table from the same 
primary table.

If so, then a script can be written to rename most existing foreign keys:

--
-- Script to rename foreign keys to the name of the foreign table.
-- Tables with multiple foreign keys referencing the same foreign table are 
skipped.
--
DO
$_$
DECLARE
sql_cmd text;
BEGIN
FOR sql_cmd IN
  SELECT * FROM
  (
SELECT
  format
  (
'ALTER TABLE %I.%I RENAME CONSTRAINT %I TO %I;',
conrel_nsp.nspname,
conrel.relname,
pg_constraint.conname,
confrel.relname
  ) AS sql_cmd,
  COUNT(*) OVER (PARTITION BY pg_constraint.conrelid, 
pg_constraint.confrelid)
  AS count_foreign_keys_to_same_table
FROM pg_constraint
JOIN pg_class AS conrel
  ON conrel.oid = pg_constraint.conrelid
JOIN pg_class AS confrel
  ON confrel.oid = pg_constraint.confrelid
JOIN pg_namespace AS conrel_nsp
  ON conrel_nsp.oid = conrel.relnamespace
WHERE pg_constraint.contype = 'f'
  ) AS x
  WHERE count_foreign_keys_to_same_table = 1
LOOP
  RAISE NOTICE '%', sql_cmd;
  EXECUTE sql_cmd;
END LOOP;
END
$_$;

For our example data model, this would produce:

ALTER TABLE public.orders RENAME CONSTRAINT orders_customer_id_fkey TO 
customers;
ALTER TABLE public.order_details RENAME CONSTRAINT order_details_order_id_fkey 
TO orders;
ALTER TABLE public.order_details RENAME CONSTRAINT 
order_details_product_id_fkey TO products;

To clarify what I mean with multiple foreign keys to the same table, here is an 
example:

CREATE TABLE p (
a int,
b int,
PRIMARY KEY (a),
UNIQUE (a,b)
);

CREATE TABLE f1 (
a int,
b int,
FOREIGN KEY (a) REFERENCES p
);

CREATE TABLE f2 (
a int,
b int,
FOREIGN KEY (a) REFERENCES p,
FOREIGN KEY (a,b) REFERENCES p(a,b)
);

For this example, only f1's foreign key constraint would be renamed:

ALTER TABLE public.f1 RENAME CONSTRAINT f1_a_fkey TO p;

Idea #2
===

Allow user to define the default format for new foreign key constraint name.

The format could use template patterns similar to how e.g. to_char() works.
If a conflict is found, it would do the same as today, try appending an 
increasing integer.

Users could then decide on a company-wide consistent naming convention
on how foreign keys are usually named, which would reduce the need to manually 
name them
using the CONSTRAINT keyword.

Finally, just for fun, here is an example of how we could write the query above,
if we would have real foreign keys on the catalogs:

  SELECT
format
(
  'ALTER TABLE %I.%I RENAME CONSTRAINT %I TO %I;',
  pg_constraint.conrel.pg_namespace.nspname,
  pg_constraint.conrel.relname,
  pg_constraint.conname,
  pg_constraint.confrel.relname,
) AS sql_cmd,
COUNT(*) OVER (PARTITION BY pg_constraint.conrelid, pg_constraint.confrelid)
AS count_foreign_keys_to_same_table
  FROM pg_constraint
  WHERE pg_constraint.contype = 'f'

In this example the foreign key constraint names have been
derived from the column names since both conrelid and confrelid,
reference pg_class.

I think this is a good example of where this improves the situation the most,
when you have multiple joins of the same table, forcing you to come up with 
multiple aliases
for the same table, keeping them all in memory while writing and reading such 
queries.
 
> On second hand, it can be very nice to have some special strict mode in 
> Postgres - maybe slower, not compatible, that disallow some dangerous or 
> unsafe queries. But it is possible to solve in extensions, but nobody did it. 
> Something like plpgsql_check for SQL - who will write sql_check?

Not a bad idea, this is a real problem, such a tool would be useful even with 
this proposed new syntax, as normal JOINs would continue to co-exist, for which 
nonsensical joins would still be possible.

/Joel

Re: [PATCH] add concurrent_abort callback for output plugin

[Markus Wanner]

On 30.03.21 09:39, Ajin Cherian wrote:
Where do you suggest this be documented? From an externally visible 
point of view, I dont see much of a surprise.


If you start to think about the option of committing a prepared 
transaction from a different node, the danger becomes immediately 
apparent:  A subscriber doesn't even know that the transaction is not 
complete.  How could it possibly know it's futile to COMMIT PREPARE it? 
 I think it's not just surprising, but outright dangerous to pretend 
having prepared the transaction, but potentially miss some of the changes.


(Essentially: do not assume the ROLLBACK PREPARED will make it to the 
subscriber.  There's no such guarantee.  The provider may crash, burn, 
and vanish before that happens.)


So I suggest to document this as a caveat for the prepare callback, 
because with this patch that's the callback which may be invoked for an 
incomplete transaction without the output plugin knowing.


Regards

Markus

Re: Idea: Avoid JOINs by using path expressions to follow FKs

[Pavel Stehule]

>
> I agree, it's not very comfortable. Maybe we can think of ways to improve
> the comfort?
>
> Here are two such ideas:
>
> Idea #1
> ===
>
> Initial semi-automated script-assisted renaming of existing foreign keys.
>
> In my experiences, multiple foreign keys per primary table is quite common,
> but not multiple foreign keys referencing the same foreign table from the
> same primary table.
>
> If so, then a script can be written to rename most existing foreign keys:
>
> --
> -- Script to rename foreign keys to the name of the foreign table.
> -- Tables with multiple foreign keys referencing the same foreign table
> are skipped.
> --
> DO
> $_$
> DECLARE
> sql_cmd text;
> BEGIN
> FOR sql_cmd IN
>   SELECT * FROM
>   (
> SELECT
>   format
>   (
> 'ALTER TABLE %I.%I RENAME CONSTRAINT %I TO %I;',
> conrel_nsp.nspname,
> conrel.relname,
> pg_constraint.conname,
> confrel.relname
>   ) AS sql_cmd,
>   COUNT(*) OVER (PARTITION BY pg_constraint.conrelid,
> pg_constraint.confrelid)
>   AS count_foreign_keys_to_same_table
> FROM pg_constraint
> JOIN pg_class AS conrel
>   ON conrel.oid = pg_constraint.conrelid
> JOIN pg_class AS confrel
>   ON confrel.oid = pg_constraint.confrelid
> JOIN pg_namespace AS conrel_nsp
>   ON conrel_nsp.oid = conrel.relnamespace
> WHERE pg_constraint.contype = 'f'
>   ) AS x
>   WHERE count_foreign_keys_to_same_table = 1
> LOOP
>   RAISE NOTICE '%', sql_cmd;
>   EXECUTE sql_cmd;
> END LOOP;
> END
> $_$;
>
> For our example data model, this would produce:
>
> ALTER TABLE public.orders RENAME CONSTRAINT orders_customer_id_fkey TO
> customers;
> ALTER TABLE public.order_details RENAME CONSTRAINT
> order_details_order_id_fkey TO orders;
> ALTER TABLE public.order_details RENAME CONSTRAINT
> order_details_product_id_fkey TO products;
>

you fix one issue, but you lost interesting informations


> To clarify what I mean with multiple foreign keys to the same table, here
> is an example:
>
> CREATE TABLE p (
> a int,
> b int,
> PRIMARY KEY (a),
> UNIQUE (a,b)
> );
>
> CREATE TABLE f1 (
> a int,
> b int,
> FOREIGN KEY (a) REFERENCES p
> );
>
> CREATE TABLE f2 (
> a int,
> b int,
> FOREIGN KEY (a) REFERENCES p,
> FOREIGN KEY (a,b) REFERENCES p(a,b)
> );
>
> For this example, only f1's foreign key constraint would be renamed:
>
> ALTER TABLE public.f1 RENAME CONSTRAINT f1_a_fkey TO p;
>
> Idea #2
> ===
>
> Allow user to define the default format for new foreign key constraint
> name.
>
> The format could use template patterns similar to how e.g. to_char() works.
> If a conflict is found, it would do the same as today, try appending an
> increasing integer.
>
> Users could then decide on a company-wide consistent naming convention
> on how foreign keys are usually named, which would reduce the need to
> manually name them
> using the CONSTRAINT keyword.
>
> Finally, just for fun, here is an example of how we could write the query
> above,
> if we would have real foreign keys on the catalogs:
>
>   SELECT
> format
> (
>   'ALTER TABLE %I.%I RENAME CONSTRAINT %I TO %I;',
>   pg_constraint.conrel.pg_namespace.nspname,
>   pg_constraint.conrel.relname,
>   pg_constraint.conname,
>   pg_constraint.confrel.relname,
> ) AS sql_cmd,
> COUNT(*) OVER (PARTITION BY pg_constraint.conrelid,
> pg_constraint.confrelid)
> AS count_foreign_keys_to_same_table
>   FROM pg_constraint
>   WHERE pg_constraint.contype = 'f'
>
> In this example the foreign key constraint names have been
> derived from the column names since both conrelid and confrelid,
> reference pg_class.
>
> I think this is a good example of where this improves the situation the
> most,
> when you have multiple joins of the same table, forcing you to come up
> with multiple aliases
> for the same table, keeping them all in memory while writing and reading
> such queries.
>

I do not have an opinion about this, I am sorry.  I cannot imagine so this
can work. In some complex cases, the graphic query designer can work
better. The invention of new syntax, or new tool should be better just than
checking correct usage of foreign constraints. I have worked with SQL for
over 25 years, and there were a lot of tools, and people don't use it too
much. So I am not good at dialog in this area, because I am a little bit
too sceptical :).

I remember multiple self joins only when developers used an EAV model. This
is an antipattern, and today we have better tools, and we don't need it.
It is scary, because it is completely against the relational model. If I
want to fix it, then I will invent a new different syntax type that can be
used for optimization of this case. But I have no idea how to do it well.
Maybe:

SELECT * FROM EAVTOENTITY( FROM data  GROUP BY objid COLUMN name varchar
WHEN attrname = 'name',  surname varchar WHEN attrname = 'surname',  ...)


>
> On second hand, it can be very nice to have some special stric

Re: wal stats questions

[Kyotaro Horiguchi]

At Tue, 30 Mar 2021 09:41:24 +0900, Masahiro Ikeda  
wrote in 
> I update the patch since there were my misunderstanding points.
> 
> On 2021/03/26 16:20, Masahiro Ikeda wrote:
> > Thanks for many your suggestions!
> > I made the patch to handle the issues.
> > 
> >> 1) What is the motivation to have both prevWalUsage and pgWalUsage,
> >>instead of just accumulating the stats since the last submission?
> >>There doesn't seem to be any comment explaining it? Computing
> >>differences to previous values, and copying to prev*, isn't free. I
> >>assume this is out of a fear that the stats could get reset before
> >>they're used for instrument.c purposes - but who knows?
> > 
> > I removed the unnecessary code copying pgWalUsage and just reset the
> > pgWalUsage after reporting the stats in pgstat_report_wal().
> 
> I didn't change this.
> 
> >> 2) Why is there both pgstat_send_wal() and pgstat_report_wal()? With the
> >>former being used by wal writer, the latter by most other processes?
> >>There again don't seem to be comments explaining this.
> > 
> > I added the comments why two functions are separated.
> > (But is it better to merge them?)
> 
> I updated the comments for following reasons.
> 
> 
> >> 3) Doing if (memcmp(&WalStats, &all_zeroes, sizeof(PgStat_MsgWal)) == 0)
> >>just to figure out if there's been any changes isn't all that
> >>cheap. This is regularly exercised in read-only workloads too. Seems
> >>adding a boolean WalStats.have_pending = true or such would be
> >>better.
> >> 4) For plain backends pgstat_report_wal() is called by
> >>pgstat_report_stat() - but it is not checked as part of the "Don't
> >>expend a clock check if nothing to do" check at the top.  It's
> >>probably rare to have pending wal stats without also passing one of
> >>the other conditions, but ...
> > 
> > I added the logic to check if the stats counters are updated or not in
> > pgstat_report_stat() using not only generated wal record but also write/sync
> > counters, and it can skip to call reporting function.
> 
> I removed the checking code whether the wal stats counters were updated or not
> in pgstat_report_stat() since I couldn't understand the valid case yet.
> pgstat_report_stat() is called by backends when the transaction is finished.
> This means that the condition seems always pass.

Doesn't the same holds for all other counters?  If you are saying that
"wal counters should be zero if all other stats counters are zero", we
need an assertion to check that and a comment to explain that.

I personally find it safer to add the WAL-stats condition to the
fast-return check, rather than addin such assertion.

pgstat_send_wal() is called mainly from pgstat_report_wal() which
consumes pgWalStats counters and WalWriterMain() which
doesn't. Checking on pgWalStats counters isn't so complex that we need
to avoid that in wal writer, thus *I* think pgstat_send_wal() and
pgstat_report_wal() can be consolidated.  Even if you have a strong
opinion that wal writer should call a separate function, the name
should be something other than pgstat_send_wal() since it ignores
pgWalUsage counters, which are supposed to be included in "WAL stats".


> I thought to implement if the WAL stats counters were not updated, skip to
> send all statistics including the counters for databases and so on. But I
> think it's not good because it may take more time to be reflected the
> generated stats by read-only transaction.

Ur, yep.

> > Although I added the condition which the write/sync counters are updated or
> > not, I haven't understood the following case yet...Sorry. I checked related
> > code and tested to insert large object, but I couldn't. I'll investigate 
> > more
> > deeply, but if you already know the function which leads the following case,
> > please let me know.
> 
> I understood the above case (Fujii-san, thanks for your advice in person).
> When to flush buffers, for example, to select buffer replacement victim,
> it requires a WAL flush if the buffer is dirty. So, to check the WAL stats
> counters are updated or not, I check the number of generated wal record and
> the counter of syncing in pgstat_report_wal().
> 
> The reason why not to check the counter of writing is that if to write is
> happened, to sync is happened too in the above case. I added the comments in
> the patch.

Mmm..  Although I couldn't read you clearly, The fact that a flush may
happen without a write means the reverse at the same time, a write may
happen without a flush.  For asynchronous commits, WAL-write happens
alone unaccompanied by a flush.  And the corresponding flush would
happen later without writes.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: Redundant errdetail prefix "The error was:" in some logical replication messages

[Peter Smith]

On Tue, Mar 30, 2021 at 5:10 PM Tom Lane  wrote:
>
> Peter Smith  writes:
> > There are a couple of error messages within the logical replication
> > code where the errdetail text includes a prefix of "The error was:"
>
> Hmm, isn't project style more usually to include the error reason
> in the primary message?  That is,
>
>  ereport(LOG,
> -(errmsg("could not drop the replication slot \"%s\" on 
> publisher",
> -slotname),
> - errdetail("The error was: %s", res->err)));
> +(errmsg("could not drop the replication slot \"%s\" on 
> publisher: %s",
> +slotname, res->err)));
>
> and so on.  If we had reason to think that res->err would be extremely
> long, maybe pushing it to errdetail would be sensible, but I'm not
> seeing that that is likely.
>
> (I think the "the" before "replication slot" could go away, too.)

Thanks for the review and advice.

PSA version 2 of this patch which adopts your suggestions.

--
Kind Regards,
Peter Smith.
Fujitsu Australia


v2-0001-Remove-redundant-errdetail-The-error-was.patch
Description: Binary data

Re: [PATCH] Provide more information to filter_prepare

[Amit Kapila]

On Mon, Mar 29, 2021 at 4:46 PM Markus Wanner
 wrote:
>
> On 29.03.21 13:04, vignesh C wrote:
> > The above content looks sufficient to me.
>
> Good, thanks.  Based on that, I'm adding v7 of the patch.
>

Pushed. In the last version, you have named the patch incorrectly.

-- 
With Regards,
Amit Kapila.

RE: [EXTERNAL] Any objection to documenting pg_sequence_last_value()?

[Hanefi Onaldi]

Hi All,

I recently used pg_sequence_last_value() when working on a feature in an 
extension, and it would have been easier for me if there were some 
documentation for this function.

I'd like to help document this function if there are no objections.

Best,
Hanefi

-Original Message-
From: James Coleman  
Sent: 6 Ağustos 2020 Perşembe 16:14
To: pgsql-hackers 
Subject: [EXTERNAL] Any objection to documenting pg_sequence_last_value()?

The function pg_sequence_last_value() was added to underlie the pg_sequences 
view, and it's the only way I'm aware of from userspace to directly get the 
last value of a sequence globally (i.e., not within the current session like 
currval()/lastval()). Obviously you can join to the pg_sequences view, but 
that's sometimes unnecessarily cumbersome since it doesn't expose the relid of 
the sequence.

When that function got added it apparently wasn't added to the docs, though I'm 
not sure if that was intentional or not.

Does anyone have any objections to documenting
pg_sequence_last_value() in the sequence manipulation functions doc page?

James

Re: Idea: Avoid JOINs by using path expressions to follow FKs

[Joel Jacobson]

On Tue, Mar 30, 2021, at 10:24, Pavel Stehule wrote:
> 
>> 
>> I think this is a good example of where this improves the situation the most,
>> when you have multiple joins of the same table, forcing you to come up with 
>> multiple aliases
>> for the same table, keeping them all in memory while writing and reading 
>> such queries.
> 
> ...
> I remember multiple self joins only when developers used an EAV model. This 
> is an antipattern, and today we have better tools, and we don't need it.  It 
> is scary, because it is completely against the relational model.

No, you are mistaken. There are no self-joins in any of the examples I 
presented.
I merely joined in the same table multiple times, but not with itself, so it's 
not a self join.

Here is the query again, it doesn't contain any self-joins:

SELECT
  format
  (
'ALTER TABLE %I.%I RENAME CONSTRAINT %I TO %I;',
conrel_nsp.nspname,
conrel.relname,
pg_constraint.conname,
confrel.relname
  ) AS sql_cmd,
  COUNT(*) OVER (PARTITION BY pg_constraint.conrelid, 
pg_constraint.confrelid)
  AS count_foreign_keys_to_same_table
FROM pg_constraint
JOIN pg_class AS conrel
  ON conrel.oid = pg_constraint.conrelid
JOIN pg_class AS confrel
  ON confrel.oid = pg_constraint.confrelid
JOIN pg_namespace AS conrel_nsp
  ON conrel_nsp.oid = conrel.relnamespace
WHERE pg_constraint.contype = 'f'

Where would the antipattern be here?

/Joel

Use consistent terminology for tablesync slots.

[Peter Smith]

Hi,

The logical replication tablesync worker creates tablesync slots.

Previously some PG docs pages were referring to these as "tablesync
slots", but other pages called them as "table synchronization slots".

PSA a trivial patch which (for consistency) now calls them all the
same -  "tablesync slots"

--
Kind Regards,
Peter Smith.
Fujitsu Australia


v1-0001-Use-consistent-terminology-for-tablesync-slots.patch
Description: Binary data

Re: Idea: Avoid JOINs by using path expressions to follow FKs

[Pavel Stehule]

út 30. 3. 2021 v 10:49 odesílatel Joel Jacobson  napsal:

> On Tue, Mar 30, 2021, at 10:24, Pavel Stehule wrote:
>
>
>
> I think this is a good example of where this improves the situation the
> most,
> when you have multiple joins of the same table, forcing you to come up
> with multiple aliases
> for the same table, keeping them all in memory while writing and reading
> such queries.
>
>
> ...
> I remember multiple self joins only when developers used an EAV model.
> This is an antipattern, and today we have better tools, and we don't need
> it.  It is scary, because it is completely against the relational model.
>
>
> No, you are mistaken. There are no self-joins in any of the examples I
> presented.
> I merely joined in the same table multiple times, but not with itself, so
> it's not a self join.
>
> Here is the query again, it doesn't contain any self-joins:
>
> SELECT
>   format
>   (
> 'ALTER TABLE %I.%I RENAME CONSTRAINT %I TO %I;',
> conrel_nsp.nspname,
> conrel.relname,
> pg_constraint.conname,
> confrel.relname
>   ) AS sql_cmd,
>   COUNT(*) OVER (PARTITION BY pg_constraint.conrelid,
> pg_constraint.confrelid)
>   AS count_foreign_keys_to_same_table
> FROM pg_constraint
> JOIN pg_class AS conrel
>   ON conrel.oid = pg_constraint.conrelid
> JOIN pg_class AS confrel
>   ON confrel.oid = pg_constraint.confrelid
> JOIN pg_namespace AS conrel_nsp
>   ON conrel_nsp.oid = conrel.relnamespace
> WHERE pg_constraint.contype = 'f'
>
> Where would the antipattern be here?
>
>
ok, this is not EAV.



/Joel
>

Re: [PATCH] add concurrent_abort callback for output plugin

[Amit Kapila]

On Tue, Mar 30, 2021 at 12:00 PM Markus Wanner
 wrote:
>
> Hello Ajin,
>
> On 30.03.21 06:48, Ajin Cherian wrote:
> > For now, I've created a patch that addresses the problem reported using
> > the existing callbacks.
>
> Thanks.
>
> > Do have a look if this fixes the problem reported.
>
> Yes, this replaces the PREPARE I would do from the concurrent_abort
> callback in a direct call to rb->prepare.
>

That sounds clearly a better choice. Because concurrent_abort()
internally trying to prepare transaction seems a bit ugly and not only
that if we want to go via that route, it needs to distinguish between
rollback to savepoint and rollback cases as well.

Now, we can try to find a way where for such cases we don't send
prepare/rollback prepare, but somehow detect it and send rollback
instead. And also some more checks will be required so that if we have
streamed the transaction then send stream_abort. I am not telling that
all this is not possible but I don't find worth making all such
checks.

>  However, it misses the most
> important part: documentation.  Because this clearly is a surprising
> behavior for a transaction that's not fully decoded and guaranteed to
> get aborted.
>

Yeah, I guess that makes sense to me. I think we can document it in
the docs [1] where we explained two-phase decoding. I think we can add
a point about concurrent aborts at the end of points mentioned in the
following paragraph: "The users that want to decode prepared
transactions need to be careful ."

[1] - 
https://www.postgresql.org/docs/devel/logicaldecoding-two-phase-commits.html

-- 
With Regards,
Amit Kapila.

Re: [PATCH] add concurrent_abort callback for output plugin

[Ajin Cherian]

On Tue, Mar 30, 2021 at 7:10 PM Markus Wanner <
markus.wan...@enterprisedb.com> wrote:

> On 30.03.21 09:39, Ajin Cherian wrote:
> > Where do you suggest this be documented? From an externally visible
> > point of view, I dont see much of a surprise.
>
>
>
> So I suggest to document this as a caveat for the prepare callback,
> because with this patch that's the callback which may be invoked for an
> incomplete transaction without the output plugin knowing.
>

I found some documentation that already was talking about concurrent aborts
and updated that.
Patch attached.

regards,
Ajin Cherian
Fujitsu Australia


v2-0001-Make-sure-a-prepare-is-sent-when-decoder-detects-.patch
Description: Binary data

Re: Use consistent terminology for tablesync slots.

[Amit Kapila]

On Tue, Mar 30, 2021 at 2:21 PM Peter Smith  wrote:
>
> Hi,
>
> The logical replication tablesync worker creates tablesync slots.
>
> Previously some PG docs pages were referring to these as "tablesync
> slots", but other pages called them as "table synchronization slots".
>
> PSA a trivial patch which (for consistency) now calls them all the
> same -  "tablesync slots"
>

+1 for the consistency. But I think it better to use "table
synchronization slots" in the user-facing docs as that makes it easier
for users to understand.

-- 
With Regards,
Amit Kapila.

Re: Add Nullif case for eval_const_expressions_mutator

[Peter Eisentraut]

On 24.03.21 11:52, houzj.f...@fujitsu.com wrote:

+   if (!has_nonconst_input)
+   return ece_evaluate_expr(expr);

That's not okay without a further check to see if the comparison function used
by the node is immutable.  Compare ScalarArrayOpExpr, for instance.


Thanks for pointing it out.
Attaching new patch with this change.


This patch looks okay to me and addresses all the feedback that was  
given.  If there are no more comments, I'll commit it in a few days.

Re: Idea: Avoid JOINs by using path expressions to follow FKs

[Joel Jacobson]

On Tue, Mar 30, 2021, at 10:24, Pavel Stehule wrote:
> For our example data model, this would produce:
>> 
>> ALTER TABLE public.orders RENAME CONSTRAINT orders_customer_id_fkey TO 
>> customers;
>> ALTER TABLE public.order_details RENAME CONSTRAINT 
>> order_details_order_id_fkey TO orders;
>> ALTER TABLE public.order_details RENAME CONSTRAINT 
>> order_details_product_id_fkey TO products;
> 
> you fix one issue, but you lost interesting informations 

No, it's not lost. It's still there:

# \d order_details
Foreign-key constraints:
"orders" FOREIGN KEY (order_id) REFERENCES orders(order_id)
"products" FOREIGN KEY (product_id) REFERENCES products(product_id)

You can still easily find out what tables/columns are referencing/referenced,
by using \d or look in the information_schema.

The primarily reason why this information is duplicated in the default name,
is AFAIK due to avoid hypothetical name conflicts,
which is only a real problem for users who would need to export the schema
to some other SQL database, or use apps that depend on the names to be
unique within the namespace, and not just within the table.

The comment in pg_constraint.c explains this:

/* Select a nonconflicting name for a new constraint.
*
* The objective here is to choose a name that is unique within the
* specified namespace.  Postgres does not require this, but the SQL
* spec does, and some apps depend on it.  Therefore we avoid choosing
* default names that so conflict.

/Joel

Re: Idea: Avoid JOINs by using path expressions to follow FKs

[Joel Jacobson]

On Tue, Mar 30, 2021, at 11:21, Joel Jacobson wrote:
> On Tue, Mar 30, 2021, at 10:24, Pavel Stehule wrote:
>> For our example data model, this would produce:
>>> 
>>> ALTER TABLE public.orders RENAME CONSTRAINT orders_customer_id_fkey TO 
>>> customers;
>>> ALTER TABLE public.order_details RENAME CONSTRAINT 
>>> order_details_order_id_fkey TO orders;
>>> ALTER TABLE public.order_details RENAME CONSTRAINT 
>>> order_details_product_id_fkey TO products;
>> 
>> you fix one issue, but you lost interesting informations 
> 
> No, it's not lost. It's still there:
> 
> # \d order_details
> Foreign-key constraints:
> "orders" FOREIGN KEY (order_id) REFERENCES orders(order_id)
> "products" FOREIGN KEY (product_id) REFERENCES products(product_id)
> 
> You can still easily find out what tables/columns are referencing/referenced,
> by using \d or look in the information_schema.
> 
> The primarily reason why this information is duplicated in the default name,
> is AFAIK due to avoid hypothetical name conflicts,
> which is only a real problem for users who would need to export the schema
> to some other SQL database, or use apps that depend on the names to be
> unique within the namespace, and not just within the table.
> 
> The comment in pg_constraint.c explains this:
> 
> /* Select a nonconflicting name for a new constraint.
> *
> * The objective here is to choose a name that is unique within the
> * specified namespace.  Postgres does not require this, but the SQL
> * spec does, and some apps depend on it.  Therefore we avoid choosing
> * default names that so conflict.
> 
> /Joel

Users who have decided to stick to PostgreSQL for ever,
and don't have any apps that depend on (the IMHO stupid) decision by the SQL 
standard
to require constraints to be unique per namespace, can and should happily 
ignore this restriction.

/Joel

Re: Use consistent terminology for tablesync slots.

[Bharath Rupireddy]

On Tue, Mar 30, 2021 at 2:44 PM Amit Kapila  wrote:
>
> On Tue, Mar 30, 2021 at 2:21 PM Peter Smith  wrote:
> >
> > Hi,
> >
> > The logical replication tablesync worker creates tablesync slots.
> >
> > Previously some PG docs pages were referring to these as "tablesync
> > slots", but other pages called them as "table synchronization slots".
> >
> > PSA a trivial patch which (for consistency) now calls them all the
> > same -  "tablesync slots"
> >
>
> +1 for the consistency. But I think it better to use "table
> synchronization slots" in the user-facing docs as that makes it easier
> for users to understand.

+1 for the phrasing "tablesync slots" to "table synchronization slots"
as it is more readable. And also the user facing error message and guc
description i.e. "logical replication table synchronization worker for
subscription" and max_sync_workers_per_subscription respectively are
showing it that way.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

Re: Re: parallel distinct union and aggregate support patch

[bu...@sohu.com]

> This patch has not gotten any review in the last two CFs and is unlikely
> to be committed for PG14 so I have moved it to the 2021-07 CF. A rebase
> is also required so marked Waiting for Author.
>  
> I can see this is a work in progress, but you may want to consider the
> several suggestions that an unbuffered approach might be better.

I have written a plan with similar functions, It is known that the following 
two situations do not work well.
1. Under "Parallel Append" plan
  Gather
  -> Parallel Append
  -> Agg
  -> Parallel Redistribute(1)
  -> ...
  -> Agg
  -> Parallel Redistribute(2)
  -> ...
  when parallel worker 1 execute "Parallel Redistribute(1)" and worker execute 
"Parallel Redistribute(2)",
  both "Parallel Redistribute" plan can not send tuples to other worker(both 
worker are stuck),
  because outher worker's memory buffer run out soon.

2. Under "Nestloop" plan
  Gather
  -> Nestloop(1)
  -> Nestloop(2)
  -> Parallel Redistribute
  -> ...
  -> IndexScan
  -> Agg
  At some point might be the case: parallel worker 1 executing Agg and 
"Parallel Redistribute" plan's memory buffer is full,
  worker 2 executing "Parallel Redistribute" and it waiting worker 1 eat 
"Parallel Redistribute" plan's memory buffer,
  it's stuck.




bu...@sohu.com

Re: [PATCH] Provide more information to filter_prepare

[Markus Wanner]

On 30.03.21 10:33, Amit Kapila wrote:

Pushed. In the last version, you have named the patch incorrectly.


Thanks a lot, Amit!

Regards

Markus

extra semicolon in postgres_fdw test cases

[Suraj Kharage]

Hi,

Noticed that an extra semicolon in a couple of test cases related to
postgres_fdw. Removed that in the attached patch. It can be backported till
v11 where we added those test cases.

-- 
--

Thanks & Regards,
Suraj kharage,



edbpostgres.com


remove_extra_semicolon_postgres_fdw.patch
Description: Binary data

Re: Refactor SSL test framework to support multiple TLS libraries

[Michael Paquier]

On Tue, Mar 30, 2021 at 03:50:28PM +0900, Michael Paquier wrote:
> The test_*() ones are just wrappers for psql able to use a customized
> connection string.  It seems to me that it would make sense to move
> those two into PostgresNode::psql itself and extend it to be able to
> handle custom connection strings?

Looking at this part, I think that this is a win in terms of future
changes for SSLServer.pm as it would become a facility only in charge
of managing the backend's SSL configuration.  This has also the
advantage to make the error handling with psql more consistent with
the other tests.

So, attached is a patch to do this simplification.  The bulk of the
changes is within the tests themselves to adapt to the merge of
$common_connstr and $connstr for the new routines of PostgresNode.pm,
and I have done things this way to ease the patch lookup.  Thoughts?
--
Michael
diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
index 1e96357d7e..403ef56d54 100644
--- a/src/test/perl/PostgresNode.pm
+++ b/src/test/perl/PostgresNode.pm
@@ -1511,6 +1511,11 @@ the B parameter is also given.
 If B is set and this parameter is given, the scalar it references
 is set to true if the psql call times out.
 
+=item connstr => B
+
+If set, use this as the connection string for the connection to the
+backend.
+
 =item replication => B
 
 If set, add B to the conninfo string.
@@ -1550,12 +1555,24 @@ sub psql
 	my $replication   = $params{replication};
 	my $timeout   = undef;
 	my $timeout_exception = 'psql timed out';
+
+	# Build the connection string.
+	my $psql_connstr;
+	if (defined $params{connstr})
+	{
+		$psql_connstr = $params{connstr};
+	}
+	else
+	{
+		$psql_connstr = $self->connstr($dbname);
+	}
+	$psql_connstr .= defined $replication ? " replication=$replication" : "";
+
 	my @psql_params   = (
 		'psql',
 		'-XAtq',
 		'-d',
-		$self->connstr($dbname)
-		  . (defined $replication ? " replication=$replication" : ""),
+		$psql_connstr,
 		'-f',
 		'-');
 
@@ -1849,6 +1866,54 @@ sub interactive_psql
 
 =pod
 
+=item $node->connect_ok($connstr, $testname)
+
+Attempt a connection to the given node, with a custom connection string.
+This is expected to succeed.
+
+=cut
+
+sub connect_ok
+{
+	local $Test::Builder::Level = $Test::Builder::Level + 1;
+
+	my ($self, $connstr, $testname) = @_;
+
+	my ($ret, $stdout, $stderr) = $self->psql('postgres',
+		"SELECT \$\$connected with $connstr\$\$",
+		connstr => "$connstr", on_error_stop => 0);
+
+	ok($ret == 0, $testname);
+	return;
+}
+
+=pod
+
+=item $node->connect_fails($connstr, $testname, $expected_stderr)
+
+Attempt a connection to the given node, with a custom connection string.
+This is expected to fail.
+
+=cut
+
+sub connect_fails
+{
+	local $Test::Builder::Level = $Test::Builder::Level + 1;
+
+	my ($self, $connstr, $expected_stderr, $testname) = @_;
+
+	my ($ret, $stdout, $stderr) = $self->psql('postgres',
+		"SELECT \$\$connected with $connstr\$\$",
+		connstr => "$connstr");
+
+	ok($ret != 0, $testname);
+
+	like($stderr, $expected_stderr, "$testname: matches");
+	return;
+}
+
+=pod
+
 =item $node->poll_query_until($dbname, $query [, $expected ])
 
 Run B<$query> repeatedly, until it returns the B<$expected> result
diff --git a/src/test/ssl/t/001_ssltests.pl b/src/test/ssl/t/001_ssltests.pl
index adaa1b4e9b..dc3b69cb46 100644
--- a/src/test/ssl/t/001_ssltests.pl
+++ b/src/test/ssl/t/001_ssltests.pl
@@ -135,102 +135,102 @@ $common_connstr =
   "user=ssltestuser dbname=trustdb sslcert=invalid hostaddr=$SERVERHOSTADDR host=common-name.pg-ssltest.test";
 
 # The server should not accept non-SSL connections.
-test_connect_fails(
-	$common_connstr, "sslmode=disable",
+$node->connect_fails(
+	$common_connstr . " " . "sslmode=disable",
 	qr/\Qno pg_hba.conf entry\E/,
 	"server doesn't accept non-SSL connections");
 
 # Try without a root cert. In sslmode=require, this should work. In verify-ca
 # or verify-full mode it should fail.
-test_connect_ok(
-	$common_connstr,
+$node->connect_ok(
+	$common_connstr . " " .
 	"sslrootcert=invalid sslmode=require",
 	"connect without server root cert sslmode=require");
-test_connect_fails(
-	$common_connstr,
+$node->connect_fails(
+	$common_connstr . " " .
 	"sslrootcert=invalid sslmode=verify-ca",
 	qr/root certificate file "invalid" does not exist/,
 	"connect without server root cert sslmode=verify-ca");
-test_connect_fails(
-	$common_connstr,
+$node->connect_fails(
+	$common_connstr . " " .
 	"sslrootcert=invalid sslmode=verify-full",
 	qr/root certificate file "invalid" does not exist/,
 	"connect without server root cert sslmode=verify-full");
 
 # Try with wrong root cert, should fail. (We're using the client CA as the
 # root, but the server's key is signed by the server CA.)
-test_connect_fails($common_connstr,
+$node->connect_fails($common_connstr . " " .
 	"sslrootcert=ssl/client_ca.crt sslmode=require",
 	qr/SSL error/, "connect with wrong server root cert sslmode=require");
-test_c

Re: [PATCH] add concurrent_abort callback for output plugin

[Markus Wanner]

On 30.03.21 11:02, Amit Kapila wrote:

On Tue, Mar 30, 2021 at 12:00 PM Markus Wanner

Yes, this replaces the PREPARE I would do from the concurrent_abort
callback in a direct call to rb->prepare.


Because concurrent_abort()
internally trying to prepare transaction seems a bit ugly and not only
that if we want to go via that route, it needs to distinguish between
rollback to savepoint and rollback cases as well.


Just to clarify: of course, the concurrent_abort callback only sends a 
message to the subscriber, which then (in our current implementation) 
upon reception of the concurrent_abort message opts to prepare the 
transaction.  Different implementations would be possible.


I would recommend this more explicit API and communication over hiding 
the concurrent abort in a prepare callback.


Regards

Markus

Re: pgbench - add pseudo-random permutation function

[Dean Rasheed]

On Mon, 22 Mar 2021 at 13:43, Dean Rasheed  wrote:
>
> On Sun, 14 Mar 2021 at 16:08, Fabien COELHO  wrote:
> >
> > > My main question on this now is, do you have a scholar reference for
> > > this algorithm?
> >
> > Nope, otherwise I would have put a reference. I'm a scholar though, if
> > it helps:-)
> >
> > I could not find any algorithm that fitted the bill. The usual approach
> > (eg benchmarking designs) is too use some hash function and assume that it
> > is not a permutation, too bad.
> >
> > Basically the algorithm mimics a few rounds of cryptographic encryption
> > adapted to any size and simple operators, whereas encryption function are
> > restricted to power of two blocks (eg the Feistel network). The structure
> > is the same AES with its AddRoundKey the xor-ing stage (adapted to non
> > power of two in whiten above), MixColumns which does the scattering, and
> > for key expansion, I used Donald Knuth generator. Basically one could say
> > that permute is an inexpensive and insecure AES:-)
> >
>
> I spent a little time playing around with this, trying to come up with
> a reasonable way to test it.

I spent more time testing this -- the previous testing was mostly for
large values of size, so I decided to also test it for small sizes.
The theoretical number of possible permutations grows rapidly with
size (size!), and the actual number of permutations observed grew
almost as quickly:

 size  size!distinct perms
  2 22
  3 66
  4 24   24
  5 120  120
  6 720  720
  7 5040 5040
  8 4032024382
  9 362880   181440
  103628800  3627355

My test script stopped once the first permutation had been seen 100
times, so it might have seen more permutations had it kept going for
longer.

However, looking at the actual output, there is a very significant
non-uniformity in the probability of particular permutations being
chosen, especially at certain sizes like 8 and 10. For example, at
size = 8, the identity permutation is significantly more likely than
almost any other permutation (roughly 13 times more likely than it
would be by random chance). Additionally, the signs are that this
non-uniformity tends to increase with size. At size = 10, the average
number of occurrences of each permutation in the test was 148, but
there were some that didn't appear at all, many that only appeared 2
or 3 times, and then some that appeared nearly 5000 times.

Also, there appears to be an unfortunate dependence on the seed -- if
the seed is even and the size is a power of 2, it looks like the
number of distinct permutations produced is just size/2, which is a
tiny fraction of size!.

This, together with the results from the previous K-S uniformity tests
at larger sizes, suggests that the current algorithm may not be random
enough to scatter values and remove correlations from a non-uniform
distribution.

In an attempt to do better, I tweaked the algorithm in the attached
patch, which makes use of erand48() to inject more randomness into the
permutation choice. Repeating the tests with the updated algorithm
shows that it has a number of advantages:

1). For small sizes (2-10), each of the size! possible permutations is
now produced with roughly equal probability. No single permutation was
much more likely than expected.

2). The loss of randomness for even seeds is gone.

3). For any given input, the output is more uniformly randomly
distributed for different seeds.

4). For any pair of nearby inputs, the corresponding outputs are more
uniformly randomly separated from one another.

5). The updated algorithm no longer uses modular_multiply(), so it
works the same on all platforms.

I still feel slightly uneasy about inventing our own algorithm here,
but I wasn't able to find anything else suitable, and I've now tested
this quite extensively. All the indications are that this updated
algorithm works well at all sizes and produces sufficiently random
results, though if anyone else can think of other approaches to
testing the core algorithm, that would be useful. For reference, I
attach 2 standalone test C programs I used for testing, which include
copies of the old and new algorithms.

I also did a quick copy editing pass over the docs, and I think the
patch is in pretty good shape. Barring objections, I'll see about
committing it later this week.

Regards,
Dean
diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
new file mode 100644
index 50cf22b..84d9566
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -1057,7 +1057,7 @@ pgbench  options<
 
   
 default_seed 
-   seed used in hash functions by default
+   seed used in hash and pseudorandom permutation functions by default
   
 
   
@@ -1866,6 +1866,24 @@ SELECT 4 AS four \; SELECT 5 AS five \as
 
   

+permute ( i, size [, seed ] )
+integer
+   
+   
+Permuted value of i, in the ra

Issue with point_ops and NaN

[Julien Rouhaud]

Hi,

While running some sanity checks on the regression tests, I found one test that
returns different results depending on whether an index or a sequential scan is
used.

Minimal reproducer:

=# CREATE TABLE point_tbl AS select '(nan,nan)'::point f1;
=# CREATE INDEX ON point_tbl USING gist(f1);

=# EXPLAIN SELECT * FROM point_tbl WHERE f1 <@ polygon 
'(0,0),(0,100),(100,100),(50,50),(100,0),(0,0)';
  QUERY PLAN
--
 Seq Scan on point_tbl  (cost=0.00..1.01 rows=1 width=16)
   Filter: (f1 <@ '((0,0),(0,100),(100,100),(50,50),(100,0),(0,0))'::polygon)
(2 rows)

=# SELECT * FROM point_tbl WHERE f1 <@ polygon 
'(0,0),(0,100),(100,100),(50,50),(100,0),(0,0)';
f1
---
 (NaN,NaN)
(1 row)

SET enable_seqscan = 0;


=# EXPLAIN SELECT * FROM point_tbl WHERE f1 <@ polygon 
'(0,0),(0,100),(100,100),(50,50),(100,0),(0,0)';
   QUERY PLAN   


 Index Only Scan using point_tbl_f1_idx on point_tbl  (cost=0.12..8.14 rows=1 
width=16)
   Index Cond: (f1 <@ 
'((0,0),(0,100),(100,100),(50,50),(100,0),(0,0))'::polygon)
(2 rows)

=# SELECT * FROM point_tbl WHERE f1 <@ polygon 
'(0,0),(0,100),(100,100),(50,50),(100,0),(0,0)';
 f1 

(0 rows)

The discrepancy comes from the fact that the sequential scan checks the
condition using point_inside() / lseg_crossing(), while the gist index will
check the condition using box_overlap() / box_ov(), which have different
opinions on how to handle NaN.

Getting a consistent behavior shouldn't be hard, but I'm unsure which behavior
is actually correct.

Re: extra semicolon in postgres_fdw test cases

[Bharath Rupireddy]

On Tue, Mar 30, 2021 at 3:21 PM Suraj Kharage
 wrote:
>
> Hi,
>
> Noticed that an extra semicolon in a couple of test cases related to 
> postgres_fdw. Removed that in the attached patch. It can be backported till 
> v11 where we added those test cases.

+1 for the change. It looks like a typo and can be backported.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

Re: row filtering for logical replication

[Amit Kapila]

On Mon, Mar 29, 2021 at 6:47 PM Euler Taveira  wrote:
>
Few comments:
==
1. How can we specify row filters for multiple tables for a
publication? Consider a case as below:
postgres=# CREATE TABLE tab_rowfilter_1 (a int primary key, b text);
CREATE TABLE
postgres=# CREATE TABLE tab_rowfilter_2 (c int primary key);
CREATE TABLE

postgres=# CREATE PUBLICATION tap_pub_1 FOR TABLE tab_rowfilter_1,
tab_rowfilter_2 WHERE (a > 1000 AND b <> 'filtered');
ERROR:  column "a" does not exist
LINE 1: ...FOR TABLE tab_rowfilter_1, tab_rowfilter_2 WHERE (a > 1000 A...

 ^

postgres=# CREATE PUBLICATION tap_pub_1 FOR TABLE tab_rowfilter_1,
tab_rowfilter_2  WHERE (c > 1000);
CREATE PUBLICATION

It gives an error when I tried to specify the columns corresponding to
the first relation but is fine for columns for the second relation.
Then, I tried few more combinations like below but that didn't work.
CREATE PUBLICATION tap_pub_1 FOR TABLE tab_rowfilter_1 As t1,
tab_rowfilter_2 As t2 WHERE (t1.a > 1000 AND t1.b <> 'filtered');

Will users be allowed to specify join conditions among columns from
multiple tables?

2.
+ /*
+ * Although ALTER PUBLICATION grammar allows WHERE clause to be specified
+ * for DROP TABLE action, it doesn't make sense to allow it. We implement
+ * this restriction here, instead of complicating the grammar to enforce
+ * it.
+ */
+ if (stmt->tableAction == DEFELEM_DROP)
+ {
+ ListCell   *lc;
+
+ foreach(lc, stmt->tables)
+ {
+ PublicationTable *t = lfirst(lc);
+
+ if (t->whereClause)
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("cannot use a WHERE clause when removing table from
publication \"%s\"",
+ NameStr(pubform->pubname;
+ }
+ }

Is there a reason to deal with this here separately rather than in the
ALTER PUBLICATION grammar?


-- 
With Regards,
Amit Kapila.

Re: [PATCH] add concurrent_abort callback for output plugin

[Markus Wanner]

On 30.03.21 11:12, Ajin Cherian wrote:
I found some documentation that already was talking about concurrent 
aborts and updated that.


Thanks.

I just noticed as of PG13, concurrent_abort is part of ReorderBufferTXN, 
so it seems the prepare_cb (or stream_prepare_cb) can actually figure a 
concurrent abort happened (and the transaction may be incomplete). 
That's good and indeed makes an additional callback unnecessary.


I recommend giving a hint to that field in the documentation as well.


diff --git a/doc/src/sgml/logicaldecoding.sgml 
b/doc/src/sgml/logicaldecoding.sgml
index 80eb96d..d2f8d39 100644
--- a/doc/src/sgml/logicaldecoding.sgml
+++ b/doc/src/sgml/logicaldecoding.sgml
@@ -545,12 +545,14 @@ CREATE TABLE another_catalog_table(data text) WITH 
(user_catalog_table = true);
  executed within that transaction. A transaction that is prepared for
  a two-phase commit using PREPARE TRANSACTION will
  also be decoded if the output plugin callbacks needed for decoding
- them are provided. It is possible that the current transaction which
+ them are provided. It is possible that the current prepared transaction 
which
  is being decoded is aborted concurrently via a ROLLBACK 
PREPARED
  command. In that case, the logical decoding of this transaction will
- be aborted too. We will skip all the changes of such a transaction once
- the abort is detected and abort the transaction when we read WAL for
- ROLLBACK PREPARED.
+ be aborted too. All the changes of such a transaction is skipped once


typo: changes [..] *are* skipped, plural.


+ the abort is detected and the prepare_cb callback is 
invoked.
+ This could result in a prepared transaction with incomplete changes.


... "in which case the concurrent_abort field of the 
passed ReorderBufferTXN struct is set.", as a proposal?



+ This is done so that eventually when the ROLLBACK 
PREPARED
+ is decoded, there is a corresponding prepared transaction with a matching 
gid.
 
 
 


Everything else sounds good to me.

Regards

Markus

Re: extra semicolon in postgres_fdw test cases

[Amit Kapila]

On Tue, Mar 30, 2021 at 4:50 PM Bharath Rupireddy
 wrote:
>
> On Tue, Mar 30, 2021 at 3:21 PM Suraj Kharage
>  wrote:
> >
> > Hi,
> >
> > Noticed that an extra semicolon in a couple of test cases related to 
> > postgres_fdw. Removed that in the attached patch. It can be backported till 
> > v11 where we added those test cases.
>
> +1 for the change. It looks like a typo and can be backported.
>

Looks good to me as well but I think one can choose not to backpatch
as there is no functional impact but OTOH, there is some value in
keeping tests/code consistent.

-- 
With Regards,
Amit Kapila.

Re: [PATCH] add concurrent_abort callback for output plugin

[Markus Wanner]

On 30.03.21 11:54, Markus Wanner wrote:
I would recommend this more explicit API and communication over hiding 
the concurrent abort in a prepare callback.


I figured we already have the ReorderBufferTXN's concurrent_abort flag, 
thus I agree the prepare_cb is sufficient and revoke this recommendation 
(and the concurrent_abort callback patch).


Regards

Markus

Re: wal stats questions

[Masahiro Ikeda]

On 2021/03/30 17:28, Kyotaro Horiguchi wrote:
> At Tue, 30 Mar 2021 09:41:24 +0900, Masahiro Ikeda  
> wrote in 
>> I update the patch since there were my misunderstanding points.
>>
>> On 2021/03/26 16:20, Masahiro Ikeda wrote:
>>> Thanks for many your suggestions!
>>> I made the patch to handle the issues.
>>>
 1) What is the motivation to have both prevWalUsage and pgWalUsage,
instead of just accumulating the stats since the last submission?
There doesn't seem to be any comment explaining it? Computing
differences to previous values, and copying to prev*, isn't free. I
assume this is out of a fear that the stats could get reset before
they're used for instrument.c purposes - but who knows?
>>>
>>> I removed the unnecessary code copying pgWalUsage and just reset the
>>> pgWalUsage after reporting the stats in pgstat_report_wal().
>>
>> I didn't change this.
>>
 2) Why is there both pgstat_send_wal() and pgstat_report_wal()? With the
former being used by wal writer, the latter by most other processes?
There again don't seem to be comments explaining this.
>>>
>>> I added the comments why two functions are separated.
>>> (But is it better to merge them?)
>>
>> I updated the comments for following reasons.
>>
>>
 3) Doing if (memcmp(&WalStats, &all_zeroes, sizeof(PgStat_MsgWal)) == 0)
just to figure out if there's been any changes isn't all that
cheap. This is regularly exercised in read-only workloads too. Seems
adding a boolean WalStats.have_pending = true or such would be
better.
 4) For plain backends pgstat_report_wal() is called by
pgstat_report_stat() - but it is not checked as part of the "Don't
expend a clock check if nothing to do" check at the top.  It's
probably rare to have pending wal stats without also passing one of
the other conditions, but ...
>>>
>>> I added the logic to check if the stats counters are updated or not in
>>> pgstat_report_stat() using not only generated wal record but also write/sync
>>> counters, and it can skip to call reporting function.
>>
>> I removed the checking code whether the wal stats counters were updated or 
>> not
>> in pgstat_report_stat() since I couldn't understand the valid case yet.
>> pgstat_report_stat() is called by backends when the transaction is finished.
>> This means that the condition seems always pass.
> 
> Doesn't the same holds for all other counters?  If you are saying that
> "wal counters should be zero if all other stats counters are zero", we
> need an assertion to check that and a comment to explain that.
> 
> I personally find it safer to add the WAL-stats condition to the
> fast-return check, rather than addin such assertion.
Thanks for your comments.

OK, I added the condition to the fast-return check. I noticed that I
misunderstood that the purpose is to avoid expanding a clock check using WAL
stats counters. But, the purpose is to make the conditions stricter, right?


> pgstat_send_wal() is called mainly from pgstat_report_wal() which
> consumes pgWalStats counters and WalWriterMain() which
> doesn't. Checking on pgWalStats counters isn't so complex that we need
> to avoid that in wal writer, thus *I* think pgstat_send_wal() and
> pgstat_report_wal() can be consolidated.  Even if you have a strong
> opinion that wal writer should call a separate function, the name
> should be something other than pgstat_send_wal() since it ignores
> pgWalUsage counters, which are supposed to be included in "WAL stats".

OK, I consolidated them.



>> I thought to implement if the WAL stats counters were not updated, skip to
>> send all statistics including the counters for databases and so on. But I
>> think it's not good because it may take more time to be reflected the
>> generated stats by read-only transaction.
> 
> Ur, yep.
> 
>>> Although I added the condition which the write/sync counters are updated or
>>> not, I haven't understood the following case yet...Sorry. I checked related
>>> code and tested to insert large object, but I couldn't. I'll investigate 
>>> more
>>> deeply, but if you already know the function which leads the following case,
>>> please let me know.
>>
>> I understood the above case (Fujii-san, thanks for your advice in person).
>> When to flush buffers, for example, to select buffer replacement victim,
>> it requires a WAL flush if the buffer is dirty. So, to check the WAL stats
>> counters are updated or not, I check the number of generated wal record and
>> the counter of syncing in pgstat_report_wal().
>>
>> The reason why not to check the counter of writing is that if to write is
>> happened, to sync is happened too in the above case. I added the comments in
>> the patch.
> 
> Mmm..  Although I couldn't read you clearly, The fact that a flush may
> happen without a write means the reverse at the same time, a write may
> happen without a flush.  For asynchronous commits, WAL-write h

Outdated comment for CreateStmt.inhRelations

[Julien Rouhaud]

Hi,

I just noticed that the comment for CreateStmt.inhRelations says that it's a
List of inhRelation, which hasn't been the case for a very long time.

Trivial patch attached.
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 12e0e026dc..334262b1dd 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -2131,7 +2131,7 @@ typedef struct CreateStmt
RangeVar   *relation;   /* relation to create */
List   *tableElts;  /* column definitions (list of 
ColumnDef) */
List   *inhRelations;   /* relations to inherit from (list of
-* inhRelation) 
*/
+* RangeVar) */
PartitionBoundSpec *partbound;  /* FOR VALUES clause */
PartitionSpec *partspec;/* PARTITION BY clause */
TypeName   *ofTypename; /* OF typename */

Re: Issue with point_ops and NaN

[Laurenz Albe]

On Tue, 2021-03-30 at 17:57 +0800, Julien Rouhaud wrote:
> While running some sanity checks on the regression tests, I found one test 
> that
> returns different results depending on whether an index or a sequential scan 
> is
> used.
> 
> Minimal reproducer:
> 
> =# CREATE TABLE point_tbl AS select '(nan,nan)'::point f1;
> =# CREATE INDEX ON point_tbl USING gist(f1);
> 
> =# EXPLAIN SELECT * FROM point_tbl WHERE f1 <@ polygon 
> '(0,0),(0,100),(100,100),(50,50),(100,0),(0,0)';
>   QUERY PLAN
> --
>  Seq Scan on point_tbl  (cost=0.00..1.01 rows=1 width=16)
>Filter: (f1 <@ '((0,0),(0,100),(100,100),(50,50),(100,0),(0,0))'::polygon)
> (2 rows)
> 
> =# SELECT * FROM point_tbl WHERE f1 <@ polygon 
> '(0,0),(0,100),(100,100),(50,50),(100,0),(0,0)';
> f1
> ---
>  (NaN,NaN)
> (1 row)
> 
> SET enable_seqscan = 0;
> 
> 
> =# EXPLAIN SELECT * FROM point_tbl WHERE f1 <@ polygon 
> '(0,0),(0,100),(100,100),(50,50),(100,0),(0,0)';
>QUERY PLAN 
>   
> 
>  Index Only Scan using point_tbl_f1_idx on point_tbl  (cost=0.12..8.14 rows=1 
> width=16)
>Index Cond: (f1 <@ 
> '((0,0),(0,100),(100,100),(50,50),(100,0),(0,0))'::polygon)
> (2 rows)
> 
> =# SELECT * FROM point_tbl WHERE f1 <@ polygon 
> '(0,0),(0,100),(100,100),(50,50),(100,0),(0,0)';
>  f1 
> 
> (0 rows)
> 
> The discrepancy comes from the fact that the sequential scan checks the
> condition using point_inside() / lseg_crossing(), while the gist index will
> check the condition using box_overlap() / box_ov(), which have different
> opinions on how to handle NaN.
> 
> Getting a consistent behavior shouldn't be hard, but I'm unsure which behavior
> is actually correct.

I'd say that this is certainly wrong:

SELECT point('NaN','NaN') <@ polygon('(0,0),(1,0),(1,1),(0,0)');

 ?column? 
--
 t
(1 row)

Yours,
Laurenz Albe

Re: Issue with point_ops and NaN

[Julien Rouhaud]

On Tue, Mar 30, 2021 at 02:47:05PM +0200, Laurenz Albe wrote:
> On Tue, 2021-03-30 at 17:57 +0800, Julien Rouhaud wrote:
> > 
> > Getting a consistent behavior shouldn't be hard, but I'm unsure which 
> > behavior
> > is actually correct.
> 
> I'd say that this is certainly wrong:
> 
> SELECT point('NaN','NaN') <@ polygon('(0,0),(1,0),(1,1),(0,0)');
> 
>  ?column? 
> --
>  t
> (1 row)

Yeah that's what I think too, but I wanted to have confirmation.

Re: Get memory contexts of an arbitrary backend process

[torikoshia]

On 2021-03-30 02:28, Fujii Masao wrote:

Thanks for reviewing and kind suggestions!


It adds pg_log_backend_memory_contexts(pid) which logs memory contexts
of the specified backend process.

The number of child contexts to be logged per parent is limited to 100
as with MemoryContextStats().

As written in commit 7b5ef8f2d07, which limits the verbosity of
memory context statistics dumps, it supposes that practical cases
where the dump gets long will typically be huge numbers of
siblings under the same parent context; while the additional
debugging value from seeing details about individual siblings
beyond 100 will not be large.

Thoughts?


I'm OK with 100. We should comment why we chose 100 for that.


Added following comments.

+   /*
+* When a backend process is consuming huge memory, logging all 
its
+* memory contexts might overrun available disk space. To 
prevent
+* this, we limit the number of child contexts per parent to 
100.

+*
+* As with MemoryContextStats(), we suppose that practical cases
+* where the dump gets long will typically be huge numbers of
+* siblings under the same parent context; while the additional
+* debugging value from seeing details about individual siblings
+* beyond 100 will not be large.
+*/
+   MemoryContextStatsDetail(TopMemoryContext, 100, false);



Here are some review comments.

Isn't it better to move HandleProcSignalLogMemoryContext() and
ProcessLogMemoryContextInterrupt() to mcxt.c from procsignal.c
(like the functions for notify interrupt are defined in async.c)
because they are the functions for memory contexts?


Agreed.
Also renamed HandleProcSignalLogMemoryContext to
HandleLogMemoryContextInterrupt.


+ * HandleProcSignalLogMemoryContext
+ *
+ * Handle receipt of an interrupt indicating log memory context.
+ * Signal handler portion of interrupt handling.

IMO it's better to comment why we need to separate the function into 
two,
i.e., HandleProcSignalLogMemoryContext() and 
ProcessLogMemoryContextInterrupt(),
like the comment for other similar function explains. What about the 
followings?


Thanks! Changed them to the suggested one.


---
HandleLogMemoryContextInterrupt

Handle receipt of an interrupt indicating logging of memory contexts.

All the actual work is deferred to ProcessLogMemoryContextInterrupt(),
because we cannot safely emit a log message inside the signal handler.
---
ProcessLogMemoryContextInterrupt

Perform logging of memory contexts of this backend process.

Any backend that participates in ProcSignal signaling must arrange to
call this function if we see LogMemoryContextPending set. It is called
from CHECK_FOR_INTERRUPTS(), which is enough because the target process
for logging of memory contexts is a backend.
---


+   if (CheckProcSignal(PROCSIG_LOG_MEMORY_CONTEXT))
+   HandleProcSignalLogMemoryContext();
+
if (CheckProcSignal(PROCSIG_BARRIER))
HandleProcSignalBarrierInterrupt();

The code for memory context logging interrupt came after barrier 
interrupt
in other places, e.g., procsignal.h. Why is this order of code 
different?


Fixed.


+/*
+ * pg_log_backend_memory_contexts
+ * Print memory context of the specified backend process.

Isn't it better to move pg_log_backend_memory_contexts() to mcxtfuncs.c
from mcxt.c because this is the SQL function memory contexts?


Agreed.

IMO we should comment why we allow only superuser to call this 
function.

What about the following?


Thanks!
Modified the patch according to the suggestions.


-
Signal a backend process to log its memory contexts.

Only superusers are allowed to signal to log the memory contexts
because allowing any users to issue this request at an unbounded rate
would cause lots of log messages and which can lead to denial of 
service.

-

+   PGPROC  *proc = BackendPidGetProc(pid);
+
+   /* Check whether the target process is PostgreSQL backend process. */
+   if (proc == NULL)

What about adding more comments as follows?

-
+   /*
+	 * BackendPidGetProc returns NULL if the pid isn't valid; but by the 
time
+	 * we reach kill(), a process for which we get a valid proc here 
might
+	 * have terminated on its own.  There's no way to acquire a lock on 
an
+	 * arbitrary process to prevent that. But since this mechanism is 
usually

+* used to debug a backend running and consuming lots of memory,
+* that it might end on its own first and its memory contexts are not
+* logged is not a problem.
+*/
+   if (proc == NULL)
+   {
+   /*
+* This is just a warning so a loop-through-resultset will not 
abort
+* if one backend logged its memory contexts during the run.
+*/
+

Re: Flaky vacuum truncate test in reloptions.sql

[Arseny Sher]

On 3/30/21 10:12 AM, Michael Paquier wrote:

> Yep, this is the same problem as the one discussed for c2dc1a7, where
> a concurrent checkpoint may cause a page to be skipped, breaking the
> test.

Indeed, Alexander Lakhin pointed me to that commit after I wrote the 
message.


> Why not just using DISABLE_PAGE_SKIPPING instead here?

I think this is not enough. DISABLE_PAGE_SKIPPING disables vm consulting 
(sets

aggressive=true in the routine); however, if the page is locked and
lazy_check_needs_freeze says there is nothing to freeze on it, we again 
don't

look at its contents closely.


-- cheers, arseny

Re: Refactor SSL test framework to support multiple TLS libraries

[Andrew Dunstan]

On 3/30/21 5:53 AM, Michael Paquier wrote:
> On Tue, Mar 30, 2021 at 03:50:28PM +0900, Michael Paquier wrote:
>> The test_*() ones are just wrappers for psql able to use a customized
>> connection string.  It seems to me that it would make sense to move
>> those two into PostgresNode::psql itself and extend it to be able to
>> handle custom connection strings?
> Looking at this part, I think that this is a win in terms of future
> changes for SSLServer.pm as it would become a facility only in charge
> of managing the backend's SSL configuration.  This has also the
> advantage to make the error handling with psql more consistent with
> the other tests.
>
> So, attached is a patch to do this simplification.  The bulk of the
> changes is within the tests themselves to adapt to the merge of
> $common_connstr and $connstr for the new routines of PostgresNode.pm,
> and I have done things this way to ease the patch lookup.  Thoughts?



Looks reasonable.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

RE: libpq debug log

[iwata....@fujitsu.com]

Hi Alvaro san, Tsunakawa san

Thank you for creating the v30 patch.

> From: Tsunakawa, Takayuki/綱川 貴之 
> Sent: Monday, March 29, 2021 9:45 AM
...
> Iwata-san,
> Please review Alvaro-san's code, and I think you can integrate all patches 
> into
> one except for 0002 and 0007.  Those two patches may be separate or
> merged into one as a test patch.

I reviewed v30 patches. I think it was good except that the documentation about 
the direction of the message was not changing.
I also tried the v30 patch using regression test and it worked fine.
I merged v30 patches and update the patch to v31.

This new version patch includes the fix of libpq.smgl fix and the addition of 
regression test mode.
In libpq.smgl, the symbol indicating the message direction has been corrected 
from "<" ">" to "B" "F" in the documentation.

> From: alvhe...@alvh.no-ip.org 
> Sent: Sunday, March 28, 2021 4:28 AM
...
> Maybe the easiest way is to have a new flag PQTRACE_REGRESS_MODE.

To prepare for regression test, I read Message Formats documentation.
https://www.postgresql.org/docs/current/protocol-message-formats.html

Following protocol messages have values that depend on the code of master at 
that time;
- BackendKeyData(B) ... includes backend PID and backend private key
- ErrorResponse(B) ... includes error message line number
- FunctionCall(F) ... includes function OID
- NoticeResponse(B) ... includes notice message line number
- NotificationResponse (B) ... includes backend PID
- ParameterDescription  ... includes parameter OID
- Parse(F) ... includes parameter data type OID
- RowDescription(B) ... includes OIDs

I checked status of conn->pqTraceFlags to decide whether output 
version-dependent messages or not in above protocol message output functions.
In ErrorResponse and NoticeResponse, I skip string type message logging only 
when field type code is "L".
In my understanding, other field code message type does not depend on version.
So I didn't skip other code type's string messages.
And I also changed description of pqTraceSetFlags()
by changing PQTRACE_SUPPRESS_TIMESTAMPS flag to the PQTRACE_REGRESS_MODE flag.

Output of regress mode is following;

B   124 ErrorResponseS "ERROR" V "ERROR" C "22023" M "unrecognized 
parameter "some_nonexistent_parameter"" F "reloptions.c" L R 
"parseRelOptionsInternal" \x00
B   14  ParameterDescription 2   

Output of non-regress mode is following;

2021-03-30 12:55:31.327913  B   124 ErrorResponseS "ERROR" V "ERROR" C 
"22023" M "unrecognized parameter "some_nonexistent_parameter"" F 
"reloptions.c" L "1447" R "parseRelOptionsInternal" \x00
2021-03-30 12:56:12.691617  B   14  ParameterDescription 2 25 701

Regards,
Aya Iwata


v31-libpq-trace-log.patch
Description: v31-libpq-trace-log.patch

Re: [HACKERS] Custom compression methods

[Tom Lane]

Robert Haas  writes:
> On Wed, Mar 24, 2021 at 2:15 PM Tom Lane  wrote:
>> But let's ignore the case of pg_upgrade and just consider a dump/restore.
>> I'd still say that unless you give --no-toast-compression then I would
>> expect the dump/restore to preserve the tables' old compression behavior.
>> Robert's argument that the pre-v14 database had no particular compression
>> behavior seems nonsensical to me.  We know exactly which compression
>> behavior it has.

> I said that it didn't have a state, not that it didn't have a
> behavior. That's not exactly the same thing. But I don't want to argue
> about it, either. It's a judgement call what's best here, and I don't
> pretend to have all the answers. If you're sure you've got it right
> ... great!

I've not heard any other comments about this, but I'm pretty sure that
preserving a table's old toast behavior is in line with what we'd normally
expect pg_dump to do --- especially in light of the fact that we did not
provide any --preserve-toast-compression switch to tell it to do so.
So I'm going to go change it.

regards, tom lane

DROP INDEX docs - explicit lock naming

[Greg Rychlewski]

Hi,

While reading the documentation for DROP INDEX[1], I noticed the lock was
described colloquially as an "exclusive" lock, which made me pause for a
second because it's the same name as the EXCLUSIVE table lock.

The attached patch explicitly states that an ACCESS EXCLUSIVE lock is
acquired.

[1]
https://www.postgresql.org/docs/current/sql-dropindex.html


v1-drop-index-doc.patch
Description: Binary data

Re: Idea: Avoid JOINs by using path expressions to follow FKs

[Rod Taylor]

On Sat, 27 Mar 2021 at 16:28, Joel Jacobson  wrote:

> Hi,
>
> The database Neo4j has a language called "Cypher" where one of the key
> selling points is they "don’t need join tables".
>
> Here is an example from
> https://neo4j.com/developer/cypher/guide-sql-to-cypher/
>
> SQL:
>
> SELECT DISTINCT c.company_name
> FROM customers AS c
> JOIN orders AS o ON c.customer_id = o.customer_id
> JOIN order_details AS od ON o.order_id = od.order_id
> JOIN products AS p ON od.product_id = p.product_id
> WHERE p.product_name = 'Chocolade';
>
> Neo4j's Cypher:
>
> MATCH (p:product
> {product_name:"Chocolade"})<-[:PRODUCT]-(:order)<-[:PURCHASED]-(c:customer)
> RETURN distinct c.company_name;
>
> Imagine if we could simply write the SQL query like this:
>
> SELECT DISTINCT od.order_id.customer_id.company_name
> FROM order_details AS od
> WHERE od.product_id.product_name = 'Chocolade';
>

I regularly do this type of thing via views. It's a bit confusing as writes
go to one set of tables while selects often go through the view with all
the details readily available.

I think I'd want these shortcuts to be well defined and obvious to someone
exploring via psql. I can also see uses where a foreign key might not be
available (left join rather than join).

I wonder if GENERATED ... VIRTUAL might be a way of defining this type of
added record.

ALTER TABLE order ADD customer record GENERATED JOIN customer USING
(customer_id) VIRTUAL;
ALTER TABLE order_detail ADD order record GENERATED JOIN order USING
(order_id) VIRTUAL;

SELECT order.customer.company_name FROM order_detail;

Of course, if they don't reference the GENERATED column then the join isn't
added to the query.

--
Rod Taylor

Re: Issue with point_ops and NaN

[Tom Lane]

Julien Rouhaud  writes:
> On Tue, Mar 30, 2021 at 02:47:05PM +0200, Laurenz Albe wrote:
>> I'd say that this is certainly wrong:
>> SELECT point('NaN','NaN') <@ polygon('(0,0),(1,0),(1,1),(0,0)');
>> 
>> ?column? 
>> --
>>  t
>>  (1 row)

> Yeah that's what I think too, but I wanted to have confirmation.

Agreed --- one could make an argument for either 'false' or NULL
result, but surely not 'true'.

I wonder if Horiguchi-san's patch [1] improves this case.

regards, tom lane

[1] https://commitfest.postgresql.org/32/2710/

Re: Refactor SSL test framework to support multiple TLS libraries

[Alvaro Herrera]

On 2021-Mar-30, Michael Paquier wrote:

> On Tue, Mar 30, 2021 at 03:50:28PM +0900, Michael Paquier wrote:
> > The test_*() ones are just wrappers for psql able to use a customized
> > connection string.  It seems to me that it would make sense to move
> > those two into PostgresNode::psql itself and extend it to be able to
> > handle custom connection strings?
> 
> Looking at this part, I think that this is a win in terms of future
> changes for SSLServer.pm as it would become a facility only in charge
> of managing the backend's SSL configuration.  This has also the
> advantage to make the error handling with psql more consistent with
> the other tests.
> 
> So, attached is a patch to do this simplification.  The bulk of the
> changes is within the tests themselves to adapt to the merge of
> $common_connstr and $connstr for the new routines of PostgresNode.pm,
> and I have done things this way to ease the patch lookup.  Thoughts?

I agree this seems a win.

The only complain I have is that "the given node" is nonsensical in
PostgresNode.  I suggest to delete the word "given".  Also "This is
expected to fail with a message that matches the regular expression
$expected_stderr".

The POD doc for connect_fails uses order: ($connstr, $testname, 
$expected_stderr)
but the routine has:
  +   my ($self, $connstr, $expected_stderr, $testname) = @_;

these should match.

(There's quite an inconsistency in the existing test code about
expected_stderr being a string or a regex; and some regexes are quite
generic: just qr/SSL error/.  Not this patch responsibility to fix that.)

As I understand, our perlcriticrc no longer requires 'return' at the end
of routines (commit 0516f94d18c5), so you can omit that.

-- 
Álvaro Herrera   Valdivia, Chile

Re: Issue with point_ops and NaN

[Julien Rouhaud]

On Tue, Mar 30, 2021 at 11:02:32AM -0400, Tom Lane wrote:
> Julien Rouhaud  writes:
> > On Tue, Mar 30, 2021 at 02:47:05PM +0200, Laurenz Albe wrote:
> >> I'd say that this is certainly wrong:
> >> SELECT point('NaN','NaN') <@ polygon('(0,0),(1,0),(1,1),(0,0)');
> >> 
> >> ?column? 
> >> --
> >>  t
> >>  (1 row)
> 
> > Yeah that's what I think too, but I wanted to have confirmation.
> 
> Agreed --- one could make an argument for either 'false' or NULL
> result, but surely not 'true'.

I would think that it should return NULL since it's not inside nor outside the
polygon, but I'm fine with false.

> I wonder if Horiguchi-san's patch [1] improves this case.

Oh I totally missed that patch :(

After a quick look I see this addition in point_inside():

+   /* NaN makes the point cannot be inside the polygon */
+   if (unlikely(isnan(x) || isnan(y)))
+   return 0;

So I would assume that it should fix this case too.  I'll check tomorrow.

Re: Autovacuum on partitioned table (autoanalyze)

[Tomas Vondra]

On 3/30/21 4:09 AM, Tomas Vondra wrote:
> Hi,
> 
> ...
> 
> We may need to "sync" the counts for individual relations in a couple
> places (e.g. after the worker is done with the leaf, it should propagate
> the remaining delta before resetting the values to 0). Maybe multi-level
> partitioning needs some additional handling, not sure.
> 

I forgot to mention one additional thing yesterday - I wonder if we need
to do something similar after a partition is attached/detached. That can
also change the parent's statistics significantly, so maybe we should
handle all partition's rows as changes_since_analyze? Not necessarily
something this patch has to handle, but might be related.


regards

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

Re: [PATCH] Add --create-only option to pg_dump/pg_dumpall

[Michael Banck]

Hi,

Am Montag, den 29.03.2021, 17:59 + schrieb Cary Huang:
> I have tried the patch and the new option is able to control the
> contents of pg_dump outputs to include only create db related
> commands. 

Thanks for testing!

> I also agree that the option name is a little misleading to the user
> so I would suggest instead of using "create-only", you can say
> something maybe like "createdb-only" because this option only applies
> to CREATE DATABASE related commands, not CREATE TABLE or other
> objects. In the help menu, you can then elaborate more that this
> option "dump only the commands related to create database like ALTER,
> GRANT..etc"

Well I have to say I agree with Peter that the option name I came up
with is pretty confusing, not sure createdb-only is much better as it
also includes GRANTs etc.

I think from a technical POV it's useful as it closes a gap between
pg_dumpall -g and pg_dump -Fc $DATABASE in my opinion, without having to
potentially schema-dump and filter out a large number of database
objects.

Anybody else have some opinions on what to call this best? Maybe just a
short option and some explanatory text in --help along with it?


Michael

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

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

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

Re: truncating timestamps on arbitrary intervals

[John Naylor]

Currently, when the origin is after the input, the result is the timestamp
at the end of the bin, rather than the beginning as expected. The attached
puts the result consistently at the beginning of the bin.

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


rationalize-future-origin.patch
Description: Binary data

Re: Hybrid Hash/Nested Loop joins and caching results from subplans

[Zhihong Yu]

Hi,
In paraminfo_get_equal_hashops(),

+   /* Reject if there are any volatile functions */
+   if (contain_volatile_functions(expr))
+   {

You can move the above code to just ahead of:

+   if (IsA(expr, Var))
+   var_relids = bms_make_singleton(((Var *) expr)->varno);

This way, when we return early, var_relids doesn't need to be populated.

Cheers

On Tue, Mar 30, 2021 at 4:42 AM David Rowley  wrote:

> On Mon, 29 Mar 2021 at 15:56, Zhihong Yu  wrote:
> > For show_resultcache_info()
> >
> > +   if (rcstate->shared_info != NULL)
> > +   {
> >
> > The negated condition can be used with a return. This way, the loop can
> be unindented.
>
> OK. I change that.
>
> > + * ResultCache nodes are intended to sit above a parameterized node in
> the
> > + * plan tree in order to cache results from them.
> >
> > Since the parameterized node is singular, it would be nice if 'them' can
> be expanded to refer to the source of result cache.
>
> I've done a bit of rewording in that paragraph.
>
> > +   rcstate->mem_used -= freed_mem;
> >
> > Should there be assertion that after the subtraction, mem_used stays
> non-negative ?
>
> I'm not sure.  I ended up adding one and also adjusting the #ifdef in
> remove_cache_entry() which had some code to validate the memory
> accounting so that it compiles when USE_ASSERT_CHECKING is defined.
> I'm unsure if that's a bit too expensive to enable during debugs but I
> didn't really want to leave the code in there unless it's going to get
> some exercise on the buildfarm.
>
> > +   if (found && entry->complete)
> > +   {
> > +   node->stats.cache_hits += 1;/* stats update */
> >
> > Once inside the if block, we would return.
>
> OK change.
>
> > +   else
> > +   {
> > The else block can be unindented (dropping else keyword).
>
> changed.
>
> > +* return 1 row.  XXX is this worth the check?
> > +*/
> > +   if (unlikely(entry->complete))
> >
> > Since the check is on a flag (with minimal overhead), it seems the check
> can be kept, with the question removed.
>
> I changed the comment, but I did leave a mention that I'm still not
> sure if it should be an Assert() or an elog.
>
> The attached patch is an updated version of the Result Cache patch
> containing the changes for the things you highlighted plus a few other
> things.
>
> I pushed the change to simplehash.h and the estimate_num_groups()
> change earlier, so only 1 patch remaining.
>
> Also, I noticed the CFBof found another unstable parallel regression
> test. This was due to some code in show_resultcache_info() which
> skipped parallel workers that appeared to not help out. It looks like
> on my machine the worker never got a chance to do anything, but on one
> of the CFbot's machines, it did.  I ended up changing the EXPLAIN
> output so that it shows the cache statistics regardless of if the
> worker helped or not.
>
> David
>

Re: truncating timestamps on arbitrary intervals

[John Naylor]

On Sat, Mar 27, 2021 at 1:06 PM Justin Pryzby  wrote:
>
> The current docs seem to be missing a "synopsis", like
>
> +
> +date_trunc(stride,
timestamp, origin)
> +

The attached
- adds a synopsis
- adds a bit more description to the parameters similar to those in
date_trunc
- documents that negative intervals are treated the same as positive ones

Note on the last point: This just falls out of the math, so was not
deliberate, but it seems fine to me. We could ban negative intervals, but
that would possibly just inconvenience some people unnecessarily. We could
also treat negative strides differently somehow, but I don't immediately
see a useful and/or intuitive change in behavior to come of that.

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


synopsis-and-add-to-description.patch
Description: Binary data

Re: Autovacuum worker doesn't immediately exit on postmaster death

[Stephen Frost]

Greetings,

* Stephen Frost (sfr...@snowman.net) wrote:
> * Stephen Frost (sfr...@snowman.net) wrote:
> > * Michael Paquier (mich...@paquier.xyz) wrote:
> > > On Mon, Mar 22, 2021 at 04:07:12PM -0400, Robert Haas wrote:
> > > > On Mon, Mar 22, 2021 at 1:48 PM Stephen Frost  
> > > > wrote:
> > > >> Thanks for that.  Attached is just a rebased version with a commit
> > > >> message added.  If there aren't any other concerns, I'll commit this in
> > > >> the next few days and back-patch it.  When it comes to 12 and older,
> > > >> does anyone want to opine about the wait event to use?  I was thinking
> > > >> PG_WAIT_TIMEOUT or WAIT_EVENT_PG_SLEEP ...
> > > > 
> > > > I'm not sure if we should back-patch this, but I think if you do you
> > > > should just add a wait event, rather than using a generic one.
> > > 
> > > I would not back-patch that either, as this is an improvement of the
> > > current state.  I agree that this had better introduce a new wait
> > > event.  Even if this stuff gets backpatched, you won't introduce an
> > > ABI incompatibility with a new event as long as you add the new event
> > > at the end of the existing enum lists, but let's keep the wait events
> > > ordered on HEAD.
> > 
> > Adding CFI's in places that really should have them is something we
> > certainly have back-patched in the past, and that's just 'an improvement
> > of the current state' too, so I don't quite follow the argument being
> > made here that this shouldn't be back-patched.
> > 
> > I don't have any problem with adding into the older releases, at the end
> > of the existing lists, the same wait event that exists in 13+ for this
> > already.
> > 
> > Any other thoughts on this, particularly about back-patching or not..?
> 
> We seem to be at a bit of an impasse on this regarding back-patching,
> which seems unfortunate to me, but without someone else commenting it
> seems like it's stalled.
> 
> I'll go ahead and push the change to HEAD soon, as there doesn't seem to
> be any contention regarding that.

Done.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: SQL/JSON: JSON_TABLE

[Erik Rijkers]

> On 2021.03.27. 02:12 Nikita Glukhov  wrote:
> 
> Attached 47th version of the patches.
> 
[..]
> 
> I have added forgotten files and fixed the first patch.
> 
> [0001-SQL-JSON-functions-v47.patch]
> [0002-JSON_TABLE-v47.patch]
> [0003-JSON_TABLE-PLAN-DEFAULT-clause-v47.patch]
> [0004-JSON_TABLE-PLAN-clause-v47.patch]

Hi,

Apply, build all fine.  It also works quite well, and according to 
specification, as far as I can tell.

But today I ran into:

ERROR:  function ExecEvalJson not in llvmjit_types.c

I think that it is caused by:

set enable_bitmapscan = off;

(I installed llvm a few days ago. llvm-3.9-dev on this debian stretch).


This is the test sql I concocted, which runs fine with enable_bitmapscan on 
(the default):

select jt1.* 
from myjsonfile100k as t(js, id) 
  , json_table(
  t.js
   , '$' columns (
"lastname"   textpath  '$. "lastname" '
  , "firstname"  textpath  '$. "firstname"'
  , "date"   textpath  '$. "date" '
  , "city"   textpath  '$. "city" '
  , "country"textpath  '$. "country"  '
  , "name 0(1)"  textpath  '$. "array"[0] '
  , "name 4(5)"  textpath  '$. "array"[4] '
  , "names"  text[]  path  '$. "array"'
  , "randfloat"  float   path  '$. "random float" '
)
) as jt1
where  js @> ('[ { "city": "Santiago de Cuba" } ]')
   and js[0]->>'firstname' = 'Gilda'
;
ERROR:  function ExecEvalJson not in llvmjit_types.c

That statement only errors out if the table is large enough. I have no time now 
to make a sample table but if no-one understands the problem off-hand, I'll try 
to construct such a table later this week (the one I'm using is large, 1.5 GB).


Erik Rijkers

Re: Proposal: Save user's original authenticated identity for logging

[Jacob Champion]

On Tue, 2021-03-30 at 09:55 +0900, Michael Paquier wrote:
> On Mon, Mar 29, 2021 at 11:53:03PM +, Jacob Champion wrote:
> > It's not a matter of the tests being stable, but of the tests needing
> > to change and evolve as the implementation changes. A big part of that
> > is visibility into what the tests are doing, so that you can debug
> > them.
> 
> Sure, but I still don't quite see why this applies here?  At the point
> of any test, like() or unlink() print the contents of the comparison
> if there is a failure, so there is no actual loss of data.  That's
> what issues_sql_like() does, for one.

The key there is "if there is a failure" -- false positives need to be
debugged too. Tests I've worked with recently for the NSS work were
succeeding for the wrong reasons. Overly generic logfile matches ("SSL
error"), for example.

> > Keep in mind that the rotate-restart-slurp method comes from an
> > existing test. I assume Andrew chose that method for the same reasons I
> > did -- it works with what we currently have.
> 
> PostgresNode::rotate_logfile got introduced in c098509, and it is just
> used in t/017_shm.pl on HEAD.  There could be more simplifications
> with 019_replslot_limit.pl, I certainly agree with that.

modules/ssl_passphrase_callback/t/001_testfunc.pl is where I pulled
this pattern from.

> > Is the additional effort to create (and maintain) that new system worth
> > two seconds per run? I feel like it's not -- but if you feel strongly
> > then I can definitely look into it.
> 
> I fear that heavily parallelized runs could feel the difference.  Ask
> Andres about that, he has been able to trigger in parallel a failure
> with pg_upgrade wiping out testtablespace while the main regression
> test suite just began :) 

Does unilateral log truncation play any nicer with parallel test runs?
I understand not wanting to make an existing problem worse, but it
doesn't seem like the existing tests were written for general
parallelism.

Would it be acceptable to adjust the tests for live rotation using the
logging collector, rather than a full restart? It would unfortunately
mean that we have to somehow wait for the rotation to complete, since
that's asynchronous.

(Speaking of asynchronous: how does the existing check-and-truncate
code make sure that the log entries it's looking for have been flushed
to disk? Shutting down the server guarantees it.)

> > Which parts would you consider confusing/in need of change? I'm happy
> > to expand where needed. Would an inline sample be more helpful than a
> > textual explanation?
> 
> That's with the use of "authentication and authorization".  How can
> users make the difference between what one or this other is without
> some explanation with the name maps?  It seems that there is no place
> in the existing docs where this difference is explained.  I am
> wondering if it would be better to not change this paragraph, or
> reword it slightly to outline that this may cause more than one log
> entry, say:
> "Causes each attempted connection to the server, and each
> authentication activity to be logged."

I took a stab at this in v13: "Causes each attempted connection to the
server to be logged, as well as successful completion of both client
authentication (if necessary) and authorization." (IMO any further in-
depth explanation of authn/z and user mapping probably belongs in the
auth method documentation, and this patch doesn't change any authn/z
behavior.)

v13 also incorporates the latest SSL cert changes, so it's just a
single patch now. Tests now cover the CN and DN clientname modes. I
have not changed the log capture method yet; I'll take a look at it
next.

--Jacob
From 48cfecfb71ac276bede9abe3e374ebee8f428117 Mon Sep 17 00:00:00 2001
From: Jacob Champion 
Date: Wed, 3 Feb 2021 11:42:05 -0800
Subject: [PATCH v13] Log authenticated identity from all auth backends

The "authenticated identity" is the string used by an auth method to
identify a particular user. In many common cases this is the same as the
Postgres username, but for some third-party auth methods, the identifier
in use may be shortened or otherwise translated (e.g. through pg_ident
mappings) before the server stores it.

To help DBAs see who has actually interacted with the system, store the
original identity when authentication succeeds, and expose it via the
log_connections setting. The log entries look something like this
example (where a local user named "pchampion" is connecting to the
database as the "admin" user):

LOG:  connection received: host=[local]
LOG:  connection authenticated: identity="pchampion" method=peer (/data/pg_hba.conf:88)
LOG:  connection authorized: user=admin database=postgres application_name=psql

port->authn_id is set according to the auth method:

bsd: the Postgres username (which is the local username)
cert: the client's Subject DN
gss: the user principal
ident: the remote username
ldap: the final bind DN
pam: the Postgres

Re: Stronger safeguard for archive recovery not to miss data

[Fujii Masao]

On 2021/03/26 22:14, David Steele wrote:

On 3/25/21 9:23 PM, Fujii Masao wrote:


On 2021/03/25 23:21, David Steele wrote:

On 1/25/21 3:55 AM, Laurenz Albe wrote:

On Mon, 2021-01-25 at 08:19 +, osumi.takami...@fujitsu.com wrote:

I think you should pst another patch where the second, now superfluous, error
message is removed.


Updated. This patch showed no failure during regression tests
and has been aligned by pgindent.


Looks good to me.
I'll set it to "ready for committer" again.


Fujii, does the new patch in [1] address your concerns?


No. I'm still not sure if this patch is good idea... I understand
why this safeguard is necessary. OTOH I'm afraid it increases
a bit the risk that users get unstartable database, i.e., lose whole database.
But maybe I'm concerned about rare case and my opinion is minority one.
So I'd like to hear more opinions about this patch.


After reviewing the patch I am +1. I think allowing corruption in recovery by 
default is not a good idea. There is currently a warning but I don't think that 
is nearly strong enough and is easily missed.

Also, "data may be missing" makes this sound like an acceptable situation. What 
is really happening is corruption is being introduced that may make the cluster unusable 
or at the very least lead to errors during normal operation.


Ok, now you, Osumi-san and Laurenz agree to this change
while I'm only the person not to like this. So unless we can hear
any other objections to this change, probably we should commit the patch.


If we want to allow recovery to play past this point I think it would make more 
sense to have a GUC (like ignore_invalid_pages [1]) that allows recovery to 
proceed and emits a warning instead of fatal.

Looking the patch, I see a few things:

1) Typo in the tests:

This protection shold apply to recovery mode

should be:

This protection should apply to recovery mode

2) I don't think it should be the job of this patch to restructure the if 
conditions, even if it is more efficient. It just obscures the purpose of the 
patch.


+1


So, I would revert all the changes in xlog.c except changing the warning to an 
error:

-    ereport(WARNING,
-    (errmsg("WAL was generated with wal_level=minimal, data may be 
missing"),
- errhint("This happens if you temporarily set wal_level=minimal 
without taking a new base backup.")));
+    ereport(FATAL,
+    (errmsg("WAL was generated with wal_level=minimal, cannot 
continue recovering"),
+ errdetail("This happens if you temporarily set 
wal_level=minimal on the server."),
+ errhint("Run recovery again from a new base backup taken after 
setting wal_level higher than minimal")));

I guess that users usually encounter this error because they have not
taken base backups yet after setting wal_level to higher than minimal
and have to use the old base backup for archive recovery. So I'm not sure
how much only this HINT is helpful for them. Isn't it better to append
something like "If there is no such backup, recover to the point in time
before wal_level is set to minimal even though which cause data loss,
to start the server." into HINT?

The following error will never be thrown because of the patch.
So IMO the following code should be removed.

if (ControlFile->wal_level < WAL_LEVEL_REPLICA)
ereport(ERROR,
(errmsg("hot standby is not possible because wal_level 
was not set to \"replica\" or higher on the primary server"),
 errhint("Either set wal_level to 
\"replica\" on the primary, or turn off hot_standby here.")));

Regards,

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

multi-install PostgresNode fails with older postgres versions

[Mark Dilger]

Andrew,

While developing some cross version tests, I noticed that PostgresNode::init 
fails for postgres versions older than 9.3, like so:

# Checking port 52814
# Found port 52814
Name: 9.2.24
Data directory: 
/Users/mark.dilger/hydra/postgresnode.review/src/test/modules/test_cross_version/tmp_check/t_001_verify_paths_9.2.24_data/pgdata
Backup directory: 
/Users/mark.dilger/hydra/postgresnode.review/src/test/modules/test_cross_version/tmp_check/t_001_verify_paths_9.2.24_data/backup
Archive directory: 
/Users/mark.dilger/hydra/postgresnode.review/src/test/modules/test_cross_version/tmp_check/t_001_verify_paths_9.2.24_data/archives
Connection string: port=52814 
host=/var/folders/6n/3f4vwbnn7fz5qk0xqhgbjrkwgp/T/L_A2w1x7qb
Log file: 
/Users/mark.dilger/hydra/postgresnode.review/src/test/modules/test_cross_version/tmp_check/log/001_verify_paths_9.2.24.log
# Running: initdb -D 
/Users/mark.dilger/hydra/postgresnode.review/src/test/modules/test_cross_version/tmp_check/t_001_verify_paths_9.2.24_data/pgdata
 -A trust -N
initdb: invalid option -- N
Try "initdb --help" for more information.
Bail out!  system initdb failed

The problem is clear enough; -N/--nosync was added in 9.3, and 
PostgresNode::init is passing -N to initdb unconditionally. I wonder if during 
PostgresNode::new a call should be made to pg_config and the version 
information grep'd out so that version specific options to various functions 
(init, backup, etc) could hinge on the version of postgres being used?

You could also just remove the -N option, but that would slow down all tests 
for everybody, so I'm not keen to do that.  Or you could remove -N in cases 
where $node->{_install_path} is defined, which would be far more acceptable.  
I'm leaning towards using the output of pg_config, though, since this problem 
is likely to come up again with other options/commands.

Thoughts?

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

Re: Failed assertion on standby while shutdown

[Maxim Orlov]

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

All the tests passed successfully.

The new status of this patch is: Ready for Committer

Re: Idea: Avoid JOINs by using path expressions to follow FKs

[Joel Jacobson]

On Tue, Mar 30, 2021, at 16:25, Rod Taylor wrote:
> On Sat, 27 Mar 2021 at 16:28, Joel Jacobson  wrote:
>> __Imagine if we could simply write the SQL query like this:
>> 
>> SELECT DISTINCT od.order_id.customer_id.company_name
>> FROM order_details AS od
>> WHERE od.product_id.product_name = 'Chocolade';
> 
> I regularly do this type of thing via views. It's a bit confusing as writes 
> go to one set of tables while selects often go through the view with all the 
> details readily available.
> 
> I think I'd want these shortcuts to be well defined and obvious to someone 
> exploring via psql. I can also see uses where a foreign key might not be 
> available (left join rather than join).
> 
> I wonder if GENERATED ... VIRTUAL might be a way of defining this type of 
> added record.
> 
> ALTER TABLE order ADD customer record GENERATED JOIN customer USING 
> (customer_id) VIRTUAL;
> ALTER TABLE order_detail ADD order record GENERATED JOIN order USING 
> (order_id) VIRTUAL;
> 
> SELECT order.customer.company_name FROM order_detail;
> 
> Of course, if they don't reference the GENERATED column then the join isn't 
> added to the query.

Interesting idea, but not sure I like it, since you would need twice as many 
columns,
and you would still need the foreign keys, right?

/Joel

Re: Calendar support in localization

[Daniel Verite]

Surafel Temesgen wrote:

> > About intervals, if there were locale-aware functions like
> >  add_interval(timestamptz, interval [, locale]) returns timestamptz
> > or
> >  sub_timestamp(timestamptz, timestamptz [,locale]) returns interval
> > that would use ICU to compute the results according to the locale,
> > wouldn't it be good enough?
> >
> >
> Yes it can be enough for now but there are patches proposed to support the
> system and application time period which are in SQL standard 

To clarify, these function signatures are not meant to oppose
a core vs extension implementation, nor an ICU vs non-ICU
implementation. They're meant to illustrate the case of using
specific functions instead of adding specific data types.
AFAIU, adding data types come from the idea that since
(non-gregorian-date + interval) doesn't have the same result as
(gregorian-date + interval), we could use a different type for
non-gregorian-date and so a different "+" operator, maybe
even a specific interval type.

For the case of temporal tables, I'm not quite familiar with the
feature, but I notice that the patch says:

+When system versioning is specified two columns are added which
+record the start timestamp and end timestamp of each row verson.
+The data type of these columns will be TIMESTAMP WITH TIME ZONE.

The user doesn't get to choose the data type, so if we'd require to
use specific data types for non-gregorian calendars, that would
seemingly complicate things for this feature. This is consistent
with the remark upthread that the SQL standard assumes the
gregorian calendar.


> what it takes to support calendar locally is input/output function
> and a converter from and to julian calendar and that may not be that
> much hard since most of the world calendar is based on julian or
> gregorian calendar[0]

The conversions from julian dates are not necessarily hard, but the
I/O functions means having localized names for all days, months, eras
of all calendars in all supported languages. If you're thinking of
implementing this from scratch (without the ICU dependency), where
would these names come from? OTOH if we're using ICU, then why
bother reinventing the julian-to-calendars conversions that ICU
already does?


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: https://www.manitou-mail.org
Twitter: @DanielVerite

Re: pgbench - add pseudo-random permutation function

[Fabien COELHO]

Hello Dean,

Thanks a lot for this work. This version looks much better than the 
previous one you sent, and has significant advantages over the one I sent, 
in particular avoiding the prime number stuff and large modular multiply.

So this looks good!

I'm happy that halves-xoring is back because it was an essential part of 
the design. ISTM that the previous version you sent only had linear/affine 
transforms which was a bad idea.


The linear transform is moved inside halves, why not, and the 
any-odd-number multiplication is prime with power-of-two trick is neat on 
these.


However I still have some reservations:

First, I have a thing against erand48. The erand 48 bits design looks like 
something that made sense when computer architectures where 16/32 bits and 
large integers were 32 bits, so a 16 bits margin looked good enough. Using 
a int16[3] type now is really depressing and probably costly on modern 
architectures. With 64 bits architecture, and given that we are 
manipulating 64 bits integers (well 63 really), ISTM that the minimal 
state size for a PRNG should be at least 64 bits. It there a good reason 
NOT to use a 64 bits state prn generator? I took Knuth's because it is 
cheap and 64 bits. I'd accept anything which is at least 64 bits. I looked 
at xor-shift things, but there was some controversy around these so I kept 
it simple and just shifted small values to get rid of the obvious cycles 
on Knuth's generator.


Second, I have a significant reservation about the very structure of the 
transformation in this version:


  loop 4 times :

// FIRST HALF STEER
m/r = pseudo randoms
if v in first "half"
   v = ((v * m) ^ r) & mask;
   rotate1(v)

// FULL SHIFT 1
r = pseudo random
v = (v + r) % size

// SECOND HALF STEER
m/r = pseudo randoms
if v in second "half"
   same as previous on second half

// FULL SHIFT 2
r = pseudo random
v = (v + r) % size

I'm really at odds with FULL SHIFT 1, because it means that up to 1/256 of 
values are kept out of STEERING. Whole chunks of values could be kept 
unshuffled because they would only have SHIFTS apply to them and each time 
fall in the not steered half. It should be an essential part of the design 
that at least one steer is applied on a value at each round, and if two 
are applied then fine, but certainly not zero. So basically I think that 
the design would be significantly improved by removing "FULL SHIFT 1".


Third, I think that the rotate code can be simplified, in particular the 
?: should be avoided because it may induce branches quite damaging to 
processor performance.


I'll give it some more thoughts.

--
Fabien.

Re: Idea: Avoid JOINs by using path expressions to follow FKs

[Joel Jacobson]

On Mon, Mar 29, 2021, at 16:17, Vik Fearing wrote:
> SELECT DISTINCT order_details."order"->customer->company_name
> FROM order_details
> WHERE order_details.product->product_name = 'Chocolade';

I like the idea of using -> instead of . (dot),
since name resolution is already complicated,
so overloading the dot operator feels like a bad idea.

I therefore propose the following syntax:

{ table_name | alias } -> constraint_name [ [ -> constraint_name ... ] -> 
column_name ]

It's necessary to start with the table name or its alias,
since two tables/aliases used in the same query
might have different constraints with the same name.

If the expression ends with a column_name,
you get the value for the column.

If the expression ends with a constraint_name,
you get the referenced table as a record.

I also have a new idea on how we can use
the nullability of the foreign key's column(s),
as a rule to determine if you would get
a LEFT JOIN or an INNER JOIN:

If ALL of the foreign key column(s) are declared as NOT NULL,
then you would get an INNER JOIN.

If ANY of the foreign key column(s) are declared as NULL,
then you would get a LEFT JOIN.

Thoughts?

/Joel

Re: making update/delete of inheritance trees scale better

[Tom Lane]

I wrote:
> I've not made any attempt to do performance testing on this,
> but I think that's about the only thing standing between us
> and committing this thing.

I think the main gating condition for committing this is "does it
make things worse for simple non-partitioned updates?".  The need
for an extra tuple fetch per row certainly makes it seem like there
could be a slowdown there.  However, in some tests here with current
HEAD (54bb91c30), I concur with Amit's findings that there's barely
any slowdown in that case.  I re-did Heikki's worst-case example [1]
of updating both columns of a 2-column table.  I also tried variants
of that involving updating two columns of a 6-column table and of a
10-column table, figuring that those cases might be a bit more
representative of typical usage (see attached scripts).  What I got
was

Times in ms, for the median of 3 runs:

Table width HEADpatch   HEADpatch
-- jit on ---   -- jit off --

2 columns   12528   13329   12574   12922
6 columns   15861   15862   14775   15249
10 columns  18399   16985   16994   16907

So even with the worst case, it's not too bad, just a few percent
worse, and once you get to a reasonable number of columns the v13
patch is starting to win.

However, I then tried a partitioned equivalent of the 6-column case
(script also attached), and it looks like

6 columns   16551   19097   15637   18201

which is really noticeably worse, 16% or so.  I poked at it with
"perf" to see if there were any clear bottlenecks, and didn't find
a smoking gun.  As best I can tell, the extra overhead must come
from the fact that all the tuples are now passing through an Append
node that's not there in the old-style plan tree.  I find this
surprising, because a (non-parallelized) Append doesn't really *do*
much; it certainly doesn't add any data copying or the like.
Maybe it's not so much the Append as that the rules for what kind of
tuple slot can be used have changed somehow?  Andres would have a
clearer idea about that than I do.

Anyway, I'm satisfied that this patch isn't likely to seriously
hurt non-partitioned cases.  There may be some micro-optimization
that could help simple partitioned cases, though.

This leaves us with a question whether to commit this patch now or
delay it till we have a better grip on why cases like this one are
slower.  I'm inclined to think that since there are a lot of clear
wins for users of partitioning, we shouldn't let the fact that there
are also some losses block committing.  But it's a judgment call.

regards, tom lane

[1] 
https://www.postgresql.org/message-id/2e50d782-36f9-e723-0c4b-d133e63c6127%40iki.fi

drop table if exists tab;
create unlogged table tab (a int4, b int4, f3 int, f4 int, f5 text, f6 float8);
\timing on
insert into tab select g, g, g, g, g::text, g from generate_series(1, 1000) 
g;
vacuum tab;
explain verbose update tab set b = b, f6 = f6;
update tab set b = b, f6 = f6;
drop table tab;
create unlogged table tab (a int4, b int4, f3 int, f4 int, f5 text, f6 float8);
\timing on
insert into tab select g, g, g, g, g::text, g from generate_series(1, 1000) 
g;
vacuum tab;
explain update tab set b = b, a = a;
update tab set b = b, a = a;
drop table if exists tab;
create unlogged table tab (a int4, b int4, f3 int, f4 int, f5 text, f6 float8)
partition by range(a);
do $$
begin
for i in 0..9 loop
  execute 'create unlogged table tab'||i||' partition of tab for values from
 ('||i*100||') to ('||(i+1)*100||');';
end loop;
end$$;

\timing on
insert into tab select g, g, g, g, g::text, g from generate_series(1, 
1000-1) g;
vacuum tab;
explain verbose update tab set b = b, f6 = f6;
update tab set b = b, f6 = f6;

Re: making update/delete of inheritance trees scale better

[Tom Lane]

I wrote:
> ... I also tried variants
> of that involving updating two columns of a 6-column table and of a
> 10-column table, figuring that those cases might be a bit more
> representative of typical usage (see attached scripts).

Argh, I managed to attach the wrong file for the 10-column test
case.  For the archives' sake, here's the right one.

regards, tom lane

drop table if exists tab;
create unlogged table tab (a int4, b int4, f3 int, f4 int, f5 text,
  f6 float8, f7 int, f8 int, f9 int, f10 int);
\timing on
insert into tab select g, g, g, g, g::text,
  g, g, g, g, g
  from generate_series(1, 1000) g;
vacuum tab;
explain verbose update tab set b = b, f6 = f6;
update tab set b = b, f6 = f6;

Re: Failed assertion on standby while shutdown

[igor levshin]

отбой: Маша сказала: уже оплатили :)


вт 30.03.21 20:44, Maxim Orlov пишет:

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

All the tests passed successfully.

The new status of this patch is: Ready for Committer

Re: Idea: Avoid JOINs by using path expressions to follow FKs

[Isaac Morland]

On Tue, 30 Mar 2021 at 14:30, Joel Jacobson  wrote:

If the expression ends with a column_name,
> you get the value for the column.
>
> If the expression ends with a constraint_name,
> you get the referenced table as a record.
>

Can’t you just leave off the “ends with a column_name” part? If you want
one of its columns, just put .column_name:

table -> constraint -> ... -> constraint . column_name

Then you know that -> expects a constraint_name and only that to its right.

Also, should the join be a left join, which would therefore return a NULL
when there is no matching record? Or could we have a variation such as ->?
to give a left join (NULL when no matching record) with -> using an inner
join (record is not included in result when no matching record).

For the record I would find something like this quite useful. I constantly
find myself joining in code lookup tables and the like, and while from a
mathematical view it’s just another join, explicitly listing the table in
the FROM clause of a large query does not assist with readability to say
the least.

Re: Support tab completion for upper character inputs in psql

[David Zhang]

Hi Tang,

Thanks a lot for the patch.

I did a quick test based on the latest patch V3 on latest master branch 
"commit 4753ef37e0eda4ba0af614022d18fcbc5a946cc9".


Case 1: before patch

  1 postgres=# set a
  2 all  allow_system_table_mods 
application_name array_nulls

  3 postgres=# set A
  4
  5 postgres=# create TABLE tbl (data text);
  6 CREATE TABLE
  7 postgres=# update tbl SET DATA =
  8
  9 postgres=# update T
 10
 11 postgres=#

Case 2: after patched

  1 postgres=# set a
  2 all  allow_system_table_mods 
application_name array_nulls

  3 postgres=# set A
  4 ALL  ALLOW_SYSTEM_TABLE_MODS 
APPLICATION_NAME ARRAY_NULLS

  5 postgres=# create TABLE tbl (data text);
  6 CREATE TABLE
  7
  8 postgres=# update tbl SET DATA =
  9
 10 postgres=# update TBL SET
 11
 12 postgres=#

So, as you can see the difference is between line 8 and 10 in case 2. It 
looks like the lowercase can auto complete more than the uppercase; 
secondly, if you can add some test cases, it would be great.


Best regards,
David

On 2021-03-22 5:41 a.m., tanghy.f...@fujitsu.com wrote:

On Tuesday, March 16, 2021 5:20 AM, Peter Eisentraut 
 wrote:


The cases that complete with a query
result are not case insensitive right now.  This affects things like

UPDATE T

as well.  I think your first patch was basically right.  But we need to
understand that this affects all completions with query results, not
just the one you wanted to fix.  So you should analyze all the callers
and explain why the proposed change is appropriate.

Thanks for your review and suggestion. Please find attached patch V3 which was 
based on the first patch[1].
Difference from the first patch is:

Add tab completion support for all query results in psql.
complete_from_query
+complete_from_versioned_query
+complete_from_schema_query
+complete_from_versioned_schema_query

[1] 
https://www.postgresql.org/message-id/a63cbd45e3884cf9b3961c2a6a95dcb7%40G08CNEXMBPEKD05.g08.fujitsu.local

The modification to support case insensitive matching in " _complete_from_query" is based on 
"complete_from_const and "complete_from_list" .
Please let me know if you find anything insufficient.

Regards,
Tang



--
David

Software Engineer
Highgo Software Inc. (Canada)
www.highgo.ca

Re: SELECT INTO deprecation

[Jan Wieck]

On 12/15/20 5:13 PM, Bruce Momjian wrote:

On Wed, Dec  9, 2020 at 09:48:54PM +0100, Peter Eisentraut wrote:

On 2020-12-03 20:26, Peter Eisentraut wrote:
> On 2020-12-03 16:34, Tom Lane wrote:
> > As I recall, a whole lot of the pain we have with INTO has to do
> > with the semantics we've chosen for INTO in a set-operation nest.
> > We think you can write something like
> > 
> >  SELECT ... INTO foo FROM ... UNION SELECT ... FROM ...
> > 
> > but we insist on the INTO being in the first component SELECT.

> > I'd like to know exactly how much of that messiness is shared
> > by SQL Server.
> 
> On sqlfiddle.com, this works:
> 
> select a into t3 from t1 union select a from t2;
> 
> but this gets an error:
> 
> select a from t1 union select a into t4 from t2;
> 
> SELECT INTO must be the first query in a statement containing a UNION,

> INTERSECT or EXCEPT operator.

So, with that in mind, here is an alternative proposal that points out that
SELECT INTO has some use for compatibility.


Do we really want to carry around confusing syntax for compatibility?  I
doubt we would ever add INTO now, even for compatibility.



If memory serves the INTO syntax is a result from the first incarnation 
of PL/pgSQL being based on the Oracle PL/SQL syntax. I think it has been 
there from the very beginning, which makes it likely that by now a lot 
of migrants are using it in rather old code.


I don't think it should be our business to throw wrenches into their 
existing applications.



Regards, Jan

--
Jan Wieck
Principle Database Engineer
Amazon Web Services

Bug? pg_identify_object_as_address() et al doesn't work with pg_enum.oid

[Joel Jacobson]

Hi,

Some catalog oid values originate from other catalogs,
such as pg_aggregate.aggfnoid -> pg_proc.oid
or pg_attribute.attrelid -> pg_class.oid.

For such oid values, the foreign catalog is the regclass
which should be passed as the first argument to
all the functions taking (classid oid, objid oid, objsubid integer)
as input, i.e. pg_describe_object(), pg_identify_object() and
pg_identify_object_as_address().

All oids values in all catalogs,
can be used with these functions,
as long as the correct regclass is passed as the first argument,
*except* pg_enum.oid.

(This is not a problem for pg_enum.enumtypid,
its regclass is 'pg_type' and works fine.)

I would have expected the regclass to be 'pg_enum'::regclass,
since there is no foreign key on pg_enum.oid.

In a way, pg_enum is similar to pg_attribute,
   pg_enum.enumtypid -> pg_type.oid
reminds me of
   pg_attribute.attrelid -> pg_class.oid

But pg_enum has its own oid column as primary key,
whereas in pg_attribute we only have a multi-column primary key (attrelid, 
attnum).

Is this a bug? I.e. should we add support to deal with pg_enum.oid?

Or is this by design?
If so, wouldn't it be good to mention this corner-case
somewhere in the documentation for pg_identify_object_as_address() et al?
That is, to explain these functions works for almost all oid values, except 
pg_enum.oid.

/Joel

Re: pgbench - add pseudo-random permutation function

[Dean Rasheed]

On Tue, 30 Mar 2021 at 19:26, Fabien COELHO  wrote:
>
> First, I have a thing against erand48.

Yeah, that's probably a fair point. However, all the existing pgbench
random functions are using it, so I think it's fair enough for
permute() to do the same (and actually 2^48 is pretty huge). Switching
to a 64-bit PRNG might not be a bad idea, but I think that's something
we'd want to do across the board, and so I think it should be out of
scope for this patch.

> Second, I have a significant reservation about the very structure of the
> transformation in this version:
>
>loop 4 times :
>
>  // FIRST HALF STEER
>  m/r = pseudo randoms
>  if v in first "half"
> v = ((v * m) ^ r) & mask;
> rotate1(v)
>
>  // FULL SHIFT 1
>  r = pseudo random
>  v = (v + r) % size
>
>  // SECOND HALF STEER
>  m/r = pseudo randoms
>  if v in second "half"
> same as previous on second half
>
>  // FULL SHIFT 2
>  r = pseudo random
>  v = (v + r) % size
>
> I'm really at odds with FULL SHIFT 1, because it means that up to 1/256 of
> values are kept out of STEERING. Whole chunks of values could be kept
> unshuffled because they would only have SHIFTS apply to them and each time
> fall in the not steered half. It should be an essential part of the design
> that at least one steer is applied on a value at each round, and if two
> are applied then fine, but certainly not zero. So basically I think that
> the design would be significantly improved by removing "FULL SHIFT 1".

Ah, that's a good point. Something else that also concerned me there
was that it might lead to 2 consecutive full shifts with nothing in
between, which would lead to less uniform randomness (like the
Irwin-Hall distribution).

I just did a quick test without the first full shift, and the results
do appear to be better, so removing that looks like a good idea.

> Third, I think that the rotate code can be simplified, in particular the
> ?: should be avoided because it may induce branches quite damaging to
> processor performance.

Yeah, I wondered about that. Perhaps there's a "trick" that can be
used to simplify it. Pre-computing the number of bits in the mask
would probably help. I'll give it some thought.

Regards,
Dean

Re: Idea: Avoid JOINs by using path expressions to follow FKs

[Joel Jacobson]

On Tue, Mar 30, 2021, at 21:02, Isaac Morland wrote:
> On Tue, 30 Mar 2021 at 14:30, Joel Jacobson  wrote:
> 
>> __
>> If the expression ends with a column_name,
>> you get the value for the column.
>> 
>> If the expression ends with a constraint_name,
>> you get the referenced table as a record.
> 
> Can’t you just leave off the “ends with a column_name” part? If you want one 
> of its columns, just put .column_name:
> 
> table -> constraint -> ... -> constraint . column_name
> 
> Then you know that -> expects a constraint_name and only that to its right.

+1

Of course! Much simpler. Thanks.

> 
> Also, should the join be a left join, which would therefore return a NULL 
> when there is no matching record? Or could we have a variation such as ->? to 
> give a left join (NULL when no matching record) with -> using an inner join 
> (record is not included in result when no matching record).

Interesting idea, but I think we can keep it simple, and still support the case 
you mention:

If we only have -> and you want to exclude records where the column is NULL 
(i.e. INNER JOIN),
I think we should just use the WHERE clause and filter on such condition.

> 
> For the record I would find something like this quite useful. I constantly 
> find myself joining in code lookup tables and the like, and while from a 
> mathematical view it’s just another join, explicitly listing the table in the 
> FROM clause of a large query does not assist with readability to say the 
> least.

Thanks for the encouraging words. I have exactly the same experience myself and 
share your view.

I look forward to continued discussion on this matter.

/Joel

Re: Get memory contexts of an arbitrary backend process

[Fujii Masao]

On 2021/03/30 22:06, torikoshia wrote:

Modified the patch according to the suggestions.


Thanks for updating the patch!

I applied the cosmetic changes to the patch and added the example of
the function call into the document. Attached is the updated version
of the patch. Could you check this version?

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index fbf6062d0a..aa9080bddc 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -24917,6 +24917,23 @@ SELECT collation for ('foo' COLLATE "de_DE");

   
 
+  
+   
+
+ pg_log_backend_memory_contexts
+
+pg_log_backend_memory_contexts ( 
pid integer )
+boolean
+   
+   
+Logs the memory contexts whose backend process has the specified
+process ID.
+Memory contexts will be logged based on the log configuration set.
+See  for more information.
+Only superusers can log the memory contexts.
+   
+  
+
   

 
@@ -24987,6 +25004,35 @@ SELECT collation for ('foo' COLLATE "de_DE");
 pg_stat_activity view.

 
+   
+pg_log_backend_memory_contexts can be used
+to log the memory contexts of the backend process. For example,
+
+postgres=# SELECT pg_log_backend_memory_contexts(pg_backend_pid());
+ pg_log_backend_memory_contexts 
+
+ t
+(1 row)
+
+The memory contexts will be logged in the log file. For example:
+LOG:  logging memory contexts of PID 10377
+STATEMENT:  SELECT pg_log_backend_memory_contexts(pg_backend_pid());
+LOG:  level: 0; TopMemoryContext: 80800 total in 6 blocks; 14432 free (5 
chunks); 66368 used
+LOG:  level: 1; pgstat TabStatusArray lookup hash table: 8192 total in 1 
blocks; 1408 free (0 chunks); 6784 used
+LOG:  level: 1; TopTransactionContext: 8192 total in 1 blocks; 7720 free (1 
chunks); 472 used
+LOG:  level: 1; RowDescriptionContext: 8192 total in 1 blocks; 6880 free (0 
chunks); 1312 used
+LOG:  level: 1; MessageContext: 16384 total in 2 blocks; 5152 free (0 chunks); 
11232 used
+LOG:  level: 1; Operator class cache: 8192 total in 1 blocks; 512 free (0 
chunks); 7680 used
+LOG:  level: 1; smgr relation table: 16384 total in 2 blocks; 4544 free (3 
chunks); 11840 used
+LOG:  level: 1; TransactionAbortContext: 32768 total in 1 blocks; 32504 free 
(0 chunks); 264 used
+...
+LOG:  level: 1; ErrorContext: 8192 total in 1 blocks; 7928 free (3 chunks); 
264 used
+LOG:  Grand total: 1651920 bytes in 201 blocks; 622360 free (88 chunks); 
1029560 used
+
+For more than 100 child contexts under the same parent one,
+100 child contexts and a summary of the remaining ones will be logged.
+   
+
   
 
   
diff --git a/src/backend/storage/ipc/procsignal.c 
b/src/backend/storage/ipc/procsignal.c
index c6a8d4611e..eac6895141 100644
--- a/src/backend/storage/ipc/procsignal.c
+++ b/src/backend/storage/ipc/procsignal.c
@@ -30,6 +30,7 @@
 #include "storage/shmem.h"
 #include "storage/sinval.h"
 #include "tcop/tcopprot.h"
+#include "utils/memutils.h"
 
 /*
  * The SIGUSR1 signal is multiplexed to support signaling multiple event
@@ -657,6 +658,9 @@ procsignal_sigusr1_handler(SIGNAL_ARGS)
if (CheckProcSignal(PROCSIG_BARRIER))
HandleProcSignalBarrierInterrupt();
 
+   if (CheckProcSignal(PROCSIG_LOG_MEMORY_CONTEXT))
+   HandleLogMemoryContextInterrupt();
+
if (CheckProcSignal(PROCSIG_RECOVERY_CONFLICT_DATABASE))
RecoveryConflictInterrupt(PROCSIG_RECOVERY_CONFLICT_DATABASE);
 
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 2b1b68109f..afaf3b1cce 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -3295,6 +3295,9 @@ ProcessInterrupts(void)
 
if (ParallelMessagePending)
HandleParallelMessages();
+
+   if (LogMemoryContextPending)
+   ProcessLogMemoryContextInterrupt();
 }
 
 
diff --git a/src/backend/utils/adt/mcxtfuncs.c 
b/src/backend/utils/adt/mcxtfuncs.c
index c02fa47550..fe9b7979e2 100644
--- a/src/backend/utils/adt/mcxtfuncs.c
+++ b/src/backend/utils/adt/mcxtfuncs.c
@@ -18,6 +18,8 @@
 #include "funcapi.h"
 #include "miscadmin.h"
 #include "mb/pg_wchar.h"
+#include "storage/proc.h"
+#include "storage/procarray.h"
 #include "utils/builtins.h"
 
 /* --
@@ -61,7 +63,7 @@ PutMemoryContextsStatsTupleStore(Tuplestorestate *tupstore,
 
/* Examine the context itself */
memset(&stat, 0, sizeof(stat));
-   (*context->methods->stats) (context, NULL, (void *) &level, &stat);
+   (*context->methods->stats) (context, NULL, (void *) &level, &stat, 
true);
 
memset(values, 0, sizeof(values));
memset(nulls, 0, sizeof(nulls));
@@ -155,3 +157,59 @@ pg_get_backend_memory_contexts(PG_FUNCTION_ARGS)
 
return (Datum) 0;
 }
+
+/*
+ * pg_log_

Re: "has_column_privilege()" issue with attnums and non-existent columns

[Joe Conway]

On 3/21/21 12:27 PM, Tom Lane wrote:

I think we may have to adjust the acl.c APIs, or maybe better provide new
entry points, so that we can have variants of pg_xxx_aclcheck that won't
throw a hard error upon not finding the row.  We cheesily tried to avoid
adjusting those APIs to support the semantics we need here, and we now see
that it didn't really work.


Ok, I took a shot at that; see attached.

Questions:

1. I confined the changes to just pg_class_aclcheck/mask
   and pg_attribute_aclcheck/mask -- did you intend
   that we do this same change across the board? Or
   perhaps do the rest of them once we open up pg15
   development?

2. This seems more invasive than something we would want
   to back patch -- agreed?

Joe
--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index add3d14..b5a6b3a 100644
*** a/src/backend/catalog/aclchk.c
--- b/src/backend/catalog/aclchk.c
*** AclMode
*** 3706,3711 
--- 3706,3719 
  pg_attribute_aclmask(Oid table_oid, AttrNumber attnum, Oid roleid,
  	 AclMode mask, AclMaskHow how)
  {
+ 	return pg_attribute_aclmask_ext(table_oid, attnum, roleid,
+ 	mask, how, NULL);
+ }
+ 
+ AclMode
+ pg_attribute_aclmask_ext(Oid table_oid, AttrNumber attnum, Oid roleid,
+ 		 AclMode mask, AclMaskHow how, bool *is_missing)
+ {
  	AclMode		result;
  	HeapTuple	classTuple;
  	HeapTuple	attTuple;
*** pg_attribute_aclmask(Oid table_oid, Attr
*** 3723,3740 
  			   ObjectIdGetDatum(table_oid),
  			   Int16GetDatum(attnum));
  	if (!HeapTupleIsValid(attTuple))
! 		ereport(ERROR,
! (errcode(ERRCODE_UNDEFINED_COLUMN),
!  errmsg("attribute %d of relation with OID %u does not exist",
! 		attnum, table_oid)));
  	attributeForm = (Form_pg_attribute) GETSTRUCT(attTuple);
  
! 	/* Throw error on dropped columns, too */
  	if (attributeForm->attisdropped)
! 		ereport(ERROR,
! (errcode(ERRCODE_UNDEFINED_COLUMN),
!  errmsg("attribute %d of relation with OID %u does not exist",
! 		attnum, table_oid)));
  
  	aclDatum = SysCacheGetAttr(ATTNUM, attTuple, Anum_pg_attribute_attacl,
  			   &isNull);
--- 3731,3768 
  			   ObjectIdGetDatum(table_oid),
  			   Int16GetDatum(attnum));
  	if (!HeapTupleIsValid(attTuple))
! 	{
! 		if (is_missing != NULL)
! 		{
! 			/* return "no privileges" instead of throwing an error */
! 			*is_missing = true;
! 			return 0;
! 		}
! 		else
! 			ereport(ERROR,
! 	(errcode(ERRCODE_UNDEFINED_COLUMN),
! 	 errmsg("attribute %d of relation with OID %u does not exist",
! 			attnum, table_oid)));
! 	}
! 
  	attributeForm = (Form_pg_attribute) GETSTRUCT(attTuple);
  
! 	/* Check dropped columns, too */
  	if (attributeForm->attisdropped)
! 	{
! 		if (is_missing != NULL)
! 		{
! 			/* return "no privileges" instead of throwing an error */
! 			*is_missing = true;
! 			ReleaseSysCache(attTuple);
! 			return 0;
! 		}
! 		else
! 			ereport(ERROR,
! 	(errcode(ERRCODE_UNDEFINED_COLUMN),
! 	 errmsg("attribute %d of relation with OID %u does not exist",
! 			attnum, table_oid)));
! 	}
  
  	aclDatum = SysCacheGetAttr(ATTNUM, attTuple, Anum_pg_attribute_attacl,
  			   &isNull);
*** AclMode
*** 3791,3796 
--- 3819,3831 
  pg_class_aclmask(Oid table_oid, Oid roleid,
   AclMode mask, AclMaskHow how)
  {
+ 	return pg_class_aclmask_ext(table_oid, roleid, mask, how, NULL);
+ }
+ 
+ AclMode
+ pg_class_aclmask_ext(Oid table_oid, Oid roleid, AclMode mask,
+ 	 AclMaskHow how, bool *is_missing)
+ {
  	AclMode		result;
  	HeapTuple	tuple;
  	Form_pg_class classForm;
*** pg_class_aclmask(Oid table_oid, Oid role
*** 3804,3813 
  	 */
  	tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(table_oid));
  	if (!HeapTupleIsValid(tuple))
! 		ereport(ERROR,
! (errcode(ERRCODE_UNDEFINED_TABLE),
!  errmsg("relation with OID %u does not exist",
! 		table_oid)));
  	classForm = (Form_pg_class) GETSTRUCT(tuple);
  
  	/*
--- 3839,3858 
  	 */
  	tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(table_oid));
  	if (!HeapTupleIsValid(tuple))
! 	{
! 		if (is_missing != NULL)
! 		{
! 			/* return "no privileges" instead of throwing an error */
! 			*is_missing = true;
! 			return 0;
! 		}
! 		else
! 			ereport(ERROR,
! 	(errcode(ERRCODE_UNDEFINED_TABLE),
! 	 errmsg("relation with OID %u does not exist",
! 			table_oid)));
! 	}
! 
  	classForm = (Form_pg_class) GETSTRUCT(tuple);
  
  	/*
*** AclResult
*** 4468,4474 
  pg_attribute_aclcheck(Oid table_oid, AttrNumber attnum,
  	  Oid roleid, AclMode mode)
  {
! 	if (pg_attribute_aclmask(table_oid, attnum, roleid, mode, ACLMASK_ANY) != 0)
  		return ACLCHECK_OK;
  	else
  		return ACLCHECK_NO_PRIV;
--- 4513,4527 
  pg_attribute_aclcheck(Oid table_oid, AttrNumber attnum,
  	  Oid roleid, AclMode mode)
  

Re: pgbench - add pseudo-random permutation function

[Dean Rasheed]

On Tue, 30 Mar 2021 at 20:31, Dean Rasheed  wrote:
>
> Yeah, that's probably a fair point. However, all the existing pgbench
> random functions are using it, so I think it's fair enough for
> permute() to do the same (and actually 2^48 is pretty huge). Switching
> to a 64-bit PRNG might not be a bad idea, but I think that's something
> we'd want to do across the board, and so I think it should be out of
> scope for this patch.
>

Of course the immediate counter-argument to changing the existing
random functions would be that doing so would break lots of people's
tests, and no one would thank us for that. Still, I think that, since
the existing random functions use a 48-bit PRNG, it's not unreasonable
for permute() to do the same.

Regards,
Dean

Re: pg_amcheck contrib application

[Robert Haas]

On Mon, Mar 29, 2021 at 7:16 PM Mark Dilger
 wrote:
> Sure, here are four patches which do the same as the single v12 patch did.

Thanks. Here are some comments on 0003 and 0004:

When you posted v11, you said that "Rather than print out all four
toast pointer fields for each toast failure, va_rawsize, va_extsize,
and va_toastrelid are only mentioned in the corruption message if they
are related to the specific corruption.  Otherwise, just the
va_valueid is mentioned in the corruption message." I like that
principal; in fact, as you know, I suggested it. But, with the v13
patches applied, exactly zero of the callers to
report_toast_corruption() appear to be following it, because none of
them include the value ID. I think you need to revise the messages,
e.g. "toasted value for attribute %u missing from toast table" ->
"toast value %u not found in toast table"; "final toast chunk number
%u differs from expected value %u" -> "toast value %u was expected to
end at chunk %u, but ended at chunk %u"; "toast chunk sequence number
is null" -> "toast value %u has toast chunk with null sequence
number". In the first of those example cases, I think you need not
mention the attribute number because it's already there in its own
column.

On a related note, it doesn't look like you are actually checking
va_toastrelid here. Doing so seems like it would be a good idea. It
also seems like it would be good to check that the compressed size is
less than or equal to the uncompressed size.

I do not like the name tuple_is_volatile, because volatile has a
couple of meanings already, and this isn't one of them. A
SQL-callable function is volatile if it might return different outputs
given the same inputs, even within the same SQL statement. A C
variable is volatile if it might be magically modified in ways not
known to the compiler. I had suggested tuple_cannot_die_now, which is
closer to the mark. If you want to be even more precise, you could
talk about whether the tuple is potentially prunable (e.g.
tuple_can_be_pruned, which inverts the sense). That's really what
we're worried about: whether MVCC rules would permit the tuple to be
pruned after we release the buffer lock and before we check TOAST.

I would ideally prefer not to rename report_corruption(). The old name
is clearer, and changing it produces a bunch of churn that I'd rather
avoid. Perhaps the common helper function could be called
report_corruption_internal(), and the callers could be
report_corruption() and report_toast_corruption().

Regarding 0001 and 0002, I think the logic in 0002 looks a lot closer
to correct now, but I want to go through it in more detail. I think,
though, that you've made some of my comments worse. For example, I
wrote: "It should be impossible for xvac to still be running, since
we've removed all that code, but even if it were, it ought to be safe
to read the tuple, since the original inserter must have committed.
But, if the xvac transaction committed, this tuple (and its associated
TOAST tuples) could be pruned at any time." You changed that to read
"We don't bother comparing against safe_xmin because the VACUUM FULL
must have committed prior to an upgrade and can't still be running."
Your comment is shorter, which is a point in its favor, but what I was
trying to emphasize is that the logic would be correct EVEN IF we
again started to use HEAP_MOVED_OFF and HEAP_MOVED_IN again. Your
version makes it sound like the code would need to be revised in that
case. If that's true, then my comment was wrong, but I didn't think it
was true, or I wouldn't have written the comment in that way.

Also, and maybe this is a job for a separate patch, but then again
maybe not, I wonder if it's really a good idea for get_xid_status to
return both a XidBoundsViolation and an XidCommitStatus. It seems to
me (without checking all that carefully) that it might be better to
just flatten all of that into a single enum, because right now it
seems like you often end up with two consecutive switch statements
where, perhaps, just one would suffice.

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

Re: Idea: Avoid JOINs by using path expressions to follow FKs

[Isaac Morland]

On Tue, 30 Mar 2021 at 15:33, Joel Jacobson  wrote:

> Also, should the join be a left join, which would therefore return a NULL
> when there is no matching record? Or could we have a variation such as ->?
> to give a left join (NULL when no matching record) with -> using an inner
> join (record is not included in result when no matching record).
>
>
> Interesting idea, but I think we can keep it simple, and still support the
> case you mention:
>
> If we only have -> and you want to exclude records where the column is
> NULL (i.e. INNER JOIN),
> I think we should just use the WHERE clause and filter on such condition.
>

Just to be clear, it will always be a left join? Agreed that getting the
inner join behaviour can be done in the WHERE clause. I think this is a
case where simple is good. As long as the left join case is supported I'm
happy.


> Thanks for the encouraging words. I have exactly the same experience
> myself and share your view.
>
> I look forward to continued discussion on this matter.
>

I had another idea: maybe the default name of a foreign key constraint to a
primary key should simply be the name of the target table? That is, if I
say:

FOREIGN KEY (...) REFERENCES t

... then unless the table name t is already in use as a constraint name, it
will be used as the constraint name. It would be nice not to have to keep
repeating, like this:

CONSTRAINT t FOREIGN KEY (...) REFERENCES t

Re: "has_column_privilege()" issue with attnums and non-existent columns

[Tom Lane]

Joe Conway  writes:
> On 3/21/21 12:27 PM, Tom Lane wrote:
>> I think we may have to adjust the acl.c APIs, or maybe better provide new
>> entry points, so that we can have variants of pg_xxx_aclcheck that won't
>> throw a hard error upon not finding the row.  We cheesily tried to avoid
>> adjusting those APIs to support the semantics we need here, and we now see
>> that it didn't really work.

> Ok, I took a shot at that; see attached.

Looks generally reasonable by eyeball.  The lack of any documentation
comment for the new functions makes me itch, although the ones that
are there are hardly better.

> 1. I confined the changes to just pg_class_aclcheck/mask
> and pg_attribute_aclcheck/mask -- did you intend
> that we do this same change across the board? Or
> perhaps do the rest of them once we open up pg15
> development?

In principle, it might be nice to fix all of those functions in acl.c
to be implemented similarly --- you could get rid of the initial
SearchSysCacheExists calls in the ones that are trying not to throw
error for is-missing cases.  In practice, as long as there's no
reachable bug for the other cases, there are probably better things
to spend time on.

> 2. This seems more invasive than something we would want
> to back patch -- agreed?

You could make an argument either way, but given the limited number
of complaints about this, I'd lean to no back-patch.

regards, tom lane

Re: "has_column_privilege()" issue with attnums and non-existent columns

[Joe Conway]

On 3/30/21 3:37 PM, Joe Conway wrote:

On 3/21/21 12:27 PM, Tom Lane wrote:

I think we may have to adjust the acl.c APIs, or maybe better provide new
entry points, so that we can have variants of pg_xxx_aclcheck that won't
throw a hard error upon not finding the row.  We cheesily tried to avoid
adjusting those APIs to support the semantics we need here, and we now see
that it didn't really work.


Ok, I took a shot at that; see attached.


Heh, I missed the forest for the trees it seems.

That version undid the changes fixing what Ian was originally complaining about.

New version attached that preserves the fixed behavior.


Questions:
1. I confined the changes to just pg_class_aclcheck/mask
 and pg_attribute_aclcheck/mask -- did you intend
 that we do this same change across the board? Or
 perhaps do the rest of them once we open up pg15
 development?

2. This seems more invasive than something we would want
 to back patch -- agreed?


The questions remain...

Joe
--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index add3d14..b5a6b3a 100644
*** a/src/backend/catalog/aclchk.c
--- b/src/backend/catalog/aclchk.c
*** AclMode
*** 3706,3711 
--- 3706,3719 
  pg_attribute_aclmask(Oid table_oid, AttrNumber attnum, Oid roleid,
  	 AclMode mask, AclMaskHow how)
  {
+ 	return pg_attribute_aclmask_ext(table_oid, attnum, roleid,
+ 	mask, how, NULL);
+ }
+ 
+ AclMode
+ pg_attribute_aclmask_ext(Oid table_oid, AttrNumber attnum, Oid roleid,
+ 		 AclMode mask, AclMaskHow how, bool *is_missing)
+ {
  	AclMode		result;
  	HeapTuple	classTuple;
  	HeapTuple	attTuple;
*** pg_attribute_aclmask(Oid table_oid, Attr
*** 3723,3740 
  			   ObjectIdGetDatum(table_oid),
  			   Int16GetDatum(attnum));
  	if (!HeapTupleIsValid(attTuple))
! 		ereport(ERROR,
! (errcode(ERRCODE_UNDEFINED_COLUMN),
!  errmsg("attribute %d of relation with OID %u does not exist",
! 		attnum, table_oid)));
  	attributeForm = (Form_pg_attribute) GETSTRUCT(attTuple);
  
! 	/* Throw error on dropped columns, too */
  	if (attributeForm->attisdropped)
! 		ereport(ERROR,
! (errcode(ERRCODE_UNDEFINED_COLUMN),
!  errmsg("attribute %d of relation with OID %u does not exist",
! 		attnum, table_oid)));
  
  	aclDatum = SysCacheGetAttr(ATTNUM, attTuple, Anum_pg_attribute_attacl,
  			   &isNull);
--- 3731,3768 
  			   ObjectIdGetDatum(table_oid),
  			   Int16GetDatum(attnum));
  	if (!HeapTupleIsValid(attTuple))
! 	{
! 		if (is_missing != NULL)
! 		{
! 			/* return "no privileges" instead of throwing an error */
! 			*is_missing = true;
! 			return 0;
! 		}
! 		else
! 			ereport(ERROR,
! 	(errcode(ERRCODE_UNDEFINED_COLUMN),
! 	 errmsg("attribute %d of relation with OID %u does not exist",
! 			attnum, table_oid)));
! 	}
! 
  	attributeForm = (Form_pg_attribute) GETSTRUCT(attTuple);
  
! 	/* Check dropped columns, too */
  	if (attributeForm->attisdropped)
! 	{
! 		if (is_missing != NULL)
! 		{
! 			/* return "no privileges" instead of throwing an error */
! 			*is_missing = true;
! 			ReleaseSysCache(attTuple);
! 			return 0;
! 		}
! 		else
! 			ereport(ERROR,
! 	(errcode(ERRCODE_UNDEFINED_COLUMN),
! 	 errmsg("attribute %d of relation with OID %u does not exist",
! 			attnum, table_oid)));
! 	}
  
  	aclDatum = SysCacheGetAttr(ATTNUM, attTuple, Anum_pg_attribute_attacl,
  			   &isNull);
*** AclMode
*** 3791,3796 
--- 3819,3831 
  pg_class_aclmask(Oid table_oid, Oid roleid,
   AclMode mask, AclMaskHow how)
  {
+ 	return pg_class_aclmask_ext(table_oid, roleid, mask, how, NULL);
+ }
+ 
+ AclMode
+ pg_class_aclmask_ext(Oid table_oid, Oid roleid, AclMode mask,
+ 	 AclMaskHow how, bool *is_missing)
+ {
  	AclMode		result;
  	HeapTuple	tuple;
  	Form_pg_class classForm;
*** pg_class_aclmask(Oid table_oid, Oid role
*** 3804,3813 
  	 */
  	tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(table_oid));
  	if (!HeapTupleIsValid(tuple))
! 		ereport(ERROR,
! (errcode(ERRCODE_UNDEFINED_TABLE),
!  errmsg("relation with OID %u does not exist",
! 		table_oid)));
  	classForm = (Form_pg_class) GETSTRUCT(tuple);
  
  	/*
--- 3839,3858 
  	 */
  	tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(table_oid));
  	if (!HeapTupleIsValid(tuple))
! 	{
! 		if (is_missing != NULL)
! 		{
! 			/* return "no privileges" instead of throwing an error */
! 			*is_missing = true;
! 			return 0;
! 		}
! 		else
! 			ereport(ERROR,
! 	(errcode(ERRCODE_UNDEFINED_TABLE),
! 	 errmsg("relation with OID %u does not exist",
! 			table_oid)));
! 	}
! 
  	classForm = (Form_pg_class) GETSTRUCT(tuple);
  
  	/*
*** AclResult
*** 4468,4474 
  pg_attribute_aclcheck(Oid table_oid, AttrNumber attnum,
  	  Oid roleid, AclMode 

Re: SQL/JSON: JSON_TABLE

[Nikita Glukhov]

On 30.03.2021 19:56, Erik Rijkers wrote:


On 2021.03.27. 02:12 Nikita Glukhov  wrote:

Attached 47th version of the patches.

Hi,

Apply, build all fine.  It also works quite well, and according to 
specification, as far as I can tell.

But today I ran into:

ERROR:  function ExecEvalJson not in llvmjit_types.c

I think that it is caused by:

set enable_bitmapscan = off;

(I installed llvm a few days ago. llvm-3.9-dev on this debian stretch).


This is the test sql I concocted, which runs fine with enable_bitmapscan on 
(the default):

select jt1.*
from myjsonfile100k as t(js, id)
   , json_table(
   t.js
, '$' columns (
 "lastname"   textpath  '$. "lastname" '
   , "firstname"  textpath  '$. "firstname"'
   , "date"   textpath  '$. "date" '
   , "city"   textpath  '$. "city" '
   , "country"textpath  '$. "country"  '
   , "name 0(1)"  textpath  '$. "array"[0] '
   , "name 4(5)"  textpath  '$. "array"[4] '
   , "names"  text[]  path  '$. "array"'
   , "randfloat"  float   path  '$. "random float" '
 )
) as jt1
where  js @> ('[ { "city": "Santiago de Cuba" } ]')
and js[0]->>'firstname' = 'Gilda'
;
ERROR:  function ExecEvalJson not in llvmjit_types.c

That statement only errors out if the table is large enough. I have no time now 
to make a sample table but if no-one understands the problem off-hand, I'll try 
to construct such a table later this week (the one I'm using is large, 1.5 GB).


Thank you for testing.


I think you can try to add 3 missing functions references to the end of
src/backend/jit/llvm/llvmjit_types.c:

 void       *referenced_functions[] =
 {
 ...
 ExecEvalXmlExpr,
+    ExecEvalJsonConstructor,
+    ExecEvalIsJsonPredicate,
+    ExecEvalJson,
 MakeExpandedObjectReadOnlyInternal,
 ...
 };


If this fixes problem, I will add this to the new version of the patches.


--
Nikita Glukhov
Postgres Professional:http://www.postgrespro.co   
The Russian Postgres Company

Re: Remove page-read callback from XLogReaderState.

[Thomas Munro]

On Thu, Mar 4, 2021 at 3:29 PM Kyotaro Horiguchi
 wrote:
> A recent commot about LSN_FORMAT_ARGS conflicted this.
> Just rebased.

FYI I've been looking at this, and I think it's a very nice
improvement.  I'll post some review comments and a rebase shortly.

unconstrained memory growth in long running procedure stored procedure after upgrading 11-12

[Merlin Moncure]

Hello all,

We just upgraded from postgres 11 to 12.6 and our server is running
out of memory and rebooting about 1-2 times a day.Application
architecture is a single threaded stored procedure, executed with CALL
that loops and never terminates. With postgres 11 we had no memory
issues.  Ultimately the crash looks like this:

terminate called after throwing an instance of 'std::bad_alloc'
  what():  std::bad_alloc
2021-03-29 04:34:31.262 CDT [1413] LOG:  server process (PID 9792) was
terminated by signal 6: Aborted
2021-03-29 04:34:31.262 CDT [1413] DETAIL:  Failed process was
running: CALL Main()
2021-03-29 04:34:31.262 CDT [1413] LOG:  terminating any other active
server processes
2021-03-29 04:34:31.264 CDT [9741] WARNING:  terminating connection
because of crash of another server process
2021-03-29 04:34:31.264 CDT [9741] DETAIL:  The postmaster has
commanded this server process to roll back the current transaction and
exit, because another server process exited abnormally and possibly
corrupted shared memory.
2021-03-29 04:34:31.264 CDT [9741] HINT:  In a moment you should be
able to reconnect to the database and repeat your command.
2021-03-29 04:34:31.267 CDT [1413] LOG:  archiver process (PID 9742)
exited with exit code 1
2021-03-29 04:34:31.267 CDT [1413] LOG:  all server processes
terminated; reinitializing

Attached is a self contained test case which reproduces the problem.

Instructions:
1. run the attached script in psql, pgtask_test.sql. It will create a
database, initialize it, and run the main procedure. dblink must be
available
2. in another window, run SELECT CreateTaskChain('test', 'DEV');

In the console that ran main(), you should see output that the
procedure began to do work. Once it does, a 'top' should show resident
memory growth immediately.   It's about a gigabyte an hour in my test.
Sorry for the large-ish attachment.

merlin


pgtask_test.sql
Description: Binary data


pgtask.sql
Description: Binary data

Re: multi-install PostgresNode fails with older postgres versions

[Mark Dilger]

> On Mar 30, 2021, at 10:39 AM, Mark Dilger  
> wrote:
> 
> Andrew,
> 
> While developing some cross version tests, I noticed that PostgresNode::init 
> fails for postgres versions older than 9.3, like so:
> 
> # Checking port 52814
> # Found port 52814
> Name: 9.2.24
> Data directory: 
> /Users/mark.dilger/hydra/postgresnode.review/src/test/modules/test_cross_version/tmp_check/t_001_verify_paths_9.2.24_data/pgdata
> Backup directory: 
> /Users/mark.dilger/hydra/postgresnode.review/src/test/modules/test_cross_version/tmp_check/t_001_verify_paths_9.2.24_data/backup
> Archive directory: 
> /Users/mark.dilger/hydra/postgresnode.review/src/test/modules/test_cross_version/tmp_check/t_001_verify_paths_9.2.24_data/archives
> Connection string: port=52814 
> host=/var/folders/6n/3f4vwbnn7fz5qk0xqhgbjrkwgp/T/L_A2w1x7qb
> Log file: 
> /Users/mark.dilger/hydra/postgresnode.review/src/test/modules/test_cross_version/tmp_check/log/001_verify_paths_9.2.24.log
> # Running: initdb -D 
> /Users/mark.dilger/hydra/postgresnode.review/src/test/modules/test_cross_version/tmp_check/t_001_verify_paths_9.2.24_data/pgdata
>  -A trust -N
> initdb: invalid option -- N
> Try "initdb --help" for more information.
> Bail out!  system initdb failed
> 
> The problem is clear enough; -N/--nosync was added in 9.3, and 
> PostgresNode::init is passing -N to initdb unconditionally. I wonder if 
> during PostgresNode::new a call should be made to pg_config and the version 
> information grep'd out so that version specific options to various functions 
> (init, backup, etc) could hinge on the version of postgres being used?
> 
> You could also just remove the -N option, but that would slow down all tests 
> for everybody, so I'm not keen to do that.  Or you could remove -N in cases 
> where $node->{_install_path} is defined, which would be far more acceptable.  
> I'm leaning towards using the output of pg_config, though, since this problem 
> is likely to come up again with other options/commands.
> 
> Thoughts?

I fixed this largely as outlined above, introducing a few new functions which 
ease test development and using one of them to condition the behavior of init() 
on the postgres version.

In the tests I have been developing (not included), the developer (or some 
buildfarm script) has to list all postgresql installations in a configuration 
file, like so:

/Users/mark.dilger/install/8.4
/Users/mark.dilger/install/9.0.23
/Users/mark.dilger/install/9.1.24
/Users/mark.dilger/install/9.2.24
/Users/mark.dilger/install/9.3.25
/Users/mark.dilger/install/9.4.26
/Users/mark.dilger/install/9.5.25
/Users/mark.dilger/install/9.6
/Users/mark.dilger/install/10
/Users/mark.dilger/install/11
/Users/mark.dilger/install/12
/Users/mark.dilger/install/13

The tests can't be hardcoded to know anything about which specific postgres 
versions will be installed, or what version of postgres exists in any 
particular install directory.  It makes the tests easier to maintain if they 
can do stuff like:

   $node{$_} = PostgresNode->get_new_node(...) for (@installed_versions);
   
   if ($node{a}->newer_than_version($node{b}))
   {
  # check that newer version A's whatever can connect to and work with 
older server B
 
   }

I therefore included functions of that sort in the patch along with the 
$node->at_least_version(version) function that the fix uses.



v1-0001-Extending-PostgresNode-cross-version-functionalit.patch.WIP
Description: Binary data


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

Trouble with initdb trying to run regression tests

[Isaac Morland]

I've built Postgres inside a Ubuntu Vagrant VM. When I try to "make check",
I get a complaint about the permissions on the data directory:

[]
pg_regress: initdb failed
Examine /vagrant/src/test/regress/log/initdb.log for the reason.
Command was: "initdb" -D "/vagrant/src/test/regress/./tmp_check/data"
--no-clean --no-sync > "/vagrant/src/test/regress/log/initdb.log" 2>&1
make[1]: *** [GNUmakefile:125: check] Error 2
make[1]: Leaving directory '/vagrant/src/test/regress'
make: *** [GNUmakefile:69: check] Error 2
vagrant@ubuntu-focal:/vagrant$ tail /vagrant/src/test/regress/log/initdb.log
creating subdirectories ... ok
selecting dynamic shared memory implementation ... posix
selecting default max_connections ... 20
selecting default shared_buffers ... 400kB
selecting default time zone ... Etc/UTC
creating configuration files ... ok
running bootstrap script ... 2021-03-30 21:38:32.746 UTC [23154] FATAL:
 data directory "/vagrant/src/test/regress/./tmp_check/data" has invalid
permissions
2021-03-30 21:38:32.746 UTC [23154] DETAIL:  Permissions should be u=rwx
(0700) or u=rwx,g=rx (0750).
child process exited with exit code 1
initdb: data directory "/vagrant/src/test/regress/./tmp_check/data" not
removed at user's request
vagrant@ubuntu-focal:/vagrant$

Has anybody had this problem? The directory in question is created by the
make check activities so I would have thought that it would set the
permissions; and if not, then everybody trying to run regression tests
would bump into this.

Re: SQL/JSON: JSON_TABLE

[Erik Rijkers]

> On 2021.03.30. 22:25 Nikita Glukhov  wrote:
> 
>  
> On 30.03.2021 19:56, Erik Rijkers wrote:
> 
> >> On 2021.03.27. 02:12 Nikita Glukhov  wrote:
> >>
> >> Attached 47th version of the patches.
> > Hi,
> >
> > Apply, build all fine.  It also works quite well, and according to 
> > specification, as far as I can tell.
> >
> > But today I ran into:
> >
> > ERROR:  function ExecEvalJson not in llvmjit_types.c
> >
> > I think that it is caused by:
> >
> > set enable_bitmapscan = off;
> >
> > (I installed llvm a few days ago. llvm-3.9-dev on this debian stretch).
> >
> >
> > This is the test sql I concocted, which runs fine with enable_bitmapscan on 
> > (the default):
> >
> > select jt1.*
> > from myjsonfile100k as t(js, id)
> >, json_table(
> >t.js
> > , '$' columns (
> >  "lastname"   textpath  '$. "lastname" '
> >, "firstname"  textpath  '$. "firstname"'
> >, "date"   textpath  '$. "date" '
> >, "city"   textpath  '$. "city" '
> >, "country"textpath  '$. "country"  '
> >, "name 0(1)"  textpath  '$. "array"[0] '
> >, "name 4(5)"  textpath  '$. "array"[4] '
> >, "names"  text[]  path  '$. "array"'
> >, "randfloat"  float   path  '$. "random float" '
> >  )
> > ) as jt1
> > where  js @> ('[ { "city": "Santiago de Cuba" } ]')
> > and js[0]->>'firstname' = 'Gilda'
> > ;
> > ERROR:  function ExecEvalJson not in llvmjit_types.c
> >
> > That statement only errors out if the table is large enough. I have no time 
> > now to make a sample table but if no-one understands the problem off-hand, 
> > I'll try to construct such a table later this week (the one I'm using is 
> > large, 1.5 GB).
> 
> Thank you for testing.
> 
> 
> I think you can try to add 3 missing functions references to the end of
> src/backend/jit/llvm/llvmjit_types.c:
> 
>   void       *referenced_functions[] =
>   {
>   ...
>   ExecEvalXmlExpr,
> +    ExecEvalJsonConstructor,
> +    ExecEvalIsJsonPredicate,
> +    ExecEvalJson,
>   MakeExpandedObjectReadOnlyInternal,
>   ...
>   };
> 
> 
> If this fixes problem, I will add this to the new version of the patches.

It does almost fix it, but in the above is a typo:
+  ExecEvalIsJsonPredicate should to be changed to:
+  ExecEvalJsonIsPredicate.

With that change the problem vanishes.

Thanks!

Erik Rijkers








> 
> -- 
> Nikita Glukhov
> Postgres Professional:http://www.postgrespro.co   
> The Russian Postgres Company

Re: Refactor SSL test framework to support multiple TLS libraries

[Daniel Gustafsson]

> On 30 Mar 2021, at 11:53, Michael Paquier  wrote:
> 
> On Tue, Mar 30, 2021 at 03:50:28PM +0900, Michael Paquier wrote:
>> The test_*() ones are just wrappers for psql able to use a customized
>> connection string.  It seems to me that it would make sense to move
>> those two into PostgresNode::psql itself and extend it to be able to
>> handle custom connection strings?
> 
> Looking at this part, I think that this is a win in terms of future
> changes for SSLServer.pm as it would become a facility only in charge
> of managing the backend's SSL configuration.  This has also the
> advantage to make the error handling with psql more consistent with
> the other tests.
> 
> So, attached is a patch to do this simplification.  The bulk of the
> changes is within the tests themselves to adapt to the merge of
> $common_connstr and $connstr for the new routines of PostgresNode.pm,
> and I have done things this way to ease the patch lookup.  Thoughts?

LGTM with the findings that Alvaro reported.

+$node->connect_ok($common_connstr . " " . "user=ssltestuser",

This double concatenation could be a single concat, or just use scalar value
interpolation in the string to make it even more readable.  As it isn't using
the same line broken pattern of the others the concat looks a bit weird as a
result.

Thanks for picking it up, as I have very limited time for hacking right now.

--
Daniel Gustafsson   https://vmware.com/

Re: multi-install PostgresNode fails with older postgres versions

[Alvaro Herrera]

On 2021-Mar-30, Mark Dilger wrote:

> The problem is clear enough; -N/--nosync was added in 9.3, and
> PostgresNode::init is passing -N to initdb unconditionally. I wonder
> if during PostgresNode::new a call should be made to pg_config and the
> version information grep'd out so that version specific options to
> various functions (init, backup, etc) could hinge on the version of
> postgres being used?

Yeah, I think making it backwards-compatible would be good.  Is running
pg_config to obtain the version the best way to do it?  I'm not sure --
what other ways are there?  I can't of anything.  (Asking the user seems
right out.)

-- 
Álvaro Herrera39°49'30"S 73°17'W
Officer Krupke, what are we to do?
Gee, officer Krupke, Krup you! (West Side Story, "Gee, Officer Krupke")

Re: making update/delete of inheritance trees scale better

[Tom Lane]

I wrote:
> However, I then tried a partitioned equivalent of the 6-column case
> (script also attached), and it looks like
> 6 columns 16551   19097   15637   18201
> which is really noticeably worse, 16% or so.

... and on the third hand, that might just be some weird compiler-
and platform-specific artifact.

Using the exact same compiler (RHEL8's gcc 8.3.1) on a different
x86_64 machine, I measure the same case as about 7% slowdown not
16%.  That's still not great, but it calls the original measurement
into question, for sure.

Using Apple's clang 12.0.0 on an M1 mini, the patch actually clocks
in a couple percent *faster* than HEAD, for both the partitioned and
unpartitioned 6-column test cases.

So I'm not sure what to make of these results, but my level of concern
is less than it was earlier today.  I might've just gotten trapped by
the usual bugaboo of micro-benchmarking, ie putting too much stock in
only one test case.

regards, tom lane

Re: Refactor SSL test framework to support multiple TLS libraries

[Alvaro Herrera]

On 2021-Mar-30, Daniel Gustafsson wrote:

> +$node->connect_ok($common_connstr . " " . "user=ssltestuser",
> 
> This double concatenation could be a single concat, or just use scalar value
> interpolation in the string to make it even more readable.  As it isn't using
> the same line broken pattern of the others the concat looks a bit weird as a
> result.

+1 for using a single scalar.

-- 
Álvaro Herrera   Valdivia, Chile

Re: multi-install PostgresNode fails with older postgres versions

[Mark Dilger]

> On Mar 30, 2021, at 3:12 PM, Alvaro Herrera  wrote:
> 
> On 2021-Mar-30, Mark Dilger wrote:
> 
>> The problem is clear enough; -N/--nosync was added in 9.3, and
>> PostgresNode::init is passing -N to initdb unconditionally. I wonder
>> if during PostgresNode::new a call should be made to pg_config and the
>> version information grep'd out so that version specific options to
>> various functions (init, backup, etc) could hinge on the version of
>> postgres being used?
> 
> Yeah, I think making it backwards-compatible would be good.  Is running
> pg_config to obtain the version the best way to do it?  I'm not sure --
> what other ways are there?  I can't of anything.  (Asking the user seems
> right out.)

Once you have a node running, you can query the version using safe_psql, but 
that clearly doesn't work soon enough, since we need the information prior to 
running initdb.

One of the things I noticed while playing with this new toy (thanks, Andrew!) 
is that if you pass a completely insane install_path, you don't get any errors. 
 In fact, you get executables and libraries from whatever 
PATH="/no/such/postgres:$PATH" gets you, probably the executables and libraries 
of your latest development branch.  By forcing get_new_node to call the 
pg_config of the path you pass in, you'd fix that problem.  I didn't do that, 
mind you, but you could.  I just executed pg_config, which means you'll still 
get the wrong version owing to the PATH confusion.


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

Re: multi-install PostgresNode fails with older postgres versions

[Alvaro Herrera]

On 2021-Mar-30, Mark Dilger wrote:

> Once you have a node running, you can query the version using
> safe_psql, but that clearly doesn't work soon enough, since we need
> the information prior to running initdb.

I was thinking something like examining some file in the install dir --
say, include/server/pg_config.h, but that seems messier than just
running pg_config.

> One of the things I noticed while playing with this new toy (thanks,
> Andrew!) is that if you pass a completely insane install_path, you
> don't get any errors.  In fact, you get executables and libraries from
> whatever PATH="/no/such/postgres:$PATH" gets you, probably the
> executables and libraries of your latest development branch.  By
> forcing get_new_node to call the pg_config of the path you pass in,
> you'd fix that problem.  I didn't do that, mind you, but you could.  I
> just executed pg_config, which means you'll still get the wrong
> version owing to the PATH confusion.

Hmm, I think it should complain if you give it a path that doesn't
actually contain a valid installation.

-- 
Álvaro Herrera   Valdivia, Chile
"But static content is just dynamic content that isn't moving!"
http://smylers.hates-software.com/2007/08/15/fe244d0c.html

Re: "has_column_privilege()" issue with attnums and non-existent columns

[Tom Lane]

Joe Conway  writes:
> Heh, I missed the forest for the trees it seems.
> That version undid the changes fixing what Ian was originally complaining 
> about.

Duh, right.  It would be a good idea for there to be a code comment
explaining this, because it's *far* from obvious.  Say like

* Check for column-level privileges first.  This serves in
* part as a check on whether the column even exists, so we
* need to do it before checking table-level privilege.

My gripe about providing API-spec comments for the new aclchk.c
entry points still stands.  Other than that, I think it's good
to go.

regards, tom lane

Re: Trouble with initdb trying to run regression tests

[Tom Lane]

Isaac Morland  writes:
> I've built Postgres inside a Ubuntu Vagrant VM. When I try to "make check",
> I get a complaint about the permissions on the data directory:

> vagrant@ubuntu-focal:/vagrant$ tail /vagrant/src/test/regress/log/initdb.log
> creating subdirectories ... ok
> selecting dynamic shared memory implementation ... posix
> selecting default max_connections ... 20
> selecting default shared_buffers ... 400kB
> selecting default time zone ... Etc/UTC
> creating configuration files ... ok
> running bootstrap script ... 2021-03-30 21:38:32.746 UTC [23154] FATAL:
>  data directory "/vagrant/src/test/regress/./tmp_check/data" has invalid
> permissions
> 2021-03-30 21:38:32.746 UTC [23154] DETAIL:  Permissions should be u=rwx
> (0700) or u=rwx,g=rx (0750).
> child process exited with exit code 1

Further up in initdb.log, there was probably some useful information
about whether it found an existing directory there or not.

regards, tom lane

Re: Idea: Avoid JOINs by using path expressions to follow FKs

[Joel Jacobson]

On Tue, Mar 30, 2021, at 22:01, Isaac Morland wrote:
> On Tue, 30 Mar 2021 at 15:33, Joel Jacobson  wrote:
>>> Also, should the join be a left join, which would therefore return a NULL 
>>> when there is no matching record? Or could we have a variation such as ->? 
>>> to give a left join (NULL when no matching record) with -> using an inner 
>>> join (record is not included in result when no matching record).
>> 
>> Interesting idea, but I think we can keep it simple, and still support the 
>> case you mention:
>> 
>> If we only have -> and you want to exclude records where the column is NULL 
>> (i.e. INNER JOIN),
>> I think we should just use the WHERE clause and filter on such condition.
>> 
> 
> Just to be clear, it will always be a left join? Agreed that getting the 
> inner join behaviour can be done in the WHERE clause. I think this is a case 
> where simple is good. As long as the left join case is supported I'm happy.

Hmm, I guess, since technically, if all foreign key column(s) are declared as 
NOT NULL, we would know for sure such values exist, so a LEFT JOIN and INNER 
JOIN would always produce the same result.
I'm not sure if the query planner could produce different plans though, and if 
an INNER JOIN could be more efficient. If it matters, then I think we should 
generate an INNER JOIN for the "all column(s) NOT NULL" case.

>  
>> Thanks for the encouraging words. I have exactly the same experience myself 
>> and share your view.
>> 
>> I look forward to continued discussion on this matter.
> 
> I had another idea: maybe the default name of a foreign key constraint to a 
> primary key should simply be the name of the target table? That is, if I say:
> 
> FOREIGN KEY (...) REFERENCES t
> 
> ... then unless the table name t is already in use as a constraint name, it 
> will be used as the constraint name. It would be nice not to have to keep 
> repeating, like this:
> 
> CONSTRAINT t FOREIGN KEY (...) REFERENCES t
> 

I suggested earlier in the thread to allow making the default name format 
user-definable,
since some users according to the comment in pg_constraint.c might depend on 
apps that rely on the name
being unique within the namespace and not just the table.

Here is the commit that implemented this:

commit 45616f587745e0e82b00e77562d6502aa042
Author: Tom Lane 
Date:   Thu Jun 10 17:56:03 2004 +

Clean up generation of default names for constraints, indexes, and serial
sequences, as per recent discussion.  All these names are now of the
form table_column_type, with digits added if needed to make them unique.
Default constraint names are chosen to be unique across their whole schema,
not just within the parent object, so as to be more SQL-spec-compatible
and make the information schema views more useful.

So if nothing has changed since then, I don't think we should change the 
default name for all users.
But like I said earlier, I think it would be good if users who know what they 
are doing could override the default name format.

/Joel

Re: Proposal: Save user's original authenticated identity for logging

[Jacob Champion]

On Tue, 2021-03-30 at 17:06 +, Jacob Champion wrote:
> Would it be acceptable to adjust the tests for live rotation using the
> logging collector, rather than a full restart? It would unfortunately
> mean that we have to somehow wait for the rotation to complete, since
> that's asynchronous.

I wasn't able to make live rotation work in a sane way. So, v14 tries
to thread the needle with a riff on your earlier idea:

> If you want to keep this information around
> for debugging, I guess that we could just print the contents of the
> backend logs to regress_log_001_password instead?  This could be done
> with a simple wrapper routine that prints the past contents of the log
> file before truncating them.

Rather than putting Postgres log data into the Perl logs, I rotate the
logs exactly once at the beginning -- so that there's an
old 001_ssltests_primary.log, and a new 001_ssltests_primary_1.log --
and then every time we truncate the logfile, I shuffle the bits from
the new logfile into the old one. So no one has to learn to find the
log entries in a new place, we don't get an explosion of rotated logs,
we don't lose the log data, we don't match incorrect portions of the
logs, and we only pay the restart price once. This is wrapped into a
small Perl module, LogCollector.

WDYT?

--Jacob
diff --git a/src/test/ssl/t/001_ssltests.pl b/src/test/ssl/t/001_ssltests.pl
index 9fac0689ee..b5fb15f794 100644
--- a/src/test/ssl/t/001_ssltests.pl
+++ b/src/test/ssl/t/001_ssltests.pl
@@ -10,6 +10,7 @@ use FindBin;
 use lib $FindBin::RealBin;
 
 use SSLServer;
+use LogCollector;
 
 if ($ENV{with_ssl} ne 'openssl')
 {
@@ -454,8 +455,7 @@ test_connect_fails(
 );
 
 
-my $log = $node->rotate_logfile();
-$node->restart;
+my $log = LogCollector->new($node);
 
 # correct client cert using whole DN
 my $dn_connstr = "$common_connstr dbname=certdb_dn";
@@ -466,16 +466,10 @@ test_connect_ok(
"certificate authorization succeeds with DN mapping"
 );
 
-$node->stop('fast');
-my $log_contents = slurp_file($log);
-
-like(
-   $log_contents,
+$log->like(
qr/connection authenticated: 
identity="CN=ssltestuser-dn,OU=Testing,OU=Engineering,O=PGDG" method=cert/,
"authenticated DNs are logged");
 
-$node->start;
-
 # same thing but with a regex
 $dn_connstr = "$common_connstr dbname=certdb_dn_re";
 
@@ -485,8 +479,7 @@ test_connect_ok(
"certificate authorization succeeds with DN regex mapping"
 );
 
-$log = $node->rotate_logfile();
-$node->restart;
+$log->rotate;
 
 # same thing but using explicit CN
 $dn_connstr = "$common_connstr dbname=certdb_cn";
@@ -497,17 +490,10 @@ test_connect_ok(
"certificate authorization succeeds with CN mapping"
 );
 
-$node->stop('fast');
-$log_contents = slurp_file($log);
-
-like(
-   $log_contents,
+$log->like(
qr/connection authenticated: 
identity="CN=ssltestuser-dn,OU=Testing,OU=Engineering,O=PGDG" method=cert/,
"full authenticated DNs are logged even in CN mapping mode");
 
-$node->start;
-
-
 
 TODO:
 {
@@ -565,8 +551,7 @@ SKIP:
"certificate authorization fails because of file permissions");
 }
 
-$log = $node->rotate_logfile();
-$node->restart;
+$log->rotate;
 
 # client cert belonging to another user
 test_connect_fails(
@@ -576,17 +561,10 @@ test_connect_fails(
"certificate authorization fails with client cert belonging to another 
user"
 );
 
-$node->stop('fast');
-$log_contents = slurp_file($log);
-
-like(
-   $log_contents,
+$log->like(
qr/connection authenticated: identity="CN=ssltestuser" method=cert/,
"cert authentication succeeds even if authorization fails");
 
-$log = $node->rotate_logfile();
-$node->restart;
-
 # revoked client cert
 test_connect_fails(
$common_connstr,
@@ -594,25 +572,16 @@ test_connect_fails(
qr/SSL error/,
"certificate authorization fails with revoked client cert");
 
-$node->stop('fast');
-$log_contents = slurp_file($log);
-
-unlike(
-   $log_contents,
+$log->unlike(
qr/connection authenticated:/,
"revoked certs do not authenticate users");
 
-$node->start;
-
 # Check that connecting with auth-option verify-full in pg_hba:
 # works, iff username matches Common Name
 # fails, iff username doesn't match Common Name.
 $common_connstr =
   "sslrootcert=ssl/root+server_ca.crt sslmode=require dbname=verifydb 
hostaddr=$SERVERHOSTADDR";
 
-$log = $node->rotate_logfile();
-$node->restart;
-
 test_connect_ok(
$common_connstr,
"user=ssltestuser sslcert=ssl/client.crt sslkey=ssl/client_tmp.key",
@@ -634,18 +603,12 @@ test_connect_ok(
"auth_option clientcert=verify-ca succeeds with mismatching username 
and Common Name"
 );
 
-$node->stop('fast');
-$log_contents = slurp_file($log);
-
 # None of the above connections to verifydb should have resulted in
 # authentication.
-unlike(
-   $log_contents,
+$log->unlike(
qr/connection authenticated:/,
"trust auth method does not set authenticated

What to call an executor node which lazily caches tuples in a hash table?

[David Rowley]

Hackers,

Over on [1] I've been working on adding a new type of executor node
which caches tuples in a hash table belonging to a given cache key.

The current sole use of this node type is to go between a
parameterized nested loop and the inner node in order to cache
previously seen sets of parameters so that we can skip scanning the
inner scan for parameter values that we've already cached.  The node
could also be used to cache results from correlated subqueries,
although that's not done yet.

The cache limits itself to not use more than hash_mem by evicting the
least recently used entries whenever more space is needed for new
entries.

Currently, in the patch, the node is named "Result Cache".  That name
was not carefully thought out. I just needed to pick something when
writing the code.

Here's an EXPLAIN output with the current name:

postgres=# explain (costs off) select relkind,c from pg_class c1,
lateral (select count(*) c from pg_class c2 where c1.relkind =
c2.relkind) c2;
 QUERY PLAN

 Nested Loop
   ->  Seq Scan on pg_class c1
   ->  Result Cache
 Cache Key: c1.relkind
 ->  Aggregate
   ->  Seq Scan on pg_class c2
 Filter: (c1.relkind = relkind)
(7 rows)

I just got off a team call with Andres, Thomas and Melanie. During the
call I mentioned that I didn't like the name "Result Cache". Many name
suggestions followed:

Here's a list of a few that were mentioned:

Probe Cache
Tuple Cache
Keyed Materialize
Hash Materialize
Result Cache
Cache
Hash Cache
Lazy Hash
Reactive Hash
Parameterized Hash
Parameterized Cache
Keyed Inner Cache
MRU Cache
MRU Hash

I was hoping to commit the final patch pretty soon, but thought I'd
have another go at seeing if we can get some consensus on a name
before doing that. Otherwise, I'd sort of assumed that we'd just reach
some consensus after everyone complained about the current name after
the feature is committed.

My personal preference is "Lazy Hash", but I feel it might be better
to use the word "Reactive" instead of "Lazy".

There was some previous discussion on the name in [2]. I suggested
some other names in [3]. Andy voted for "Tuple Cache" in [4]

Votes? Other suggestions?

(I've included all the people who have shown some previous interest in
naming this node.)

David

[1] 
https://www.postgresql.org/message-id/flat/CAApHDvrPcQyQdWERGYWx8J%2B2DLUNgXu%2BfOSbQ1UscxrunyXyrQ%40mail.gmail.com
[2] 
https://www.postgresql.org/message-id/CA%2BTgmoZMxLeanqrS00_p3xNsU3g1v3EKjNZ4dM02ShRxxLiDBw%40mail.gmail.com
[3] 
https://www.postgresql.org/message-id/CAApHDvoj_sH1H3JVXgHuwnxf1FQbjRVOqqgxzOgJX13NiA9-cg%40mail.gmail.com
[4] 
https://www.postgresql.org/message-id/CAKU4AWoshM0JoymwBK6PKOFDMKg-OO6qtSVU_Piqb0dynxeL5w%40mail.gmail.com

Re: What to call an executor node which lazily caches tuples in a hash table?

[Zhihong Yu]

Hi,
I was reading this part of the description:

the Result Cache's
hash table is much smaller than the hash join's due to result cache only
caching useful values rather than all tuples from the inner side of the
join.

I think the word 'Result' should be part of the cache name considering the
above.

Cheers

On Tue, Mar 30, 2021 at 4:30 PM David Rowley  wrote:

> Hackers,
>
> Over on [1] I've been working on adding a new type of executor node
> which caches tuples in a hash table belonging to a given cache key.
>
> The current sole use of this node type is to go between a
> parameterized nested loop and the inner node in order to cache
> previously seen sets of parameters so that we can skip scanning the
> inner scan for parameter values that we've already cached.  The node
> could also be used to cache results from correlated subqueries,
> although that's not done yet.
>
> The cache limits itself to not use more than hash_mem by evicting the
> least recently used entries whenever more space is needed for new
> entries.
>
> Currently, in the patch, the node is named "Result Cache".  That name
> was not carefully thought out. I just needed to pick something when
> writing the code.
>
> Here's an EXPLAIN output with the current name:
>
> postgres=# explain (costs off) select relkind,c from pg_class c1,
> lateral (select count(*) c from pg_class c2 where c1.relkind =
> c2.relkind) c2;
>  QUERY PLAN
> 
>  Nested Loop
>->  Seq Scan on pg_class c1
>->  Result Cache
>  Cache Key: c1.relkind
>  ->  Aggregate
>->  Seq Scan on pg_class c2
>  Filter: (c1.relkind = relkind)
> (7 rows)
>
> I just got off a team call with Andres, Thomas and Melanie. During the
> call I mentioned that I didn't like the name "Result Cache". Many name
> suggestions followed:
>
> Here's a list of a few that were mentioned:
>
> Probe Cache
> Tuple Cache
> Keyed Materialize
> Hash Materialize
> Result Cache
> Cache
> Hash Cache
> Lazy Hash
> Reactive Hash
> Parameterized Hash
> Parameterized Cache
> Keyed Inner Cache
> MRU Cache
> MRU Hash
>
> I was hoping to commit the final patch pretty soon, but thought I'd
> have another go at seeing if we can get some consensus on a name
> before doing that. Otherwise, I'd sort of assumed that we'd just reach
> some consensus after everyone complained about the current name after
> the feature is committed.
>
> My personal preference is "Lazy Hash", but I feel it might be better
> to use the word "Reactive" instead of "Lazy".
>
> There was some previous discussion on the name in [2]. I suggested
> some other names in [3]. Andy voted for "Tuple Cache" in [4]
>
> Votes? Other suggestions?
>
> (I've included all the people who have shown some previous interest in
> naming this node.)
>
> David
>
> [1]
> https://www.postgresql.org/message-id/flat/CAApHDvrPcQyQdWERGYWx8J%2B2DLUNgXu%2BfOSbQ1UscxrunyXyrQ%40mail.gmail.com
> [2]
> https://www.postgresql.org/message-id/CA%2BTgmoZMxLeanqrS00_p3xNsU3g1v3EKjNZ4dM02ShRxxLiDBw%40mail.gmail.com
> [3]
> https://www.postgresql.org/message-id/CAApHDvoj_sH1H3JVXgHuwnxf1FQbjRVOqqgxzOgJX13NiA9-cg%40mail.gmail.com
> [4]
> https://www.postgresql.org/message-id/CAKU4AWoshM0JoymwBK6PKOFDMKg-OO6qtSVU_Piqb0dynxeL5w%40mail.gmail.com
>
>
>

Re: unconstrained memory growth in long running procedure stored procedure after upgrading 11-12

[Justin Pryzby]

On Tue, Mar 30, 2021 at 04:17:03PM -0500, Merlin Moncure wrote:
> Hello all,
> 
> We just upgraded from postgres 11 to 12.6 and our server is running
> out of memory and rebooting about 1-2 times a day.Application
> architecture is a single threaded stored procedure, executed with CALL
> that loops and never terminates. With postgres 11 we had no memory
> issues.  Ultimately the crash looks like this:
> 
> terminate called after throwing an instance of 'std::bad_alloc'
>   what():  std::bad_alloc
> 2021-03-29 04:34:31.262 CDT [1413] LOG:  server process (PID 9792) was
> terminated by signal 6: Aborted

I haven't tried your test, but this sounds a lot like the issue I reported with
JIT, which is enabled by default in v12.

https://www.postgresql.org/docs/12/release-12.html
Enable Just-in-Time (JIT) compilation by default, if the server has been built 
with support for it (Andres Freund)
Note that this support is not built by default, but has to be selected 
explicitly while configuring the build.

https://www.postgresql.org/message-id/20201001021609.GC8476%40telsasoft.com
terminate called after throwing an instance of 'std::bad_alloc'

I suggest to try ALTER SYSTEM SET jit_inline_above_cost=-1; SELECT 
pg_reload_conf();

> memory growth immediately.   It's about a gigabyte an hour in my test.
> Sorry for the large-ish attachment.

Your reproducer is probably much better than mine was.

-- 
Justin

Re: libpq debug log

['alvhe...@alvh.no-ip.org']

Okay, pushed this patch and the new testing for it based on
libpq_pipeline.  We'll see how the buildfarm likes it.

I made some further changes to the last version; user-visibly, I split
the trace flags in two, keeping the timestamp suppression separate from
the redacting feature for regression testing.

I didn't like the idea of silently skipping the redacted fields, so I
changed the code to print  or  instead.  I also made the
redacting occur in the last mile (pqTraceOutputInt32 / String) rather
that in their callers: it seemed quite odd to advance the cursor in the
"else" branch.

I refactored the duplicate code that appeared for Notice and Error.
In that function, we redact not only the 'L' field (what Iwata-san was
doing) but also 'F' (file) and 'R' (routine) because those things can
move around for reasons that are not interesting to testing this code.

In the libpq_pipeline commit I added 'pipeline_abort' and 'transaction'
to the cases that generate traces, which adds coverage for
NoticeResponse and ErrorResponse.

-- 
Álvaro Herrera39°49'30"S 73°17'W