date:20240911


On 04.09.24 11:28, Peter Eisentraut wrote:

On 03.09.24 22:56, Jacob Champion wrote:

The parse_strval field could use a better explanation.

I actually don't understand the need for this field.  AFAICT, this is
just used to record whether strval is valid.

No, it's meant to track the value of the need_escapes argument to the
constructor. I've renamed it and moved the assignment to hopefully
make that a little more obvious. WDYT?


Yes, this is clearer.

This patch (v28-0001) looks good to me now.


This has been committed.

About the subsequent patches:

Is there any sense in dealing with the libpq and backend patches 
separately in sequence, or is this split just for ease of handling?


(I suppose the 0004 "review comments" patch should be folded into the 
respective other patches?)


What could be the next steps to keep this moving along, other than stare 
at the remaining patches until we're content with them? ;-)

Re: Separate HEAP WAL replay logic into its own file

On Tue, Jul 30, 2024 at 06:48:26AM +, Li, Yong wrote:
> Thank you Kou for your review. I will move the CF to the next
> phase and see what happens.

Quite a fan of what you are proposing here, knowing that heapam.c is
still 8.8k lines of code even after moving the 1.3k lines dedicated to
WAL records.

+#include "access/heapam_xlog.h"

This is included in heapam.h, but missing from the patch.  I guess
that you fat-fingered a `git add`.
--
Michael

signature.asc
Description: PGP signature

Re: ANALYZE ONLY

2024-09-11 Thread Michael Harris

Thanks for the feedback.

On Tue, 10 Sept 2024 at 22:03, torikoshia  wrote:
> Regarding the addition of partition descendant tables, should we also
> update the below comment on expand_vacuum_rel? Currently it refers only
> partitions:
>
> |  * Given a VacuumRelation, fill in the table OID if it wasn't
> specified,
> |  * and optionally add VacuumRelations for partitions of the table.
>

Well spotted! I have updated the comment to read:

 * Given a VacuumRelation, fill in the table OID if it wasn't specified,
 * and optionally add VacuumRelations for partitions or descendant tables
 * of the table.

Cheers
Mike.

v5-0001-Implementation-of-the-ONLY-feature.patch
Description: Binary data

Re: json_query conditional wrapper bug

2024-09-11 Thread Amit Langote

On Wed, Sep 11, 2024 at 5:15 AM Peter Eisentraut  wrote:
> On 10.09.24 10:00, Amit Langote wrote:
> > Sorry for missing this report and thanks Andrew for the offlist heads up.
> >
> > On Wed, Sep 4, 2024 at 7:16 PM Peter Eisentraut  
> > wrote:
> >> On 28.08.24 11:21, Peter Eisentraut wrote:
> >>> These are ok:
> >>>
> >>> select json_query('{"a": 1, "b": 42}'::jsonb, 'lax $.b' without wrapper);
> >>>json_query
> >>> 
> >>>42
> >>>
> >>> select json_query('{"a": 1, "b": 42}'::jsonb, 'lax $.b' with
> >>> unconditional wrapper);
> >>>json_query
> >>> 
> >>>[42]
> >>>
> >>> But this appears to be wrong:
> >>>
> >>> select json_query('{"a": 1, "b": 42}'::jsonb, 'lax $.b' with conditional
> >>> wrapper);
> >>>json_query
> >>> 
> >>>[42]
> >>>
> >>> This should return an unwrapped 42.
> >>
> >> If I make the code change illustrated in the attached patch, then I get
> >> the correct result here.  And various regression test results change,
> >> which, to me, all look more correct after this patch.  I don't know what
> >> the code I removed was supposed to accomplish, but it seems to be wrong
> >> somehow.  In the current implementation, the WITH CONDITIONAL WRAPPER
> >> clause doesn't appear to work correctly in any case I could identify.
> >
> > Agreed that this looks wrong.
> >
> > I've wondered why the condition was like that but left it as-is,
> > because I thought at one point that that's needed to ensure that the
> > returned single scalar SQL/JSON item is valid jsonb.
> >
> > I've updated your patch to include updated test outputs and a nearby
> > code comment expanded.  Do you intend to commit it or do you prefer
> > that I do?
>
> This change looks unrelated:
>
> -ERROR:  new row for relation "test_jsonb_constraints" violates check
> constraint "test_jsonb_constraint4"
> +ERROR:  new row for relation "test_jsonb_constraints" violates check
> constraint "test_jsonb_constraint5"
>
> Is this some randomness in the way these constraints are evaluated?

The result of JSON_QUERY() in the CHECK constraint changes, so the
constraint that previously failed now succeeds after this change,
because the comparison looked like this before and after:

-- before
postgres=# select jsonb '[10]' < jsonb '[10]';
 ?column?
--
 f
(1 row)

-- after
postgres=# select jsonb '10' < jsonb '[10]';
 ?column?
--
 t
(1 row)

That causes the next constraint to be evaluated and its failure
reported instead.

In the attached, I've adjusted the constraint for the test case to be
a bit more relevant and removed a nearby somewhat redundant test,
mainly because its output changes after the adjustment.

--
Thanks, Amit Langote


v3-0001-WIP-Fix-JSON_QUERY-WITH-CONDITIONAL-WRAPPER.patch
Description: Binary data

Re: broken build - FC 41

2024-09-11 Thread Daniel Gustafsson

> On 9 Sep 2024, at 15:20, Pavel Stehule  wrote:

> The question is why the missing header was not detected by configure?

We don't test for every 3rd party header we include.  If engines were separate
from OpenSSL we'd probably probe for it, but this separation is a packager
decision and not one from the OpenSSL project.

> The description of this package says so the OpenSSL ENGINE is deprecated?

OpenSSL deprecated the concept of engines in favor of providers in OpenSSL 3.0,
but as is common with OpenSSL they are still around and there is a way to keep
them running albeit in a limited fashion.

PostgreSQL still support OpenSSL 1.1.1 where engines aren't deprecated, and I
expect we will for some time.

--
Daniel Gustafsson

Re: Psql meta-command conninfo+

2024-09-11 Thread Hunaid Sohail

Hi,

On Tue, Sep 10, 2024 at 9:16 PM Alvaro Herrera 
wrote:

> On 2024-Sep-10, Jim Jones wrote:
>
> > Is \conninfo+ no longer supposed to return the results in tabular form?
> > At least it wasn't the case till v28.
>
> I suspect the reason it's no longer a table is that it was previously a
> query (which is easily printed as a table by calling printQuery) and now
> it's just a client-side thing, and Hunaid didn't know how to handle that
> as a table.  The good news is, it should be really easy to do
> printTableInit(), then a bunch of printTableAddHeader() and
> printTableAddCell(), end with printTable().  I think the tabular format
> is better for sure.
>

I have made the requested changes. Now output is returned in tabular form.
Indentation/whitespace issues are fixed.

$bin/psql --port=5430 postgres
postgres=# \conninfo+
You are connected to database "postgres" as user "hunaid" via socket in
"/tmp" at port "5430".
Connection Information
  Parameter   | Value
--+
 Protocol Version | 3
 SSL Connection   | no
 GSSAPI Authenticated | no
 Client Encoding  | UTF8
 Server Encoding  | UTF8
 Session User | hunaid
 Backend PID  | 121800
(7 rows)

Regards,
Hunaid Sohail


v32-0001-Add-psql-meta-command-conninfo.patch
Description: Binary data

Re: Conflict detection for update_deleted in logical replication

2024-09-11 Thread Amit Kapila

On Wed, Sep 11, 2024 at 8:32 AM Zhijie Hou (Fujitsu)
 wrote:
>
> On Tuesday, September 10, 2024 7:25 PM Amit Kapila  
> wrote:
> >
> > One minor comment on 0003
> > ===
> > 1.
> > get_slot_confirmed_flush()
> > {
> > ...
> > + /*
> > + * To prevent concurrent slot dropping and creation while filtering the
> > + * slots, take the ReplicationSlotControlLock outside of the loop.
> > + */
> > + LWLockAcquire(ReplicationSlotControlLock, LW_SHARED);
> > +
> > + foreach_ptr(String, name, MySubscription->feedback_slots) { XLogRecPtr
> > + confirmed_flush; ReplicationSlot *slot;
> > +
> > + slot = ValidateAndGetFeedbackSlot(strVal(name));
> >
> > Why do we need to validate slots each time here? Isn't it better to do it 
> > once?
>
> I think it's possible that the slot was correct but changed or dropped later,
> so it could be useful to give a warning in this case to hint user to adjust 
> the
> slots, otherwise, the xmin of the publisher's slot won't be advanced and might
> cause dead tuples accumulation. This is similar to the checks we performed for
> the slots in "synchronized_standby_slots". (E.g. StandbySlotsHaveCaughtup)
>

In the case of "synchronized_standby_slots", we seem to be invoking
such checks via StandbySlotsHaveCaughtup() when we need to wait for
WAL and also we have some optimizations that avoid the frequent
checking for validation checks. OTOH, this patch doesn't have any such
optimizations. We can optimize it by maintaining a local copy of
feedback slots to avoid looping all the slots each time (if this is
required, we can make it a top-up patch so that it can be reviewed
separately). I have also thought of maintaining the updated value of
confirmed_flush_lsn for feedback slots corresponding to a subscription
in shared memory but that seems tricky because then we have to
maintain slot->subscription mapping. Can you think of any other ways?

Having said that it is better to profile this in various scenarios
like by increasing the frequency of keep_alieve message and or in idle
subscriber cases where we try to send this new feedback message.

-- 
With Regards,
Amit Kapila.

Fix orphaned 2pc file which may casue instance restart failed

2024-09-11 Thread ????

Hi all,     I found that there is a race condition between two 
global transaction, which may cause instance
restart failed with error 'could not access status of transaction xxx","Could 
not open file ""pg_xact/xxx"": No such file or directory'.


     The scenery to reproduce the problem is:
         1. gxact1 is doing 
`FinishPreparedTransaction` and checkpoint
             is issued, so gxact1 will 
generate a 2pc file.
         2. then gxact1 was removed from 
`TwoPhaseState->prepXacts` and
             its state memory was returned 
to freelist.
         3. but just before gxact1 remove its 2pc 
file, gxact2 is issued,
             gxact2 will reuse the same 
state memory of gxact1 and will
             reset `gxact->ondisk` to 
false.
        4. gxact1 continue and found that 
`gxact->ondisk` is false, it won't
            remove its 2pc file. This file is 
orphaned.


    If gxact1's local xid is not frozen, the startup process will 
remove
the orphaned 2pc file. However, if the xid's corresponding clog file is
truncated by `vacuum`, the startup process will raise error 'could not
access status of transaction xxx', due to it could not found the
transaction's status file in dir `pg_xact`.



   The potential fix is attached.

0001-Fix-orphaned-2pc-file-which-may-casue-instance-resta.patch
Description: Binary data

Re: Fix orphaned 2pc file which may casue instance restart failed

On Sun, Sep 08, 2024 at 01:01:37PM +0800, 清浅 wrote:
> Hi all,  I found that there is a race condition
> between two global transaction, which may cause instance restart
> failed with error 'could not access status of transaction
> xxx","Could not open file ""pg_xact/xxx"": No such file or
> directory'.
> 
> 
>   The scenery to reproduce the problem is:
> 1. gxact1 is doing `FinishPreparedTransaction` and checkpoint
>   is issued, so gxact1 will generate a 2pc file.
> 2. then gxact1 was removed from `TwoPhaseState->prepXacts` and
>   its state memory was returned to freelist.
> 3. but just before gxact1 remove its 2pc file, gxact2 is issued,
>   gxact2 will reuse the same state memory of gxact1 and will
>   reset `gxact->ondisk` to false.
> 4. gxact1 continue and found that `gxact->ondisk` is false, it won't
>   remove its 2pc file. This file is orphaned.
> 
>   If gxact1's local xid is not frozen, the startup process will remove
> the orphaned 2pc file. However, if the xid's corresponding clog file is
> truncated by `vacuum`, the startup process will raise error 'could not
> access status of transaction xxx', due to it could not found the
> transaction's status file in dir `pg_xact`.

Hmm.  I've not seen that in the field.  Let me check that..
--
Michael


signature.asc
Description: PGP signature

Re: not null constraints, again

2024-09-11 Thread jian he

On Wed, Sep 11, 2024 at 9:11 AM jian he  wrote:
>
> On Wed, Sep 11, 2024 at 2:18 AM Alvaro Herrera  
> wrote:
> >
> > Hello, here's a v2 of this patch.  I have fixed --I think-- all the
> > issues you and Tender Wang reported (unless I declined a fix in some
> > previous email).
> >

after applying your changes.

in ATExecAddConstraint, ATAddCheckNNConstraint.
ATAddCheckNNConstraint(wqueue, tab, rel,
newConstraint, recurse, false, is_readd,
lockmode);
if passed to ATAddCheckNNConstraint rel is a partitioned table.
ATAddCheckNNConstraint itself can recurse to create not-null pg_constraint
for itself and it's partitions (children table).
This is fine as long as we only call ATExecAddConstraint once.

but ATExecAddConstraint itself will recurse, it will call
the partitioned table and each of the partitions.

The first time ATExecAddConstraint with a partitioned table with the
following calling chain
ATAddCheckNNConstraint-> AddRelationNewConstraints -> AdjustNotNullInheritance1
works fine.

the second time ATExecAddConstraint with the partitions
ATAddCheckNNConstraint-> AddRelationNewConstraints -> AdjustNotNullInheritance1
AdjustNotNullInheritance1 will make the partitions
pg_constraint->coninhcount bigger than 1.

for example:
drop table if exists idxpart, idxpart0, idxpart1 cascade;
create table idxpart (a int) partition by range (a);
create table idxpart0 (a int primary key);
alter table idxpart attach partition idxpart0 for values from (0) to (1000);
alter table idxpart add primary key (a);

After the above query
pg_constraint->coninhcount of idxpart0_a_not_null  becomes 2.
but it should be 1
?

Re: Test improvements and minor code fixes for formatting.c.

2024-09-11 Thread Aleksander Alekseev

Hi Tom,

> Meh ... cfbot points out I did the float-to-int conversions wrong.
> v2 attached.

I'm having difficulties applying the patch. Could you please do `git
format-patch` and resend it?

-- 
Best regards,
Aleksander Alekseev

Re: broken build - FC 41

2024-09-11 Thread Pavel Stehule

Hi

st 11. 9. 2024 v 9:54 odesílatel Daniel Gustafsson  napsal:

> > On 9 Sep 2024, at 15:20, Pavel Stehule  wrote:
>
> > The question is why the missing header was not detected by configure?
>
> We don't test for every 3rd party header we include.  If engines were
> separate
> from OpenSSL we'd probably probe for it, but this separation is a packager
> decision and not one from the OpenSSL project.
>
> > The description of this package says so the OpenSSL ENGINE is deprecated?
>
> OpenSSL deprecated the concept of engines in favor of providers in OpenSSL
> 3.0,
> but as is common with OpenSSL they are still around and there is a way to
> keep
> them running albeit in a limited fashion.
>
> PostgreSQL still support OpenSSL 1.1.1 where engines aren't deprecated,
> and I
> expect we will for some time.
>

ok

Thank you for the reply

Regards

Pavel

>
> --
> Daniel Gustafsson
>
>

Re: json_query conditional wrapper bug


On 11.09.24 09:51, Amit Langote wrote:

I've updated your patch to include updated test outputs and a nearby
code comment expanded.  Do you intend to commit it or do you prefer
that I do?


This change looks unrelated:

-ERROR:  new row for relation "test_jsonb_constraints" violates check
constraint "test_jsonb_constraint4"
+ERROR:  new row for relation "test_jsonb_constraints" violates check
constraint "test_jsonb_constraint5"

Is this some randomness in the way these constraints are evaluated?


The result of JSON_QUERY() in the CHECK constraint changes, so the
constraint that previously failed now succeeds after this change,
because the comparison looked like this before and after:

-- before
postgres=# select jsonb '[10]' < jsonb '[10]';
  ?column?
--
  f
(1 row)

-- after
postgres=# select jsonb '10' < jsonb '[10]';
  ?column?
--
  t
(1 row)

That causes the next constraint to be evaluated and its failure
reported instead.

In the attached, I've adjusted the constraint for the test case to be
a bit more relevant and removed a nearby somewhat redundant test,
mainly because its output changes after the adjustment.


Ok, that looks good.  Good that we could clear that up a bit.

Re: Psql meta-command conninfo+

2024-09-11 Thread Jim Jones




On 11.09.24 10:16, Hunaid Sohail wrote:
I have made the requested changes. Now output is returned in tabular 
form. Indentation/whitespace issues are fixed.


$bin/psql --port=5430 postgres
postgres=# \conninfo+
You are connected to database "postgres" as user "hunaid" via socket 
in "/tmp" at port "5430".

    Connection Information
      Parameter       | Value
--+
 Protocol Version     | 3
 SSL Connection       | no
 GSSAPI Authenticated | no
 Client Encoding      | UTF8
 Server Encoding      | UTF8
 Session User         | hunaid
 Backend PID          | 121800
(7 rows)


Thanks for working on this.

Any particular reason for the design change? In v28 it returned a table 
with a single row and multiple columns --- one column per attribute. But 
now it returns multiple rows. In this case, I was expecting 1 row with 7 
columns instead of 7 rows with 2 columns.


Jim

Re: PG_TEST_EXTRA and meson

2024-09-11 Thread Ashutosh Bapat

On Mon, Sep 2, 2024 at 8:32 PM Nazir Bilal Yavuz  wrote:
>
> Hi,
>
> On Fri, 30 Aug 2024 at 21:36, Jacob Champion
>  wrote:
> >
> > On Wed, Aug 28, 2024 at 8:21 AM Nazir Bilal Yavuz  
> > wrote:
> > > I do not exactly remember the reason but I think I copied the same
> > > behavior as before, PG_TEST_EXTRA variable was checked in the
> > > src/test/Makefile so I exported it there.
> >
> > Okay, give v3 a try then. This exports directly from Makefile.global.
> > Since that gets pulled into a bunch of places, the scope is a lot
> > wider than it used to be;

This looks similar to what meson does with PG_TEST_EXTRA, it is
available via get_option(). So we are closing the gap between meson
and make. That's the intention.
>  I've disabled it for PGXS so it doesn't end
> > up poisoning other extensions.
>
> Patch looks good and it passes all the test cases in Ashutosh's test script.
>

Thanks Bilal for testing the patch. Can you or Jacob please create one
patchset including both meson and make fixes? Please keep the meson
and make changes in separate patches though. I think the meson patches
come from [1] (they probably need a rebase, git am failed) and make
patch comes from [2].The one fixing make needs a good commit message.

some comments on code

1. comments on changes to meson

+ variable if it exists. See  for

s/exists/set/

-# Test suites that are not safe by default but can be run if selected
-# by the user via the whitespace-separated list in variable PG_TEST_EXTRA.
-# Export PG_TEST_EXTRA so it can be checked in individual tap tests.
-test_env.set('PG_TEST_EXTRA', get_option('PG_TEST_EXTRA'))

A naive question. What happens if we add PG_TEST_EXTRA in meson.build
itself rather than passing it via testwrap? E.g. like
if "PG_TEST_EXTRA" not in os.environ
test_env.set('PG_TEST_EXTRA', get_option('PG_TEST_EXTRA'))

I am worried that we might have to add an extra argument to testwrap
for every environment variable that influences the tests. Avoiding it
would be better.

option('PG_TEST_EXTRA', type: 'string', value: '',
- description: 'Enable selected extra tests')
+ description: 'Enable selected extra tests, please note that this
configure option is overridden by PG_TEST_EXTRA environment variable
if it exists')

All the descriptions look much shorter than this one. I suggest we
shorten this one too as
"Enable selected extra tests. Overridden by PG_TEST_EXTRA environment variable."
not as short as other descriptions but shorter than before and yet
serves its intended purpose. Or just make it the same as the one in
configure.ac. Either way the descriptions in configure.ac and
meson_options.txt should be in sync.

+# If PG_TEST_EXTRA is not set in the environment, then look for its Meson
+# configure option.
+if "PG_TEST_EXTRA" not in os.environ and args.pg_test_extra:
+ env_dict["PG_TEST_EXTRA"] = args.pg_test_extra
+

If somebody looks at just these lines, it's not clear how
PG_TEST_EXTRA is passed to the test environment if it's available in
os.environ. So one has to understand that env_dict is the test
environment. If that's so, the code and comment rewritten as below
makes more sense to me. What do you think?

# If PG_TEST_EXTRA is not already part of the test environment, check if it's
# passed via program argument --pg_test_extra. Thus we also override
# configuration time value by run time value of PG_TEST_EXTRA.
if "PG_TEST_EXTRA" not in env_dict and args.pg_test_extra:
env_dict["PG_TEST_EXTRA"] = args.pg_test_extra

But in case we decide to fix meson.build as suggested in one of the
commentsabove, this change will be unnecessary.

Note that PG_TEST_EXTRA is used in only TAP tests right now. The way
the value passed to --pg_test_extra is handled in testwrap, it will
available to every test, not just TAP tests. But this looks fine to me
since the documentation of PG_TEST_EXTRA or its name itself does not
show any intention to limit it only to TAP tests.

2. comments on make changes
Since Makefile.global.in is included in src/test/Makefile I was
expecting that the PG_TEST_EXTRA picked from configuration would be
available in src/test/Makefile from which it would be exported. But
that doesn't seem to be the case. In fact, I am getting doubtful about
the role of the current "export PG_TEST_EXTRA" in /src/test/Makefile.
Even if I remove it, it doesn't affect anything. Commands a.
PG_TEST_EXTRA=xid_wraparound make check, b.
PG_TEST_EXTRA=xid_wraparound make -C $XID_MODULE_DIR check run the
tests (and don't skip them).

Anyway with the proposed change PG_TEST_EXTRA passed at configuration
time is used if it's not defined at run time as expected. I think the
patch looks good. Nothing to complain there.

[1] 
https://www.postgresql.org/message-id/CAN55FZ1Tko2N=x4f6icgfhb7syjyo_app-9ebfct-uh6tei...@mail.gmail.com
[2] 
https://www.postgresql.org/message-id/CAOYmi+=6hnvhbfovsv6x2_dvdycudl4amnj7im15gafw__b...@mail.gmail.com

--
Best Wishes,
Ashutosh Bapat

Re: Document DateStyle effect on jsonpath string()


On 10.09.24 22:16, Tom Lane wrote:

Peter Eisentraut  writes:

These JSON path functions are specified by the SQL standard, so they
shouldn't depend on PostgreSQL-specific settings.  At least in new
functionality we should avoid that, no?


Hmm ... but does the standard precisely define the output format?

Since these conversions are built on our own timestamp I/O code,
I rather imagine there is quite a lot of behavior there that's
not to be found in the standard.  That doesn't really trouble
me as long as the spec's behavior is a subset of it (i.e.,
reachable as long as you've got the right parameter settings).


Actually, the standard prohibits this call:

"""
XV) If JM specifies string, then:

1) Forallj,1(one)≤j≤n,
Case:

a) If Ij is not a character string, number, or Boolean value,
then let ST be data exception — non-string SQL/JSON item (2202X).

b) Otherwise, let X be an SQL variable whose value is Ij. Let ML be an 
implementation-defined (IL006) maximum

length of variable-length character strings. Let Vj be the result of

CAST (X AS CHARACTER VARYING(ML)

If this conversion results in an exception condition, then
let ST be that exception condition.
"""

So I guess we have extended this and the current behavior is consistent 
with item b).


What I'm concerned about is that this makes the behavior of JSON_QUERY 
non-immutable.  Maybe there are other reasons for it to be 
non-immutable, in which case this isn't important.  But it might be 
worth avoiding that?

Re: Trim the heap free memory

2024-09-11 Thread Rafia Sabih

Unfortunately, I still see a compiling issue with this patch,

memtrim.c:15:10: fatal error: 'malloc.h' file not found
#include 
 ^~
1 error generated.

On Wed, 28 Aug 2024 at 12:54, shawn wang  wrote:

> Hi Ashutosh,
>
> Ashutosh Bapat  于2024年8月26日周一 19:05写道：
>
>> Hi Shawn,
>> It will be good to document usage of this function. Please add
>> document changes in your patch. We need to document the impact of this
>> function so that users can judiciously decide whether or not to use
>> this function and under what conditions. Also they would know what to
>> expect when they use this function.
>
>
> I have already incorporated the usage of this function into the new patch.
>
>
> Currently, there is no memory information that can be extremely accurate to
> reflect whether a trim operation should be performed. Here are two
> conditions
> that can be used as references:
> 1. Check the difference between the process's memory usage (for example,
> the top command, due to the relationship with shared memory, it is
> necessary
> to subtract SHR from RES) and the statistics of the memory context. If the
> difference is very large, this function should be used to release memory;
> 2. Execute malloc_stats(). If the system bytes are greater than the
> in-use bytes, this indicates that this function can be used to release
> memory.
>
>>
>>
> Running it after a query finishes is one thing but that can't be
>> guaranteed because of the asynchronous nature of signal handlers.
>> malloc_trim() may be called while a query is being executed. We need
>> to assess that impact as well.
>>
>> Can you please share some numbers - TPS, latency etc. with and without
>> this function invoked during a benchmark run?
>>
>
> I have placed malloc_trim() at the end of the exec_simple_query function,
> so that malloc_trim() is executed once for each SQL statement executed. I
> used pgbench to reproduce the performance impact,
> and the results are as follows.
> *Database preparation：*
>
>> create database testc;
>> create user t1;
>> alter database testc owner to t1;
>> ./pgbench testc -U t1 -i -s 100
>> ./pgbench testc -U t1 -S -c 100 -j 100 -T 600
>
> *Without Trim*:
>
>> $./pgbench testc -U t1 -S -c 100 -j 100 -T 600
>> pgbench (18devel)
>> starting vacuum...end.
>> transaction type: 
>> scaling factor: 100
>> query mode: simple
>> number of clients: 100
>> number of threads: 100
>> maximum number of tries: 1
>> duration: 600 s
>> number of transactions actually processed: 551984376
>> number of failed transactions: 0 (0.000%)
>> latency average = 0.109 ms
>> initial connection time = 23.569 ms
>> tps = 920001.842189 (without initial connection time)
>
> *With Trim :*
>
>> $./pgbench testc -U t1 -S -c 100 -j 100 -T 600
>> pgbench (18devel)
>> starting vacuum...end.
>> transaction type: 
>> scaling factor: 100
>> query mode: simple
>> number of clients: 100
>> number of threads: 100
>> maximum number of tries: 1
>> duration: 600 s
>> number of transactions actually processed: 470690787
>> number of failed transactions: 0 (0.000%)
>> latency average = 0.127 ms
>> initial connection time = 23.632 ms
>> tps = 784511.901558 (without initial connection time)
>
>

-- 
Regards,
Rafia Sabih

Re: Add contrib/pg_logicalsnapinspect

2024-09-11 Thread Bertrand Drouvot

Hi,

On Wed, Sep 11, 2024 at 10:30:37AM +0530, Amit Kapila wrote:
> On Tue, Sep 10, 2024 at 8:56 PM Bertrand Drouvot
>  wrote:
> >
> > On Mon, Sep 09, 2024 at 04:24:09PM +0530, Amit Kapila wrote:
> > > On Fri, Aug 30, 2024 at 5:18 PM Bertrand Drouvot
> > >  wrote:
> > > > as we decided not to expose the SnapBuildOnDisk and SnapBuild structs 
> > > > to public
> > > > and to create/expose 2 new functions in snapbuild.c then the functions 
> > > > in the
> > > > module would do nothing but expose the data coming from the 
> > > > snapbuild.c's
> > > > functions (get the tuple and send it to the client). That sounds weird 
> > > > to me to
> > > > create a module that would "only" do so, that's why I thought that in 
> > > > core
> > > > functions taking care of everything make more sense.
> > > >
> > >
> > > I see your point. Does anyone else have an opinion on the need for
> > > these functions and whether to expose them from a contrib module or
> > > have them as core functions?
> >
> > I looked at when the SNAPBUILD_VERSION has been changed:
> >
> > ec5896aed3 (2014)
> > a975ff4980 (2021)
> > 8bdb1332eb (2021)
> > 7f13ac8123 (2022)
> > bb19b70081 (2024)
> >
> > So it's not like we are changing the SnapBuildOnDisk or SnapBuild structs 
> > that
> > frequently. Furthermore, those structs are serialized and so we have to 
> > preserve
> > their on-disk compatibility (means we can change them only in a major 
> > release
> > if we need to).
> >
> > So, I think it would not be that much of an issue to expose those structs 
> > and
> > create a new contrib module (as v1 did propose) instead of in core new 
> > functions.
> >
> > If we want to insist that external modules "should" not rely on those 
> > structs then
> > we could put them into a new internal_snapbuild.h file (instead of 
> > snapbuild.h
> > as proposed in v1).
> >
> 
> Adding snapbuild_internal.h sounds like a good idea.

Thanks for the feedback!

> > At the end, I think that creating a contrib module and exposing those 
> > structs in
> > internal_snapbuild.h make more sense (as compared to in core functions).
> >
> 
> Fail enough. We can keep the module name as logicalinspect so that we
> can extend it for other logical decoding/replication-related files in
> the future.

Yeah, good idea. Done that way in v3 attached.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
>From b51d403fa62af10f090c1e2339627e3ee29af524 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot 
Date: Wed, 14 Aug 2024 08:46:05 +
Subject: [PATCH v3] Add contrib/pg_logicalinspect

Provides SQL functions that allow to inspect logical decoding components.

It currently allows to inspect the contents of serialized logical snapshots of
a running database cluster, which is useful for debugging or educational
purposes.
---
 contrib/Makefile  |   1 +
 contrib/meson.build   |   1 +
 contrib/pg_logicalinspect/.gitignore  |   4 +
 contrib/pg_logicalinspect/Makefile|  31 +++
 .../expected/logical_inspect.out  |  52 
 contrib/pg_logicalinspect/logicalinspect.conf |   1 +
 contrib/pg_logicalinspect/meson.build |  39 +++
 .../pg_logicalinspect--1.0.sql|  43 +++
 contrib/pg_logicalinspect/pg_logicalinspect.c | 249 ++
 .../pg_logicalinspect.control |   5 +
 .../specs/logical_inspect.spec|  34 +++
 doc/src/sgml/contrib.sgml |   1 +
 doc/src/sgml/filelist.sgml|   1 +
 doc/src/sgml/pglogicalinspect.sgml| 145 ++
 src/backend/replication/logical/snapbuild.c   | 190 +
 src/include/port/pg_crc32c.h  |  16 +-
 src/include/replication/internal_snapbuild.h  | 204 ++
 src/include/replication/snapbuild.h   |   2 +-
 18 files changed, 826 insertions(+), 193 deletions(-)
   7.6% contrib/pg_logicalinspect/expected/
   5.7% contrib/pg_logicalinspect/specs/
  32.4% contrib/pg_logicalinspect/
  13.3% doc/src/sgml/
  17.4% src/backend/replication/logical/
   4.1% src/include/port/
  18.9% src/include/replication/

diff --git a/contrib/Makefile b/contrib/Makefile
index abd780f277..952855d9b6 100644
--- a/contrib/Makefile
+++ b/contrib/Makefile
@@ -32,6 +32,7 @@ SUBDIRS = \
 		passwordcheck	\
 		pg_buffercache	\
 		pg_freespacemap \
+		pg_logicalinspect \
 		pg_prewarm	\
 		pg_stat_statements \
 		pg_surgery	\
diff --git a/contrib/meson.build b/contrib/meson.build
index 14a8906865..159ff41555 100644
--- a/contrib/meson.build
+++ b/contrib/meson.build
@@ -46,6 +46,7 @@ subdir('passwordcheck')
 subdir('pg_buffercache')
 subdir('pgcrypto')
 subdir('pg_freespacemap')
+subdir('pg_logicalinspect')
 subdir('pg_prewarm')
 subdir('pgrowlocks')
 subdir('pg_stat_statements')
diff --git a/contrib/pg_logicalinspect/.gitignore b/contrib/pg_logicalinspect/.gitignore
new file mode 10064

Make pg_stat_io view count IOs as bytes instead of blocks

2024-09-11 Thread Nazir Bilal Yavuz

Hi,

Currently, in the pg_stat_io view, IOs are counted as blocks. However,
there are two issues with this approach:

1- The actual number of IO requests to the kernel is lower because IO
requests can be merged before sending the final request. Additionally, it
appears that all IOs are counted in block size.
2- Some IOs may not align with block size. For example, WAL read IOs are
done in variable bytes and it is not possible to correctly show these IOs
in the pg_stat_io view [1].

To address this, I propose showing the total number of IO requests to the
kernel (as smgr function calls) and the total number of bytes in the IO. To
implement this change, the op_bytes column will be removed from the
pg_stat_io view. Instead, the [reads | writes | extends] columns will track
the total number of IO requests, and newly added [read | write |
extend]_bytes columns will track the total number of bytes in the IO.

Example benefit of this change:

Running query [2], the result is:

╔═══╦══╦══╦═══╗
║backend_type   ║  object  ║  context ║ avg_io_blocks ║
╠═══╬══╬══╬═══╣
║   client backend  ║ relation ║ bulkread ║ 15.99 ║
╠═══╬══╬══╬═══╣
║ background worker ║ relation ║ bulkread ║ 15.99 ║
╚═══╩══╩══╩═══╝

You can rerun the same query [2] after setting io_combine_limit to 32 [3].
The result is:

╔═══╦══╦══╦═══╗
║backend_type   ║  object  ║  context ║ avg_io_blocks ║
╠═══╬══╬══╬═══╣
║   client backend  ║ relation ║ bulkread ║ 31.70 ║
╠═══╬══╬══╬═══╣
║ background worker ║ relation ║ bulkread ║ 31.60 ║
╚═══╩══╩══╩═══╝

I believe that having visibility into avg_io_[bytes | blocks] is valuable
information that could help optimize Postgres.

Any feedback would be appreciated.

[1]
https://www.postgresql.org/message-id/CAN55FZ1ny%2B3kpdm5X3nGZ2Jp3wxZO-744eFgxktS6YQ3%3DOKR-A%40mail.gmail.com

[2]
CREATE TABLE t as select i, repeat('a', 600) as filler from
generate_series(1, 1000) as i;
SELECT pg_stat_reset_shared('io');
SELECT * FROM t WHERE i = 0;
SELECT backend_type, object, context, TRUNC((read_bytes / reads / (SELECT
current_setting('block_size')::numeric)), 2) as avg_io_blocks FROM
pg_stat_io WHERE reads > 0;

[3] SET io_combine_limit TO 32;

-- 
Regards,
Nazir Bilal Yavuz
Microsoft
From f02b0d261880aa3f933a9350b6b1557f6b14f292 Mon Sep 17 00:00:00 2001
From: Nazir Bilal Yavuz 
Date: Wed, 11 Sep 2024 11:04:18 +0300
Subject: [PATCH v1] Make pg_stat_io count IOs as bytes instead of blocks

Currently in pg_stat_io view, IOs are counted as blocks. There are two
problems with this approach:

1- The actual number of I/O requests sent to the kernel is lower because
I/O requests may be merged before being sent. Additionally, it gives the
impression that all I/Os are done in block size, which shadows the
benefits of merging I/O requests.

2- There may be some IOs which are not done in block size in the future.
For example, WAL read IOs are done in variable bytes and it is not
possible to correctly show these IOs in pg_stat_io view.

Because of these problems, now show the total number of IO requests to
the kernel (as smgr function calls) and total number of bytes in the IO.
Also, remove op_bytes column from the pg_stat_io view.
---
 src/include/catalog/pg_proc.dat|  6 +-
 src/include/pgstat.h   |  9 ++-
 src/backend/catalog/system_views.sql   |  4 +-
 src/backend/storage/buffer/bufmgr.c| 14 ++---
 src/backend/storage/buffer/localbuf.c  |  6 +-
 src/backend/storage/smgr/md.c  |  4 +-
 src/backend/utils/activity/pgstat_io.c | 63 ---
 src/backend/utils/adt/pgstatfuncs.c| 87 +++---
 src/test/regress/expected/rules.out|  6 +-
 doc/src/sgml/monitoring.sgml   | 61 +++---
 10 files changed, 184 insertions(+), 76 deletions(-)

diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index ff5436acacf..b0dab15bfd4 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -5826,9 +5826,9 @@
   proname => 'pg_stat_get_io', prorows => '30', proretset => 't',
   provolatile => 'v', proparallel => 'r', prorettype => 'record',
   proargtypes => '',
-  proallargtypes => '{text,text,text,int8,float8,int8,float8,int8,float8,int8,float8,int8,int8,int8,int8,int8,float8,timestamptz}',
-  proargmodes => '{o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o}',
-  proargnames => '{backend_type,object,context,reads,read_time,writes,write_time,writebacks,writeback_time,extends,extend_time,op_bytes,hits,evictions,reuses,fsyncs,fsync_time,stats_reset}',
+  proallargtypes => '{text,text,text,int8,numeric,float8,int8,numeric,float8,int8,float8,in

Re: json_query conditional wrapper bug

2024-09-11 Thread Amit Langote

On Wed, Sep 11, 2024 at 6:57 PM Peter Eisentraut  wrote:
> On 11.09.24 09:51, Amit Langote wrote:
> >>> I've updated your patch to include updated test outputs and a nearby
> >>> code comment expanded.  Do you intend to commit it or do you prefer
> >>> that I do?
> >>
> >> This change looks unrelated:
> >>
> >> -ERROR:  new row for relation "test_jsonb_constraints" violates check
> >> constraint "test_jsonb_constraint4"
> >> +ERROR:  new row for relation "test_jsonb_constraints" violates check
> >> constraint "test_jsonb_constraint5"
> >>
> >> Is this some randomness in the way these constraints are evaluated?
> >
> > The result of JSON_QUERY() in the CHECK constraint changes, so the
> > constraint that previously failed now succeeds after this change,
> > because the comparison looked like this before and after:
> >
> > -- before
> > postgres=# select jsonb '[10]' < jsonb '[10]';
> >   ?column?
> > --
> >   f
> > (1 row)
> >
> > -- after
> > postgres=# select jsonb '10' < jsonb '[10]';
> >   ?column?
> > --
> >   t
> > (1 row)
> >
> > That causes the next constraint to be evaluated and its failure
> > reported instead.
> >
> > In the attached, I've adjusted the constraint for the test case to be
> > a bit more relevant and removed a nearby somewhat redundant test,
> > mainly because its output changes after the adjustment.
>
> Ok, that looks good.  Good that we could clear that up a bit.

Thanks for checking.  Would you like me to commit it?

-- 
Thanks, Amit Langote

Re: Test improvements and minor code fixes for formatting.c.

2024-09-11 Thread Hunaid Sohail

Hi,

On Wed, Sep 11, 2024 at 2:33 PM Aleksander Alekseev <
aleksan...@timescale.com> wrote:

> I'm having difficulties applying the patch. Could you please do `git
> format-patch` and resend it?
>

Yes, I guess there is some issue with the patch but somehow I was able to
apply it.

make installcheck-world -> tested, passed
Documentation -> tested, passed

Regards,
Hunaid Sohail

Re: Psql meta-command conninfo+

2024-09-11 Thread Hunaid Sohail

Hi Jim,

On Wed, Sep 11, 2024 at 3:03 PM Jim Jones  wrote:

> Thanks for working on this.
>
> Any particular reason for the design change? In v28 it returned a table
> with a single row and multiple columns --- one column per attribute. But
> now it returns multiple rows. In this case, I was expecting 1 row with 7
> columns instead of 7 rows with 2 columns.
>

I am not sure which design you are referring to.
I haven't applied the v28 patch but the original author in thread [1]
provided sample output. The output is in tabular form with 2 columns and
multiple rows.

[1]
https://www.postgresql.org/message-id/CP8P284MB249615AED23882E1E185C8ABEC3C2%40CP8P284MB2496.BRAP284.PROD.OUTLOOK.COM

Regards,
Hunaid Sohail

Re: json_query conditional wrapper bug


On 11.09.24 13:25, Amit Langote wrote:

On Wed, Sep 11, 2024 at 6:57 PM Peter Eisentraut  wrote:

On 11.09.24 09:51, Amit Langote wrote:

I've updated your patch to include updated test outputs and a nearby
code comment expanded.  Do you intend to commit it or do you prefer
that I do?


This change looks unrelated:

-ERROR:  new row for relation "test_jsonb_constraints" violates check
constraint "test_jsonb_constraint4"
+ERROR:  new row for relation "test_jsonb_constraints" violates check
constraint "test_jsonb_constraint5"

Is this some randomness in the way these constraints are evaluated?


The result of JSON_QUERY() in the CHECK constraint changes, so the
constraint that previously failed now succeeds after this change,
because the comparison looked like this before and after:

-- before
postgres=# select jsonb '[10]' < jsonb '[10]';
   ?column?
--
   f
(1 row)

-- after
postgres=# select jsonb '10' < jsonb '[10]';
   ?column?
--
   t
(1 row)

That causes the next constraint to be evaluated and its failure
reported instead.

In the attached, I've adjusted the constraint for the test case to be
a bit more relevant and removed a nearby somewhat redundant test,
mainly because its output changes after the adjustment.


Ok, that looks good.  Good that we could clear that up a bit.


Thanks for checking.  Would you like me to commit it?


Please do.

Re: BUG #18598: AddressSanitizer detects use after free inside json_unique_hash_match()

2024-09-11 Thread Tomas Vondra

On 9/10/24 21:47, Tomas Vondra wrote:
> ...
>
> The only question that bothers me a little bit is the possibility of a
> memory leak - could it happen that we keep the copied key much longer
> than needed? Or does aggcontext have with the right life span? AFAICS
> that's where we allocate the aggregate state, so it seems fine.
> 
> Also, how far back do we need to backpatch this? ITSM PG15 does not have
> this issue, and it was introduced with the SQL/JSON stuff in PG16. Is
> that correct?
> 

Nah, I spent a bit of time looking for a memory leak, but I don't think
there's one, or at least not a new one. We use the same memory context
as for the hash table / buffer, so that should be fine.

But this made me realize the code in json_build_object_worker() can
simply use pstrdup() to copy the key into CurrentMemoryContext, which is
where the hash table of unique keys is. In fact, using unique_check.mcxt
would not be quite right:

   MemoryContext mcxt;   /* context for saving skipped keys */

And this has nothing to do with skipped keys.

So I adjusted that way and pushed.

Thanks for the report / patch.

-- 
Tomas Vondra

Re: Jargon and acronyms on this mailing list

2024-09-11 Thread Robert Haas

On Mon, Sep 9, 2024 at 3:54 PM Andrew Dunstan  wrote:
> There are some serious obstacles to changing it all over, though. I
> don't want to rewrite all the history, for example.

Because of the way git works, that really wouldn't be an issue. We'd
just push the tip of the master branch to main and then start
committing to main and delete master. The history wouldn't change at
all, because in git, a branch is really just a movable pointer to a
commit. The commits themselves don't know that they're part of a
branch.

A lot of things would break, naturally. We'd still all have master
branches in our local repositories and somebody might accidentally try
to push one of those branches back to the upstream repository and the
buildfarm and lots of other tooling would get confused and it would
all be a mess for a while, but the history itself would not change.

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

Allow CI to only run the compiler warnings task

2024-09-11 Thread Bertrand Drouvot

Hi hackers,

While working on a new pg_logicalinspect module ([1]), I reached a point where
all the CI tests were green except the compiler warnings one. Then, to save time
addressing the issue, I modified the .cirrus.tasks.yml file to $SUBJECT.

I think this could be useful for others too, so please find attached this tiny
patch.

Note that the patch does not add an extra "ci-task-only", but for simplicity it
it renames ci-os-only to ci-task-only.


[1]: 
https://www.postgresql.org/message-id/ZuF2Okt7aBR//bxu%40ip-10-97-1-34.eu-west-3.compute.internal

Looking forward to your feedback,

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
>From b7289ee0f909e905b0e083cd3be41910c9c6a827 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot 
Date: Wed, 11 Sep 2024 11:01:41 +
Subject: [PATCH v1] Allow CI to only run the compiler warnings task

That could be useful to only run the CI compiler warnings task when addressing
compiler warnings issues.

Renaming ci-os-only to ci-task-only and adding filtering for the "compilerwarnings"
task.
---
 .cirrus.tasks.yml   | 16 
 src/tools/ci/README |  7 ---
 2 files changed, 12 insertions(+), 11 deletions(-)
  22.5% src/tools/ci/

diff --git a/.cirrus.tasks.yml b/.cirrus.tasks.yml
index 90cb95c868..a4e3b51045 100644
--- a/.cirrus.tasks.yml
+++ b/.cirrus.tasks.yml
@@ -59,7 +59,7 @@ task:
   # push-wait-for-ci cycle time a bit when debugging operating system specific
   # failures. Uses skip instead of only_if, as cirrus otherwise warns about
   # only_if conditions not matching.
-  skip: $CIRRUS_CHANGE_MESSAGE =~ '.*\nci-os-only:.*'
+  skip: $CIRRUS_CHANGE_MESSAGE =~ '.*\nci-task-only:.*'
 
   env:
 CPUS: 4
@@ -142,7 +142,7 @@ task:
   <<: *freebsd_task_template
 
   depends_on: SanityCheck
-  only_if: $CIRRUS_CHANGE_MESSAGE !=~ '.*\nci-os-only:.*' || $CIRRUS_CHANGE_MESSAGE =~ '.*\nci-os-only:[^\n]*freebsd.*'
+  only_if: $CIRRUS_CHANGE_MESSAGE !=~ '.*\nci-task-only:.*' || $CIRRUS_CHANGE_MESSAGE =~ '.*\nci-task-only:[^\n]*freebsd.*'
 
   sysinfo_script: |
 id
@@ -282,7 +282,7 @@ task:
   <<: *linux_task_template
 
   depends_on: SanityCheck
-  only_if: $CIRRUS_CHANGE_MESSAGE !=~ '.*\nci-os-only:.*' || $CIRRUS_CHANGE_MESSAGE =~ '.*\nci-os-only:[^\n]*linux.*'
+  only_if: $CIRRUS_CHANGE_MESSAGE !=~ '.*\nci-task-only:.*' || $CIRRUS_CHANGE_MESSAGE =~ '.*\nci-task-only:[^\n]*linux.*'
 
   ccache_cache:
 folder: ${CCACHE_DIR}
@@ -441,7 +441,7 @@ task:
   <<: *macos_task_template
 
   depends_on: SanityCheck
-  only_if: $CIRRUS_CHANGE_MESSAGE !=~ '.*\nci-os-only:.*' || $CIRRUS_CHANGE_MESSAGE =~ '.*\nci-os-only:[^\n]*(macos|darwin|osx).*'
+  only_if: $CIRRUS_CHANGE_MESSAGE !=~ '.*\nci-task-only:.*' || $CIRRUS_CHANGE_MESSAGE =~ '.*\nci-task-only:[^\n]*(macos|darwin|osx).*'
 
   sysinfo_script: |
 id
@@ -560,7 +560,7 @@ task:
   <<: *windows_task_template
 
   depends_on: SanityCheck
-  only_if: $CIRRUS_CHANGE_MESSAGE !=~ '.*\nci-os-only:.*' || $CIRRUS_CHANGE_MESSAGE =~ '.*\nci-os-only:[^\n]*windows.*'
+  only_if: $CIRRUS_CHANGE_MESSAGE !=~ '.*\nci-task-only:.*' || $CIRRUS_CHANGE_MESSAGE =~ '.*\nci-task-only:[^\n]*windows.*'
 
   setup_additional_packages_script: |
 REM choco install -y --no-progress ...
@@ -598,8 +598,8 @@ task:
   # due to resource constraints we don't run this task by default for now
   trigger_type: manual
   # worth using only_if despite being manual, otherwise this task will show up
-  # when e.g. ci-os-only: linux is used.
-  only_if: $CIRRUS_CHANGE_MESSAGE !=~ '.*\nci-os-only:.*' || $CIRRUS_CHANGE_MESSAGE =~ '.*\nci-os-only:[^\n]*mingw.*'
+  # when e.g. ci-task-only: linux is used.
+  only_if: $CIRRUS_CHANGE_MESSAGE !=~ '.*\nci-task-only:.*' || $CIRRUS_CHANGE_MESSAGE =~ '.*\nci-task-only:[^\n]*mingw.*'
   # otherwise it'll be sorted before other tasks
   depends_on: SanityCheck
 
@@ -658,7 +658,7 @@ task:
   # use always: to continue after failures. Task that did not run count as a
   # success, so we need to recheck SanityChecks's condition here ...
   depends_on: SanityCheck
-  only_if: $CIRRUS_CHANGE_MESSAGE !=~ '.*\nci-os-only:.*'
+  only_if: $CIRRUS_CHANGE_MESSAGE !=~ '.*\nci-task-only:.*' || $CIRRUS_CHANGE_MESSAGE =~ '.*\nci-task-only:[^\n]*compilerwarnings.*'
 
   env:
 CPUS: 4
diff --git a/src/tools/ci/README b/src/tools/ci/README
index 30ddd200c9..da4125defa 100644
--- a/src/tools/ci/README
+++ b/src/tools/ci/README
@@ -61,10 +61,11 @@ Controlling CI via commit messages
 The behavior of CI can be controlled by special content in commit
 messages. Currently the following controls are available:
 
-- ci-os-only: {(freebsd|linux|macos|windows|mingw)}
+- ci-task-only: {(freebsd|linux|macos|windows|mingw|compilerwarnings)}
 
-  Only runs CI on operating systems specified. This can be useful when
-  addressing portability issues affecting only a subset of platforms.
+  Only runs CI compiler warnings or on operating systems specified. This

RE: AIX support

2024-09-11 Thread Srirama Kucherlapati

Hi Heikki and Team,

Thanks for your comments.

Here are some more details

> I still don't understand. We have Linux powerpc systems
> running happily in the buildfarm. They are happy with the
> current spinlock implementation. Why is this needed?
> What happens without it?
Not sure, by the time the below commits were made if there was a consideration
to use the gcc routines. My guess is that by using this PPC assembler code
would make the code independent of the compilers. Even the Linux ppc would use 
the
same asm. Since gcc is available on AIX, I have replaced the asm changes with
the gcc routine __sync_lock_test_and_set() to set the lock.

We have the gcc(package) build on the AIX platform and as part of the testsuite
there are no issues wrt this routine. We tried to test with the sample test
program extracted from gcc testsuite. Also we discussed the same with our
compiler teams internally and they see no issues using this routine.

 gcc-12.4.0/libgomp/testsuite/libgomp.c/sections-1.c


> > +#define TAS(lock)_check_lock((slock_t *) (lock), 0, 1)
> >
> > +#define S_UNLOCK(lock)   _clear_lock((slock_t *) (lock), 0)
> >

> How is it different from __sync_lock_test_and_set()? Why is it needed?
> What is AIX kernel memory operation?

More Info: _check_lock routine description
https://www.ibm.com/docs/en/aix/7.1?topic=c-check-lock-subroutine
The _check_lock subroutine performs an atomic (uninterruptible) sequence of
operations. The compare_and_swap subroutine is similar, but does not issue
synchronization instructions and therefore is inappropriate for updating 
lock
words.

This change need not be replaced with gcc routine as, these changes will be
compiled for the non-gcc platforms only. This piece of code would never be
compiled, as we are using only gcc to build.

I tried to replace the AIX asm (under__ppc__ macro) with the gcc routine
__sync_lock_test_and_set(), and all the buildfarm tests passed. Attached patch
and the buildfarm output. Please let me know your feedback.





> On a general note: it's your responsibility to explain all the changes
> in a way that others will understand and can verify. It is especially
> important for something critical and platform-specific like spinlocks,
> because others don't have easy access to the hardware to test these
> things independently. I also want to remind you that from the Postgres
> community point of view, you are introducing support for a new platform,
> AIX, not merely resurrecting the old stuff. Every change needs to be
> justified anew.

I do agree with you. To have a better understand on the previous changes,
I was going through the history of the file(s_lock.h) and see that multiple
changes that were made wrt the tas()routine specific to ppc/AIX. Below are
the commit levels.
I would kindly request, Tom Lane, to provide some insights on these changes.


commit e3b06a871b63b90d4a08560ce184bb33324410b8
commit 50938576d482cd36e52a60b5bb1b56026e63962a << added tas() for AIX
commit 7233aae50bea503369b0a4ef9a3b6a3864c96812
commit ceb4f5ea9c2c6c2bd44d4799ff4a62c40a038894 << added tas() for PPC
commit f9ba0a7fe56398e89fe349476d9e437c3197ea28
commit eb5e4c58d137c9258eff5e41b09cb5fe4ed6d64c
commit cd35d601b859d3a56632696b8d5293cbe547764b
commit 109867748259d286dd01fce17d5f895ce59c68d5
commit 5cfa8dd3007d7e953c6a03b0fa2215d97c581b0c
commit 631beeac3598a73dee2c2afa38fa2e734148031b
commit bc2a050d40976441cdb963ad829316c23e8df0aa
commit c41a1215f04912108068b909569551f42059db29
commit 50938576d482cd36e52a60b5bb1b56026e63962a


Please let me know if would like to try on the hardware, we have recently
setup a node in the OSU lab to try out.

Thanks,
Sriram.


0001-AIX-support-revert-the-changes-from-0b16bb8776bb8.v4.patch
Description:  0001-AIX-support-revert-the-changes-from-0b16bb8776bb8.v4.patch


buildfarm17-summary.log
Description: buildfarm17-summary.log
/* Test that all sections are touched.  */

/* { dg-require-effective-target sync_int_long } */

#include 
#include 
#include 
#include "libgomp_g.h"


#define N 100
static int data[N];
static int NTHR;

static void clean_data (void)
{
  memset (data, -1, sizeof (data));
}

static void test_data (void)
{
  int i;

  for (i = 0; i < N; ++i)
assert (data[i] != -1);
}

static void set_data (unsigned i, int val)
{
  int old;
  assert (i >= 1 && i <= N);
  old = __sync_lock_test_and_set (data+i-1, val);
  assert (old == -1);
}
  

static void f_1 (void *dummy)
{
  int iam = omp_get_thread_num ();
  unsigned long s;

  for (s = GOMP_sections_start (N); s ; s = GOMP_sections_next ())
set_data (s, iam);
  GOMP_sections_end ();
}

static void test_1 (void)
{
  clean_data ();
  GOMP_parallel_start (f_1, NULL, NTHR);
  f_1 (NULL);
  GOMP_parallel_end ();
  test_data ();
}

static void f_2 (void *dummy)
{
  int iam = omp_get_thread_nu

Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible

2024-09-11 Thread Robert Haas

On Tue, Sep 10, 2024 at 4:58 PM Noah Misch  wrote:
> ... a rule of "each wait event appears in one
> pgstat_report_wait_start()" would be a rule I don't want.

As the original committer of the wait event stuff, I intended for the
rule that you do not want to be the actual rule. However, I see that I
didn't spell that out anywhere in the commit message, or the commit
itself.

> I see this level of fine-grained naming
> as making the event name a sort of stable proxy for FILE:LINE.  I'd value
> exposing such a proxy, all else being equal, but I don't think wait event
> names like AuthLdapBindLdapbinddn/AuthLdapBindUser are the right way.  Wait
> event names should be more independent of today's code-level details.

I don't agree with that. One of the most difficult parts of supporting
PostgreSQL, in my experience, is that it's often very difficult to
find out what has gone wrong when a system starts behaving badly. It
is often necessary to ask customers to install a debugger and do stuff
with it, or give them an instrumented build, in order to determine the
root cause of a problem that in some cases is not even particularly
complicated. While needing to refer to specific source code details
may not be a common experience for the typical end user, it is
extremely common for me. This problem commonly arises with error
messages, because we have lots of error messages that are exactly the
same, although thankfully it has become less common due to "could not
find tuple for THINGY %u" no longer being a message that no longer
typically reaches users. But even when someone has a complaint about
an error message and there are multiple instances of that error
message, I know that:

(1) I can ask them to set the error verbosity to verbose. I don't have
that option for wait events.

(2) The primary function of the error message is to be understandable
to the user, which means that it needs to be written in plain English.
The primary function of a wait event is to make it possible to
understand the behavior of the system and troubleshoot problems, and
it becomes much less effective as soon as it starts saying that thing
A and thing B are so similar that nobody will ever care about the
distinction. It is very hard to be certain of that. When somebody
reports that they've got a whole bunch of wait events on some wait
event that nobody has ever complained about before, I want to go look
at the code in that specific place and try to figure out what's
happening. If I have to start imagining possible scenarios based on 2
or more call sites, or if I have to start by getting them to install a
modified build with those properly split apart and trying to reproduce
the problem, it's a lot harder.

In my experience, the number of distinct wait events that a particular
installation experiences is rarely very large. It is probably measured
in dozens. A user who wishes to disregard the distinction between
similarly-named wait events won't find it prohibitively difficult to
look over the list of all the wait events they ever see and decide
which ones they'd like to merge for reporting purposes. But a user who
really needs things separated out and finds that they aren't is simply
out of luck.

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

[PATCH] Refactor SLRU to always use long file names

2024-09-11 Thread Aleksander Alekseev

Hi,

Commit 4ed8f0913bfd introduced long SLRU file names. The proposed
patch removes SlruCtl->long_segment_names flag and makes SLRU always
use long file names. This simplifies both the code and the API.
Corresponding changes to pg_upgrade are included.

One drawback I see is that technically SLRU is an exposed API and
changing it may affect third-party code. I'm not sure if we should
seriously worry about this. Firstly, the change is trivial and
secondly, it's not clear whether such third-party code even exists (we
broke this API just recently in 4ed8f0913bfd and no one complained).

I didn't include any tests for the new pg_upgrade code. To my
knowledge we test it manually, with buildfarm members and during
alpha- and beta-testing periods. Please let me know if you think there
should be a corresponding TAP test.

Thoughts?

-- 
Best regards,
Aleksander Alekseev


v1-0001-Always-use-long-SLRU-segment-file-names.patch
Description: Binary data

Re: PG_TEST_EXTRA and meson

2024-09-11 Thread Nazir Bilal Yavuz

Hi,

On Wed, 11 Sept 2024 at 13:04, Ashutosh Bapat
 wrote:
>
> Thanks Bilal for testing the patch. Can you or Jacob please create one
> patchset including both meson and make fixes? Please keep the meson
> and make changes in separate patches though. I think the meson patches
> come from [1] (they probably need a rebase, git am failed) and make
> patch comes from [2].The one fixing make needs a good commit message.

I created and attached a patchset and wrote a commit message to 'make'
fix. Please feel free to edit them.

> some comments on code
>
> 1. comments on changes to meson
>
> + variable if it exists. See  for
>
> s/exists/set/

Done.

> -# Test suites that are not safe by default but can be run if selected
> -# by the user via the whitespace-separated list in variable PG_TEST_EXTRA.
> -# Export PG_TEST_EXTRA so it can be checked in individual tap tests.
> -test_env.set('PG_TEST_EXTRA', get_option('PG_TEST_EXTRA'))
>
> A naive question. What happens if we add PG_TEST_EXTRA in meson.build
> itself rather than passing it via testwrap? E.g. like
> if "PG_TEST_EXTRA" not in os.environ
> test_env.set('PG_TEST_EXTRA', get_option('PG_TEST_EXTRA'))

Then this configure time option will be passed to the test environment
and there is no way to change it without reconfiguring if we don't
touch the testwrap file.

> I am worried that we might have to add an extra argument to testwrap
> for every environment variable that influences the tests. Avoiding it
> would be better.

If we want to have both configure time and run time variables, I
believe that this is the only way for now.

> option('PG_TEST_EXTRA', type: 'string', value: '',
> - description: 'Enable selected extra tests')
> + description: 'Enable selected extra tests, please note that this
> configure option is overridden by PG_TEST_EXTRA environment variable
> if it exists')
>
> All the descriptions look much shorter than this one. I suggest we
> shorten this one too as
> "Enable selected extra tests. Overridden by PG_TEST_EXTRA environment 
> variable."
> not as short as other descriptions but shorter than before and yet
> serves its intended purpose. Or just make it the same as the one in
> configure.ac. Either way the descriptions in configure.ac and
> meson_options.txt should be in sync.

I liked your suggestion, done in both meson_options.txt and configure.ac.

> +# If PG_TEST_EXTRA is not set in the environment, then look for its Meson
> +# configure option.
> +if "PG_TEST_EXTRA" not in os.environ and args.pg_test_extra:
> + env_dict["PG_TEST_EXTRA"] = args.pg_test_extra
> +
>
> If somebody looks at just these lines, it's not clear how
> PG_TEST_EXTRA is passed to the test environment if it's available in
> os.environ. So one has to understand that env_dict is the test
> environment. If that's so, the code and comment rewritten as below
> makes more sense to me. What do you think?
>
> # If PG_TEST_EXTRA is not already part of the test environment, check if it's
> # passed via program argument --pg_test_extra. Thus we also override
> # configuration time value by run time value of PG_TEST_EXTRA.
> if "PG_TEST_EXTRA" not in env_dict and args.pg_test_extra:
> env_dict["PG_TEST_EXTRA"] = args.pg_test_extra

I think your suggestion is better, done.

> But in case we decide to fix meson.build as suggested in one of the
> commentsabove, this change will be unnecessary.
>
> Note that PG_TEST_EXTRA is used in only TAP tests right now. The way
> the value passed to --pg_test_extra is handled in testwrap, it will
> available to every test, not just TAP tests. But this looks fine to me
> since the documentation of PG_TEST_EXTRA or its name itself does not
> show any intention to limit it only to TAP tests.

I agree, it looks fine to me as well.

> 2. comments on make changes
> Since Makefile.global.in is included in src/test/Makefile I was
> expecting that the PG_TEST_EXTRA picked from configuration would be
> available in src/test/Makefile from which it would be exported. But
> that doesn't seem to be the case. In fact, I am getting doubtful about
> the role of the current "export PG_TEST_EXTRA" in /src/test/Makefile.
> Even if I remove it, it doesn't affect anything. Commands a.
> PG_TEST_EXTRA=xid_wraparound make check, b.
> PG_TEST_EXTRA=xid_wraparound make -C $XID_MODULE_DIR check run the
> tests (and don't skip them).

Yes, it looks like it is useless. If we export PG_TEST_EXTRA, then it
should be already available on the environment, right?

> Anyway with the proposed change PG_TEST_EXTRA passed at configuration
> time is used if it's not defined at run time as expected. I think the
> patch looks good. Nothing to complain there.

--
Regards,
Nazir Bilal Yavuz
Microsoft
From 3a511b2a287b7660a0be1751fda8b52f1f5995cc Mon Sep 17 00:00:00 2001
From: Nazir Bilal Yavuz 
Date: Wed, 11 Sep 2024 15:41:27 +0300
Subject: [PATCH v4 1/2] Make PG_TEST_EXTRA env variable override its Meson
 configure option

Since PG_TEST_EXTRA configure option is set while configur

Re: Allow CI to only run the compiler warnings task

2024-09-11 Thread Nazir Bilal Yavuz

Hi,

On Wed, 11 Sept 2024 at 15:36, Bertrand Drouvot
 wrote:
>
> Hi hackers,
>
> While working on a new pg_logicalinspect module ([1]), I reached a point where
> all the CI tests were green except the compiler warnings one. Then, to save 
> time
> addressing the issue, I modified the .cirrus.tasks.yml file to $SUBJECT.
>
> I think this could be useful for others too, so please find attached this tiny
> patch.

+1 for this. I encountered the same issue before.

> Note that the patch does not add an extra "ci-task-only", but for simplicity 
> it
> it renames ci-os-only to ci-task-only.

I think this change makes sense. I gave a quick try to your patch with
ci-task-only: ["", "linux", "compilerwarnings"] and it worked as
expected.

--
Regards,
Nazir Bilal Yavuz
Microsoft

Re: Test improvements and minor code fixes for formatting.c.

Aleksander Alekseev  writes:
> I'm having difficulties applying the patch. Could you please do `git
> format-patch` and resend it?

patch(1) is generally far more forgiving than 'git am'.

regards, tom lane

Re: Document DateStyle effect on jsonpath string()

Peter Eisentraut  writes:
> What I'm concerned about is that this makes the behavior of JSON_QUERY 
> non-immutable.  Maybe there are other reasons for it to be 
> non-immutable, in which case this isn't important.  But it might be 
> worth avoiding that?

Fair point, but haven't we already bit that bullet with respect
to timezones?

[ looks... ]  Hmm, it looks like jsonb_path_exists_tz is marked
stable while jsonb_path_exists is claimed to be immutable.
So yeah, there's a problem here.  I'm not 100% convinced that
jsonb_path_exists was truly immutable before, but for sure it
is not now, and that's bad.

regression=# select jsonb_path_query('"2023-08-15 12:34:56"', 
'$.timestamp().string()');
   jsonb_path_query
---
 "2023-08-15 12:34:56"
(1 row)

regression=# set datestyle = postgres;
SET
regression=# select jsonb_path_query('"2023-08-15 12:34:56"', 
'$.timestamp().string()');
  jsonb_path_query  

 "Tue Aug 15 12:34:56 2023"
(1 row)

regards, tom lane

Re: First draft of PG 17 release notes

On Tue, Sep 10, 2024 at 08:28:42AM +0200, Jelte Fennema-Nio wrote:
> I think as an extension author there are usually three types of
> changes that are relevant:
> 1. New APIs/hooks that are meant for extension authors
> 2. Stuff that causes my existing code to not compile anymore
> 3. Stuff that changes behaviour of existing APIs code in a
> incompatible but silent way
> 
> For 1, I think adding them to the release notes makes total sense,
> especially if the new APIs are documented not only in source code, but
> also on the website. Nathan his change is of this type, so I agree
> with him it should be in the release notes.
> 
> For 2, I'll be able to easily find the PG commit that caused the
> compilation failure by grepping git history for the old API. So having
> these changes in the release notes seems unnecessary.
> 
> For 3, it would be very useful if it would be in the release notes,
> but I think in many cases it's hard to know what commits do this. So
> unless it's obviously going to break a bunch of extensions silently, I
> think we don't have to add such changes to the release notes.

So, we are looking at this commit:

commit b5a9b18cd0b
Author: Thomas Munro 
Date:   Wed Apr 3 00:17:06 2024 +1300

Provide API for streaming relation data.

Introduce an abstraction allowing relation data to be accessed as a
stream of buffers, with an implementation that is more efficient than
the equivalent sequence of ReadBuffer() calls.

Client code supplies a callback that can say which block number it wants
next, and then consumes individual buffers one at a time from the
stream.  This division puts read_stream.c in control of how far ahead it
can see and allows it to read clusters of neighboring blocks with
StartReadBuffers().  It also issues POSIX_FADV_WILLNEED advice ahead of
time when random access is detected.

Other variants of I/O stream will be proposed in future work (for
example to support recovery, whose LsnReadQueue device in
xlogprefetcher.c is a distant cousin of this code and should eventually
be replaced by this), but this basic API is sufficient for many common
executor usage patterns involving predictable access to a single fork of
a single relation.

Several patches using this API are proposed separately.

This stream concept is loosely based on ideas from Andres Freund on how
we should pave the way for later work on asynchronous I/O.

You are right that I do mention changes specifically designed for the
use of extensions, but there is no mention in the commit message of its
use for extensions.  In fact, I thought this was too low-level to be of
use for extensions.  However, if people feel it should be added, we have
enough time to add it.

I also mention changes that are _likely_ to affect extensions, but not
all changes that could affect extensions.  An interesting idea would be
to report all function signature changes in each major release in some
way.

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

  When a patient asks the doctor, "Am I going to die?", he means 
  "Am I going to die soon?"

Re: First draft of PG 17 release notes

On Tue, Sep 10, 2024 at 09:52:42AM -0700, Masahiko Sawada wrote:
> On Mon, Sep 9, 2024 at 11:29 PM Jelte Fennema-Nio  wrote:
> >
> > On Tue, 10 Sept 2024 at 04:47, Bruce Momjian  wrote:
> > > Yes.  There are so many changes at the source code level it is unwise to
> > > try and get them into the main release notes.  If someone wants to
> > > create an addendum, like was suggested for pure performance
> > > improvements, that would make sense.
> >
> > I agree that the release notes cannot fit every change. But I also
> > don't think any extension author reads the complete git commit log
> > every release, so taking the stance that they should be seems
> > unhelpful. And the "Source Code" section does exist so at some level
> > you seem to disagree with that too. So what is the way to decide that
> > something makes the cut for the "Source Code" section?
> >
> > I think as an extension author there are usually three types of
> > changes that are relevant:
> > 1. New APIs/hooks that are meant for extension authors
> > 2. Stuff that causes my existing code to not compile anymore
> > 3. Stuff that changes behaviour of existing APIs code in a
> > incompatible but silent way
> >
> > For 1, I think adding them to the release notes makes total sense,
> > especially if the new APIs are documented not only in source code, but
> > also on the website. Nathan his change is of this type, so I agree
> > with him it should be in the release notes.
> 
> +1. I think that the increment JSON parser that is already mentioned
> in the release note would fall in this type too; it's not a feature
> aimed just for extension authors, but it's kind of source and internal
> changes IMO. Since the DSM registry feature is described in the doc, I
> think it would make sense to have it in the release notes and probably
> has a link to the "Requesting Shared Memory After Startup" section.

This commit?

commit 8b2bcf3f287
Author: Nathan Bossart 
Date:   Fri Jan 19 14:24:36 2024 -0600

Introduce the dynamic shared memory registry.

Yes, we have time to add it.

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

  When a patient asks the doctor, "Am I going to die?", he means 
  "Am I going to die soon?"

Re: Detailed release notes

On Sat, Sep  7, 2024 at 10:12:00AM +0800, jian he wrote:
> On Sat, Sep 7, 2024 at 6:30 AM Jelte Fennema-Nio  wrote:
> >
> > On Thu, 22 Aug 2024 at 21:34, Marcos Pegoraro  wrote:
> > > I understand your point, and agree with that for previous releases, but 
> > > since we have a month only for version 17, will this process work 
> > > properly until that date ?
> > > I think a release notes is more read as soon as it is available than 
> > > other months, isn't it ?
> > > And this feature is just a HTML page, so if it's done manually or 
> > > automatically, from the reader point of view it'll be exactly the same.
> >
> > Big +1 to this. It would definitely be great if we would have these
> > commit links for previous release notes. But the PG17 GA release is
> > probably happening in 19 days on September 26th. I feel like we should
> > focus on getting the manual improvements from Jian He merged,
> > otherwise we'll end up with release notes for PG17 on release day that
> > are significantly less useful than they could have been. Let's not
> > make perfect the enemy of good here.
> >
> 
> hi. Thanks for your interest.
> patch updated.
> 
> 
> I have proof-read release-17.sgml again,
> making sure the commit url's commit is the same as
> release-17.sgml comment's git commit.

I will write a script to do this, but I am not sure I will have it done
by the time we release Postgres 17.

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

  When a patient asks the doctor, "Am I going to die?", he means 
  "Am I going to die soon?"

Re: First draft of PG 17 release notes

2024-09-11 Thread Nathan Bossart

On Wed, Sep 11, 2024 at 10:12:58AM -0400, Bruce Momjian wrote:
> On Tue, Sep 10, 2024 at 09:52:42AM -0700, Masahiko Sawada wrote:
>> On Mon, Sep 9, 2024 at 11:29 PM Jelte Fennema-Nio  wrote:
>> > For 1, I think adding them to the release notes makes total sense,
>> > especially if the new APIs are documented not only in source code, but
>> > also on the website. Nathan his change is of this type, so I agree
>> > with him it should be in the release notes.
>> 
>> +1. I think that the increment JSON parser that is already mentioned
>> in the release note would fall in this type too; it's not a feature
>> aimed just for extension authors, but it's kind of source and internal
>> changes IMO. Since the DSM registry feature is described in the doc, I
>> think it would make sense to have it in the release notes and probably
>> has a link to the "Requesting Shared Memory After Startup" section.
> 
> This commit?
> 
>   commit 8b2bcf3f287
>   Author: Nathan Bossart 
>   Date:   Fri Jan 19 14:24:36 2024 -0600
>   
>   Introduce the dynamic shared memory registry.
> 
> Yes, we have time to add it.

Yes, that's the one.

-- 
nathan

Re: Add on_error and log_verbosity options to file_fdw

2024-09-11 Thread Fujii Masao





On 2024/08/08 16:36, torikoshia wrote:

Attached patches.
0001 adds new option 'silent' to log_verbosity and 0002 adds on_error and 
log_verbosity options to file_fdw.


Thanks for the patches!

Here are the review comments for 0001 patch.

+  silent excludes verbose messages.

This should clarify that in silent mode, not only verbose messages but also
default ones are suppressed?

+   cstate->opts.log_verbosity != COPY_LOG_VERBOSITY_SILENT)

I think using "cstate->opts.log_verbosity >= COPY_LOG_VERBOSITY_DEFAULT" instead
might improve readability.

-   COPY_LOG_VERBOSITY_DEFAULT = 0, /* logs no additional messages, default 
*/
-   COPY_LOG_VERBOSITY_VERBOSE, /* logs additional messages */
+   COPY_LOG_VERBOSITY_SILENT = -1, /* logs none */
+   COPY_LOG_VERBOSITY_DEFAULT = 0, /* logs no additional messages, default 
*/
+   COPY_LOG_VERBOSITY_VERBOSE, /* logs additional messages */

Why do we need to assign specific numbers like -1 or 0 in this enum definition?


Here are the review comments for 0002 patch.

+   
pgstat_progress_update_param(PROGRESS_COPY_TUPLES_SKIPPED,
+   
 ++skipped);

The skipped tuple count isn't accurate because fileIterateForeignScan() resets
"skipped" to 0.

+   if (cstate->opts.on_error != COPY_ON_ERROR_STOP &&
+   cstate->escontext->error_occurred)
+   {
+   /*
+* Soft error occurred, skip this tuple and deal with 
error
+* information according to ON_ERROR.
+*/
+   if (cstate->opts.on_error == COPY_ON_ERROR_IGNORE)

If COPY_ON_ERROR_IGNORE indicates tuple skipping, shouldn’t we not only reset
error_occurred but also call "pgstat_progress_update_param" and continue within
this block?

+   for(;;)
+   {

Using "goto" here might improve readability instead of using a "for" loop.

Regards,

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

Re: Remove old RULE privilege completely

2024-09-11 Thread Fujii Masao





On 2024/09/10 4:49, Nathan Bossart wrote:

Ok, so, patch attached.

There was a test to check if has_table_privilege() accepted the keyword RULE.
The patch removed it since it's now unnecessary and would only waste cycles
testing that has_table_privilege() no longer accepts the keyword.


LGTM


Thanks for the review! Barring any objections, I'll commit the patch.

Regards,

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

Re: BUG #18598: AddressSanitizer detects use after free inside json_unique_hash_match()

2024-09-11 Thread Junwang Zhao

Hi Tomas,

On Wed, Sep 11, 2024 at 8:08 PM Tomas Vondra  wrote:
>
> On 9/10/24 21:47, Tomas Vondra wrote:
> > ...
> >
> > The only question that bothers me a little bit is the possibility of a
> > memory leak - could it happen that we keep the copied key much longer
> > than needed? Or does aggcontext have with the right life span? AFAICS
> > that's where we allocate the aggregate state, so it seems fine.
> >
> > Also, how far back do we need to backpatch this? ITSM PG15 does not have
> > this issue, and it was introduced with the SQL/JSON stuff in PG16. Is
> > that correct?
> >
>
> Nah, I spent a bit of time looking for a memory leak, but I don't think
> there's one, or at least not a new one. We use the same memory context
> as for the hash table / buffer, so that should be fine.
>
> But this made me realize the code in json_build_object_worker() can
> simply use pstrdup() to copy the key into CurrentMemoryContext, which is
> where the hash table of unique keys is. In fact, using unique_check.mcxt
> would not be quite right:
>
>MemoryContext mcxt;   /* context for saving skipped keys */
>
> And this has nothing to do with skipped keys.
>
> So I adjusted that way and pushed.
>

I didn't get the time to reply to you quickly, sorry about that.
Thank you for improving the patch and appreciate your time
for working on this.

>
>
> Thanks for the report / patch.
>
> --
> Tomas Vondra



-- 
Regards
Junwang Zhao

Re: Document DateStyle effect on jsonpath string()

On Sep 11, 2024, at 10:11, Tom Lane  wrote:

> [ looks... ]  Hmm, it looks like jsonb_path_exists_tz is marked
> stable while jsonb_path_exists is claimed to be immutable.
> So yeah, there's a problem here.  I'm not 100% convinced that
> jsonb_path_exists was truly immutable before, but for sure it
> is not now, and that's bad.
> 
> regression=# select jsonb_path_query('"2023-08-15 12:34:56"', 
> '$.timestamp().string()');
>   jsonb_path_query
> ---
> "2023-08-15 12:34:56"
> (1 row)
> 
> regression=# set datestyle = postgres;
> SET
> regression=# select jsonb_path_query('"2023-08-15 12:34:56"', 
> '$.timestamp().string()');
>  jsonb_path_query  
> 
> "Tue Aug 15 12:34:56 2023"
> (1 row)

I wonder, then, whether .string() should be modified to use the ISO format in 
UTC, and therefore be immutable. That’s the format you get if you omit 
.string() and let result be stringified from a date/time/timestamp.

FWIW, that’s how my Go port works, since I didn’t bother to replicate the 
DateStyle GUC (example[1]).

Best,

David

[1]: 
https://theory.github.io/sqljson/playground/?p=%2524.timestamp%28%29.string%28%29&j=%25222023-08-15%252012%253A34%253A56%2522&a=&o=1

Re: Jargon and acronyms on this mailing list

2024-09-11 Thread Andrew Dunstan




On 2024-09-11 We 8:34 AM, Robert Haas wrote:

On Mon, Sep 9, 2024 at 3:54 PM Andrew Dunstan  wrote:

There are some serious obstacles to changing it all over, though. I
don't want to rewrite all the history, for example.

Because of the way git works, that really wouldn't be an issue. We'd
just push the tip of the master branch to main and then start
committing to main and delete master. The history wouldn't change at
all, because in git, a branch is really just a movable pointer to a
commit. The commits themselves don't know that they're part of a
branch.

A lot of things would break, naturally. We'd still all have master
branches in our local repositories and somebody might accidentally try
to push one of those branches back to the upstream repository and the
buildfarm and lots of other tooling would get confused and it would
all be a mess for a while, but the history itself would not change.




I think you misunderstood me. I wasn't referring to the git history, but 
the buildfarm history.


Anyway, I think what I have done should suffice. You should no longer 
see the name HEAD on the buildfarm server, although it will continue to 
exists in the database.


Incidentally, I wrote a blog post about changing the client default name 
some years ago: 



I also have scripting to do the git server changes (basically to set its 
default branch), although it's rather github-specific.



cheers


andrew

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

Re: Document DateStyle effect on jsonpath string()

"David E. Wheeler"  writes:
> I wonder, then, whether .string() should be modified to use the ISO format in 
> UTC, and therefore be immutable. That’s the format you get if you omit 
> .string() and let result be stringified from a date/time/timestamp.

What "let result be stringified" behavior are you thinking of,
exactly?  AFAICS there's not sensitivity to timezone unless you
use the _tz variant, otherwise it just regurgitates the input.

I agree that we should force ISO datestyle, but I'm not quite sure
about whether we're in the clear with timezone handling.  We already
had a bunch of specialized rules about timezone handling in the _tz
and not-_tz variants of these functions.  It seems to me that simply
forcing UTC would not be consistent with that pre-existing behavior.
However, I may not have absorbed enough caffeine yet.

regards, tom lane

Re: Detailed release notes

On Wed, Sep 11, 2024 at 10:16:04AM -0400, Bruce Momjian wrote:
> On Sat, Sep  7, 2024 at 10:12:00AM +0800, jian he wrote:
> > On Sat, Sep 7, 2024 at 6:30 AM Jelte Fennema-Nio  wrote:
> > >
> > > On Thu, 22 Aug 2024 at 21:34, Marcos Pegoraro  wrote:
> > > > I understand your point, and agree with that for previous releases, but 
> > > > since we have a month only for version 17, will this process work 
> > > > properly until that date ?
> > > > I think a release notes is more read as soon as it is available than 
> > > > other months, isn't it ?
> > > > And this feature is just a HTML page, so if it's done manually or 
> > > > automatically, from the reader point of view it'll be exactly the same.
> > >
> > > Big +1 to this. It would definitely be great if we would have these
> > > commit links for previous release notes. But the PG17 GA release is
> > > probably happening in 19 days on September 26th. I feel like we should
> > > focus on getting the manual improvements from Jian He merged,
> > > otherwise we'll end up with release notes for PG17 on release day that
> > > are significantly less useful than they could have been. Let's not
> > > make perfect the enemy of good here.
> > >
> > 
> > hi. Thanks for your interest.
> > patch updated.
> > 
> > 
> > I have proof-read release-17.sgml again,
> > making sure the commit url's commit is the same as
> > release-17.sgml comment's git commit.
> 
> I will write a script to do this, but I am not sure I will have it done
> by the time we release Postgres 17.

I now see you have done the entire PG 17 release notes, so I can just
use your patch and work on a script later.  I will apply your patch
soon.

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

  When a patient asks the doctor, "Am I going to die?", he means 
  "Am I going to die soon?"

Re: Use read streams in pg_visibility

2024-09-11 Thread Noah Misch

On Wed, Sep 11, 2024 at 09:19:09AM +0300, Nazir Bilal Yavuz wrote:
> On Wed, 11 Sept 2024 at 01:38, Noah Misch  wrote:
> > I also fixed the mix of tabs and spaces inside test file string literals.
> 
> I ran both pgindent and pgperltidy but they didn't catch them. Is
> there an automated way to catch them?

git diff --check

Re: Document DateStyle effect on jsonpath string()

On Sep 11, 2024, at 11:11, Tom Lane  wrote:

> What "let result be stringified" behavior are you thinking of,
> exactly?  AFAICS there's not sensitivity to timezone unless you
> use the _tz variant, otherwise it just regurgitates the input.

There is stringification of a time, date, or timestamp value, which has no TZ, 
but is still affected by DateStyle. Then there is stringification of timetz or 
timestamptz, which can be created by the .time_tz() and .timstamp_tz() 
functions, and therefore are impacted by both the DateStyle and TimeZone 
configs, even when not using the _tz variant:

david=# set timezone = 'America/New_York';
SET
david=# select jsonb_path_query('"2023-08-15 12:34:56-09"', 
'$.timestamp_tz().string()');
 jsonb_path_query 
--
 "2023-08-15 17:34:56-04"

david=# set timezone = 'America/Los_Angeles';
SET
david=# select jsonb_path_query('"2023-08-15 12:34:56-09"', 
'$.timestamp_tz().string()');
 jsonb_path_query 
--
 "2023-08-15 14:34:56-07"
(1 row)

> I agree that we should force ISO datestyle, but I'm not quite sure
> about whether we're in the clear with timezone handling.  We already
> had a bunch of specialized rules about timezone handling in the _tz
> and not-_tz variants of these functions.  It seems to me that simply
> forcing UTC would not be consistent with that pre-existing behavior.
> However, I may not have absorbed enough caffeine yet.

True, it would not be consistent with the existing behaviors, but I believe 
these are all new in Postgres 17.

Best,

David

Re: Document DateStyle effect on jsonpath string()

"David E. Wheeler"  writes:
> On Sep 11, 2024, at 11:11, Tom Lane  wrote:
>> What "let result be stringified" behavior are you thinking of,
>> exactly?  AFAICS there's not sensitivity to timezone unless you
>> use the _tz variant, otherwise it just regurgitates the input.

> There is stringification of a time, date, or timestamp value, which
> has no TZ, but is still affected by DateStyle.

What I understood you to be referencing is what happens without
string(), which AFAICS does not result in any timezone rotation:

regression=# set timezone = 'America/New_York';
SET
regression=# select jsonb_path_query('"2023-08-15 12:34:56-09"', 
'$.timestamp_tz()');
  jsonb_path_query   
-
 "2023-08-15T12:34:56-09:00"
(1 row)

I think I'd be content to have string() duplicate that behavior
--- in fact, it seems like it'd be odd if it doesn't match.

regards, tom lane

Re: Document DateStyle effect on jsonpath string()

I wrote:
> I think I'd be content to have string() duplicate that behavior
> --- in fact, it seems like it'd be odd if it doesn't match.

Building on that thought, maybe we could fix it as attached?
This changes the just-committed test cases of course, and I did
not look at whether there are documentation changes to make.

regards, tom lane

diff --git a/src/backend/utils/adt/jsonpath_exec.c b/src/backend/utils/adt/jsonpath_exec.c
index e3ee0093d4..b9c2443b65 100644
--- a/src/backend/utils/adt/jsonpath_exec.c
+++ b/src/backend/utils/adt/jsonpath_exec.c
@@ -72,6 +72,7 @@
 #include "utils/datetime.h"
 #include "utils/float.h"
 #include "utils/formatting.h"
+#include "utils/json.h"
 #include "utils/jsonpath.h"
 #include "utils/lsyscache.h"
 #include "utils/memutils.h"
@@ -1629,32 +1630,13 @@ executeItemOptUnwrapTarget(JsonPathExecContext *cxt, JsonPathItem *jsp,
 		break;
 	case jbvDatetime:
 		{
-			switch (jb->val.datetime.typid)
-			{
-case DATEOID:
-	tmp = DatumGetCString(DirectFunctionCall1(date_out,
-			  jb->val.datetime.value));
-	break;
-case TIMEOID:
-	tmp = DatumGetCString(DirectFunctionCall1(time_out,
-			  jb->val.datetime.value));
-	break;
-case TIMETZOID:
-	tmp = DatumGetCString(DirectFunctionCall1(timetz_out,
-			  jb->val.datetime.value));
-	break;
-case TIMESTAMPOID:
-	tmp = DatumGetCString(DirectFunctionCall1(timestamp_out,
-			  jb->val.datetime.value));
-	break;
-case TIMESTAMPTZOID:
-	tmp = DatumGetCString(DirectFunctionCall1(timestamptz_out,
-			  jb->val.datetime.value));
-	break;
-default:
-	elog(ERROR, "unrecognized SQL/JSON datetime type oid: %u",
-		 jb->val.datetime.typid);
-			}
+			char		buf[MAXDATELEN + 1];
+
+			JsonEncodeDateTime(buf,
+			   jb->val.datetime.value,
+			   jb->val.datetime.typid,
+			   &jb->val.datetime.tz);
+			tmp = pstrdup(buf);
 		}
 		break;
 	case jbvNull:

Re: Invalid Assert while validating REPLICA IDENTITY?

2024-09-11 Thread vignesh C

On Wed, 11 Sept 2024 at 10:05, Amit Kapila  wrote:
>
> On Tue, Sep 10, 2024 at 2:16 PM Amit Kapila  wrote:
> >
> > On Tue, Sep 10, 2024 at 11:36 AM vignesh C  wrote:
> > >
> > > On Mon, 9 Sept 2024 at 13:12, Amit Kapila  wrote:
> > > >
> > > > The second part of the assertion is incomplete. The
> > > > IsIndexUsableForReplicaIdentityFull() should be used only when the
> > > > remote relation has REPLICA_IDENTITY_FULL set. I haven't tested all
> > > > possible cases yet but I think the attached should be a better way to
> > > > write this assertion.
> > >
> > > The changes look good to me.
> > >
> >
> > Thanks, I'll push this tomorrow unless Dilip or anyone else has any
> > comments. BTW, as the current code doesn't lead to any bug or
> > assertion failure, I am planning to push this change to HEAD only, let
> > me know if you think otherwise.
> >
>
> Pushed.

I noticed that Drongo failed the subscription-check for the 015_stream
test, as reported at [1].

In publisher log I could see the following statement executed
015_stream.pl line 253:
$node_publisher->safe_psql('postgres', "TRUNCATE TABLE test_tab_2");

With the following log content from 015_stream_publisher.log:
2024-09-11 09:13:36.886 UTC [512:4] 015_stream.pl LOG:  statement:
TRUNCATE TABLE test_tab_2

and lastly executed wait_for_catchup 015_stream.pl line 254:
2024-09-11 09:13:38.811 UTC [4320:4] 015_stream.pl LOG:  statement:
SELECT '0/20E3BC8' <= replay_lsn AND state = 'streaming'
 FROM pg_catalog.pg_stat_replication
 WHERE application_name IN ('tap_sub', 'walreceiver')

In subscriber log I could see the following statement executed
015_stream.pl line 255:
$node_subscriber->safe_psql('postgres',
"CREATE UNIQUE INDEX idx_tab on test_tab_2(a)");

With the following log content from 015_stream_subscriber.log:
2024-09-11 09:13:39.610 UTC [3096:4] 015_stream.pl LOG:  statement:
CREATE UNIQUE INDEX idx_tab on test_tab_2(a)

However, there is no clear indication of what transpired afterward or
why the remaining statements were not executed. It appears this issue
is unrelated to the recent patch, as the changes introduced by the
patch affect only update and delete operations.
I will keep monitoring the buildfarm for additional runs to determine
if the issue recurs and draw conclusions based on the observations.

[1] - 
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=drongo&dt=2024-09-11%2007%3A24%3A53

Regards,
Vignesh

Re: Document DateStyle effect on jsonpath string()

On Sep 11, 2024, at 12:26, Tom Lane  wrote:

> Building on that thought, maybe we could fix it as attached?
> This changes the just-committed test cases of course, and I did
> not look at whether there are documentation changes to make.

It looks like that’s what datum_to_json_internal() in json.c does, which IIUC 
is the default stringification for date and time values.

David

Re: First draft of PG 17 release notes

2024-09-11 Thread Alvaro Herrera

Hello,

I noticed that these two items in the current notes are separate:



 
  
  Allow specification of partitioned table
  access methods (Justin Pryzby, Soumyadeep Chakraborty,
  Michael Paquier)
  
 



 
  
  Add DEFAULT setting for ALTER TABLE
  .. SET ACCESS METHOD (Michael Paquier)
  
 

They are very very closely related, so I suggest they should be
together as a single item.  Also, the first one is somewhat strangely
worded IMO (we don't have "partitioned table access methods" -- rather,
we have table access methods for partitioned tables).  Maybe something
like

* Improve ALTER TABLE ... SET ACCESS METHOD

  This command can now also be applied to partitioned tables, so that it
  can https://www.postgresql.org/docs/17/sql-altertable.html#SQL-ALTERTABLE-DESC-SET-ACCESS-METHOD";>
  influence partitions created later. (Justin, Soumyadeep, Michaël)

  In addition, it now accepts the value DEFAULT to reset a previously
  set value. (Michaël)


There's also a bunch of items on EXPLAIN, which could perhaps be grouped
in a single item with sub-paras for each individual change; I'd also
move it to the bottom of E.1.3.2.

-- 
Álvaro HerreraBreisgau, Deutschland  —  https://www.EnterpriseDB.com/
"This is a foot just waiting to be shot"(Andrew Dunstan)

Re: Document DateStyle effect on jsonpath string()

"David E. Wheeler"  writes:
> On Sep 11, 2024, at 12:26, Tom Lane  wrote:
>> Building on that thought, maybe we could fix it as attached?

> It looks like that’s what datum_to_json_internal() in json.c does, which IIUC 
> is the default stringification for date and time values.

Right.  I actually lifted the code from convertJsonbScalar in
jsonb_util.c.

Here's a more fleshed-out patch with docs and regression test
fixes.  I figured we could shorten the tests a bit now that
the point is just to verify that datestyle *doesn't* affect it.

regards, tom lane

diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 1bde4091ca..aa1ac2c4fe 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -18017,16 +18017,15 @@ ERROR:  jsonpath member accessor can only be applied to an object


 String value converted from a JSON boolean, number, string, or
-datetime (the output format for datetimes is determined by
-the  parameter)
+datetime


 jsonb_path_query_array('[1.23, "xyz", false]', '$[*].string()')
 ["1.23", "xyz", "false"]


-jsonb_path_query('"2023-08-15"', '$.datetime().string()')
-"2023-08-15"
+jsonb_path_query('"2023-08-15 12:34:56"', '$.timestamp().string()')
+"2023-08-15T12:34:56"

   
 
diff --git a/src/backend/utils/adt/jsonpath_exec.c b/src/backend/utils/adt/jsonpath_exec.c
index e3ee0093d4..b9c2443b65 100644
--- a/src/backend/utils/adt/jsonpath_exec.c
+++ b/src/backend/utils/adt/jsonpath_exec.c
@@ -72,6 +72,7 @@
 #include "utils/datetime.h"
 #include "utils/float.h"
 #include "utils/formatting.h"
+#include "utils/json.h"
 #include "utils/jsonpath.h"
 #include "utils/lsyscache.h"
 #include "utils/memutils.h"
@@ -1629,32 +1630,13 @@ executeItemOptUnwrapTarget(JsonPathExecContext *cxt, JsonPathItem *jsp,
 		break;
 	case jbvDatetime:
 		{
-			switch (jb->val.datetime.typid)
-			{
-case DATEOID:
-	tmp = DatumGetCString(DirectFunctionCall1(date_out,
-			  jb->val.datetime.value));
-	break;
-case TIMEOID:
-	tmp = DatumGetCString(DirectFunctionCall1(time_out,
-			  jb->val.datetime.value));
-	break;
-case TIMETZOID:
-	tmp = DatumGetCString(DirectFunctionCall1(timetz_out,
-			  jb->val.datetime.value));
-	break;
-case TIMESTAMPOID:
-	tmp = DatumGetCString(DirectFunctionCall1(timestamp_out,
-			  jb->val.datetime.value));
-	break;
-case TIMESTAMPTZOID:
-	tmp = DatumGetCString(DirectFunctionCall1(timestamptz_out,
-			  jb->val.datetime.value));
-	break;
-default:
-	elog(ERROR, "unrecognized SQL/JSON datetime type oid: %u",
-		 jb->val.datetime.typid);
-			}
+			char		buf[MAXDATELEN + 1];
+
+			JsonEncodeDateTime(buf,
+			   jb->val.datetime.value,
+			   jb->val.datetime.typid,
+			   &jb->val.datetime.tz);
+			tmp = pstrdup(buf);
 		}
 		break;
 	case jbvNull:
diff --git a/src/test/regress/expected/jsonb_jsonpath.out b/src/test/regress/expected/jsonb_jsonpath.out
index 70eeb655a2..acdf7e436f 100644
--- a/src/test/regress/expected/jsonb_jsonpath.out
+++ b/src/test/regress/expected/jsonb_jsonpath.out
@@ -2652,30 +2652,30 @@ select jsonb_path_query('"2023-08-15 12:34:56 +5:30"', '$.timestamp().string()')
 ERROR:  cannot convert value from timestamptz to timestamp without time zone usage
 HINT:  Use *_tz() function for time zone support.
 select jsonb_path_query_tz('"2023-08-15 12:34:56 +5:30"', '$.timestamp().string()'); -- should work
-jsonb_path_query_tz 
-
- "Tue Aug 15 00:04:56 2023"
+  jsonb_path_query_tz  
+---
+ "2023-08-15T00:04:56"
 (1 row)
 
 select jsonb_path_query('"2023-08-15 12:34:56"', '$.timestamp_tz().string()');
 ERROR:  cannot convert value from timestamp to timestamptz without time zone usage
 HINT:  Use *_tz() function for time zone support.
 select jsonb_path_query_tz('"2023-08-15 12:34:56"', '$.timestamp_tz().string()'); -- should work
-  jsonb_path_query_tz   
-
- "Tue Aug 15 12:34:56 2023 PDT"
+ jsonb_path_query_tz 
+-
+ "2023-08-15T12:34:56-07:00"
 (1 row)
 
 select jsonb_path_query('"2023-08-15 12:34:56 +5:30"', '$.timestamp_tz().string()');
-jsonb_path_query
-
- "Tue Aug 15 00:04:56 2023 PDT"
+  jsonb_path_query   
+-
+ "2023-08-15T12:34:56+05:30"
 (1 row)
 
 select jsonb_path_query('"2023-08-15 12:34:56"', '$.timestamp().string()');
-  jsonb_path_query  
-
- "Tue Aug 15 12:34:56 2023"
+   jsonb_path_query
+---
+ "2023-08-15T12:34:56"
 (1 row

Re: libpq: Process buffered SSL read bytes to support records >8kB on async API

2024-09-11 Thread Lars Kanis


Thank you Jacob for verifying this issue!


Gory details of the packet sizes, if it's helpful:
- max TLS record size is 12k, because it made the math easier
- server sends DataRow of 32006 bytes, followed by DataRow of 806
bytes, followed by CommandComplete/ReadyForQuery
- so there are three TLS records on the wire containing
   1) DataRow 1 fragment 1 (12k bytes)
   2) DataRow 1 fragment 2 (12k bytes)
   3) DataRow 1 fragment 3 (7430 bytes) + DataRow 2 (806 bytes)
+ CommandComplete + ReadyForQuery


How did you verify the issue on the server side - with YugabyteDB or 
with a modified Postgres server? I'd like to verify the GSSAPI part and 
I'm familiar with the Postgres server only.




I agree that PQconsumeInput() needs to ensure that the transport
buffers are all drained. But I'm not sure this is a complete solution;
doesn't GSS have the same problem? And are there any other sites that
need to make the same guarantee before returning?


Which other sites do you mean? The synchronous transfer already works, 
since the select() is short-circuit in case of pending bytes: [1]




I need to switch away from this for a bit. Would you mind adding this
to the next Commitfest as a placeholder?


No problem; registered: https://commitfest.postgresql.org/50/5251/

--

Regards, Lars

[1] 
https://github.com/postgres/postgres/blob/77761ee5dddc0518235a51c533893e81e5f375b9/src/interfaces/libpq/fe-misc.c#L1070

Re: Document DateStyle effect on jsonpath string()

On Sep 11, 2024, at 15:08, Tom Lane  wrote:

> Right.  I actually lifted the code from convertJsonbScalar in
> jsonb_util.c.
> 
> Here's a more fleshed-out patch with docs and regression test
> fixes.  I figured we could shorten the tests a bit now that
> the point is just to verify that datestyle *doesn't* affect it.

Looks good. Although…

Should it use the database-native stringification standard or the jsonpath 
stringification standard? In the case of the former, output should omit the “T” 
time separator and simplify the time zone `07:00` to `07`. But if it’s the 
latter case, then it’s good as is.

Best,

David

Re: not null constraints, again

2024-09-11 Thread Alvaro Herrera

On 2024-Sep-11, jian he wrote:

> On Wed, Sep 11, 2024 at 2:18 AM Alvaro Herrera  
> wrote:
> >
> > Hello, here's a v2 of this patch.  I have fixed --I think-- all the
> > issues you and Tender Wang reported (unless I declined a fix in some
> > previous email).
> >
> 
> + /*
> + * The constraint must appear as inherited in children, so create a
> + * modified constraint object to use.
> + */
> + constr = copyObject(constr);
> + constr->inhcount = 1;
> 
> in ATAddCheckNNConstraint, we don't need the above copyObject call.
> because at the beginning of ATAddCheckNNConstraint, we do
> newcons = AddRelationNewConstraints(rel, NIL,
> list_make1(copyObject(constr)),
> recursing || is_readd,/*
> allow_merge */
> !recursing, /* is_local */
> is_readd,/* is_internal */
> NULL);/* queryString not available
>  * here */

I'm disinclined to change this.  The purpose of creating a copy at this
point is to avoid modifying an object that doesn't belong to us.  It
doesn't really matter much that we have an additional copy anyway; this
isn't in any way performance-critical or memory-intensive.

> create table idxpart (a int) partition by range (a);
> create table idxpart0 (like idxpart);
> alter table idxpart0 add primary key (a);
> alter table idxpart attach partition idxpart0 for values from (0) to (1000);
> alter table idxpart add primary key (a);
> 
> alter table idxpart0 DROP CONSTRAINT idxpart0_pkey;
> alter table idxpart0 DROP CONSTRAINT idxpart0_a_not_null;
> 
> First DROP CONSTRAINT failed as the doc said,
> but the second success.
> but the second DROP CONSTRAINT should fail?
> Even if you drop success, idxpart0_a_not_null still exists.
> it also conflicts with the pg_constraint I've quoted above.

Hmm, this is because we allow a DROP CONSTRAINT to set conislocal from
true to false.  So the constraint isn't *actually* dropped.  If you try
the DROP CONSTRAINT a second time, you'll get an error then.  Maybe I
should change the order of checks here, so that we forbid doing the
conislocal change; that would be more consistent with what we document.
I'm undecided about this TBH -- maybe I should reword the documentation
you cite in a different way.

> transformTableLikeClause, expandTableLikeClause
> can be further simplified when the relation don't have not-null as all like:
> 
> /*
>  * Reproduce not-null constraints by copying them.  This doesn't require
>  * any option to have been given.
>  */
> if (tupleDesc->constr && tupleDesc->constr->has_not_null)
> {
> lst = RelationGetNotNullConstraints(RelationGetRelid(relation), 
> false);
> cxt->nnconstraints = list_concat(cxt->nnconstraints, lst);
> }

True.

> Also, seems AdjustNotNullInheritance never being called/used?

Oh, right, I removed the last callsite recently.  I'll remove the
function, and rename AdjustNotNullInheritance1 to
AdjustNotNullInheritance now that that name is free.

Thanks for reviewing!  I'll handle your other comment tomorrow.

-- 
Álvaro HerreraBreisgau, Deutschland  —  https://www.EnterpriseDB.com/

Re: Document DateStyle effect on jsonpath string()

"David E. Wheeler"  writes:
> Should it use the database-native stringification standard or the jsonpath 
> stringification standard? In the case of the former, output should omit the 
> “T” time separator and simplify the time zone `07:00` to `07`. But if it’s 
> the latter case, then it’s good as is.

Seems to me it should be the jsonpath convention.  If the spec
does require any specific spelling, surely it must be that one.

regards, tom lane

Accept invalidation messages before the query starts inside a transaction

2024-09-11 Thread Andrei Lepikhov


Hi,

During the PGDAY.Spain'2024 Yurii Rashkovskii found out boring behaviour 
related to functions versioning:
If you open a transaction in one session and execute the function, then 
replace this function with a new version or drop it in a concurrent 
session; the first session doesn't see any changes: it will use the old 
version of the function in the following query.
If you execute the function without explicit transaction, subsequent 
query execution will employ the new version of the function.


It happens because of documented behaviour [1], which doesn't guarantee 
isolation levels for internal access to the system catalogues. But it 
looks more consistent for a user to see changes the same way with and 
without explicit transactions, doesn't it? At least, an extension 
developer may be confident that after updating to a new version, no one 
user will employ the old version of the extension's function and not 
think about the consequences it may cause.


I don't know whether to classify this as a bug. The sketch of the patch 
with an example isolation test is attached.


[1] https://www.postgresql.org/docs/16/mvcc-caveats.html

--
regards, Andrei LepikhovFrom 67b15acb4db396a24deab0efefb602227050ed63 Mon Sep 17 00:00:00 2001
From: "Andrei V. Lepikhov" 
Date: Wed, 11 Sep 2024 21:16:59 +0200
Subject: [PATCH] Accept invalidation messages before the start of a query in
 transaction

---
 src/backend/tcop/postgres.c   |  3 +++
 .../isolation/expected/function-replace.out   | 20 ++
 src/test/isolation/isolation_schedule |  1 +
 .../isolation/specs/function-replace.spec | 26 +++
 4 files changed, 50 insertions(+)
 create mode 100644 src/test/isolation/expected/function-replace.out
 create mode 100644 src/test/isolation/specs/function-replace.spec

diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 8bc6bea113..4ed8d6f786 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -73,6 +73,7 @@
 #include "tcop/utility.h"
 #include "utils/guc_hooks.h"
 #include "utils/injection_point.h"
+#include "utils/inval.h"
 #include "utils/lsyscache.h"
 #include "utils/memutils.h"
 #include "utils/ps_status.h"
@@ -2774,6 +2775,8 @@ start_xact_command(void)
!get_timeout_active(CLIENT_CONNECTION_CHECK_TIMEOUT))
enable_timeout_after(CLIENT_CONNECTION_CHECK_TIMEOUT,
 
client_connection_check_interval);
+
+   AcceptInvalidationMessages();
 }
 
 static void
diff --git a/src/test/isolation/expected/function-replace.out 
b/src/test/isolation/expected/function-replace.out
new file mode 100644
index 00..b1393be4bf
--- /dev/null
+++ b/src/test/isolation/expected/function-replace.out
@@ -0,0 +1,20 @@
+Parsed test spec with 2 sessions
+
+starting permutation: s1_exec_fn s2_replace s1_exec_fn s2_drop s1_exec_fn 
s1_end
+step s1_exec_fn: SELECT test_fn();
+test_fn
+---
+  1
+(1 row)
+
+step s2_replace: CREATE OR REPLACE FUNCTION test_fn() RETURNS integer LANGUAGE 
sql AS $$ SELECT 2; $$;
+step s1_exec_fn: SELECT test_fn();
+test_fn
+---
+  2
+(1 row)
+
+step s2_drop: DROP FUNCTION test_fn;
+step s1_exec_fn: SELECT test_fn();
+ERROR:  function test_fn() does not exist
+step s1_end: END;
diff --git a/src/test/isolation/isolation_schedule 
b/src/test/isolation/isolation_schedule
index 143109aa4d..91a8ad67a6 100644
--- a/src/test/isolation/isolation_schedule
+++ b/src/test/isolation/isolation_schedule
@@ -115,3 +115,4 @@ test: serializable-parallel-2
 test: serializable-parallel-3
 test: matview-write-skew
 test: lock-nowait
+test: function-replace
diff --git a/src/test/isolation/specs/function-replace.spec 
b/src/test/isolation/specs/function-replace.spec
new file mode 100644
index 00..54ce156d29
--- /dev/null
+++ b/src/test/isolation/specs/function-replace.spec
@@ -0,0 +1,26 @@
+#
+# Accept invalidation messages before the start of a new query inside the
+# transaction
+#
+
+setup
+{
+  CREATE FUNCTION test_fn() RETURNS integer LANGUAGE sql AS $$ SELECT 1; $$;
+}
+
+session s1
+setup { BEGIN; }
+step s1_exec_fn { SELECT test_fn(); }
+step s1_end { END; }
+
+session s2
+step s2_drop { DROP FUNCTION test_fn; }
+step s2_replace { CREATE OR REPLACE FUNCTION test_fn() RETURNS integer 
LANGUAGE sql AS $$ SELECT 2; $$; }
+
+permutation
+  s1_exec_fn
+  s2_replace
+  s1_exec_fn
+  s2_drop
+  s1_exec_fn
+  s1_end
\ No newline at end of file
-- 
2.46.0

Re: Document DateStyle effect on jsonpath string()

On Sep 11, 2024, at 15:43, Tom Lane  wrote:

> Seems to me it should be the jsonpath convention.  If the spec
> does require any specific spelling, surely it must be that one.

WFM, though now I’ll have to go change my port 😂.

D

Re: Accept invalidation messages before the query starts inside a transaction

Andrei Lepikhov  writes:
> I don't know whether to classify this as a bug. The sketch of the patch 
> with an example isolation test is attached.

This seems like an extremely strange place (and an extremely
brute-force way) to insert an AcceptInvalidationMessages call.
Under what circumstances wouldn't we do one or more AIMs later
on, eg while acquiring locks?

regards, tom lane

Re: Pgstattuple on Sequences: Seeking Community Feedback on Potential Patch

2024-09-11 Thread Nathan Bossart

Barring objections, I'm planning to commit v3 soon.  Robert/Matthias, I'm
not sure you are convinced this is the right thing to do (or worth doing,
rather), but I don't sense that you are actually opposed to it, either.
Please correct me if I am wrong.

-- 
nathan

Re: Extract numeric filed in JSONB more effectively

On Wed, 17 Apr 2024 at 17:17, Andy Fan  wrote:
> rebase to the latest master again.

There's a lot of complexity in the v18 patch that I don't understand
the need for.

I imagined you'd the patch should create a SupportRequestSimplify
support function for jsonb_numeric() that checks if the input
expression is an OpExpr with funcid of jsonb_object_field().  All you
do then is ditch the cast and change the OpExpr to call a new function
named jsonb_object_field_numeric() which returns the val.numeric
directly.  Likely the same support function could handle jsonb casts
to other types too, in which case you'd just call some other function,
e.g jsonb_object_field_timestamp() or jsonb_object_field_boolean().

Can you explain why the additional complexity is needed over what's in
the attached patch?

David
diff --git a/src/backend/utils/adt/jsonb.c b/src/backend/utils/adt/jsonb.c
index 928552d551..7b60b36189 100644
--- a/src/backend/utils/adt/jsonb.c
+++ b/src/backend/utils/adt/jsonb.c
@@ -18,7 +18,9 @@
 #include "funcapi.h"
 #include "libpq/pqformat.h"
 #include "miscadmin.h"
+#include "nodes/supportnodes.h"
 #include "utils/builtins.h"
+#include "utils/fmgroids.h"
 #include "utils/json.h"
 #include "utils/jsonb.h"
 #include "utils/jsonfuncs.h"
@@ -2069,6 +2071,64 @@ jsonb_numeric(PG_FUNCTION_ARGS)
PG_RETURN_NUMERIC(retValue);
 }
 
+Datum
+jsonb_object_field_numeric(PG_FUNCTION_ARGS)
+{
+   Jsonb *jb = PG_GETARG_JSONB_P(0);
+   text *key = PG_GETARG_TEXT_PP(1);
+   JsonbValue *v;
+   JsonbValue vbuf;
+
+   if (!JB_ROOT_IS_OBJECT(jb))
+   PG_RETURN_NULL();
+
+   v = getKeyJsonValueFromContainer(&jb->root,
+
VARDATA_ANY(key),
+
VARSIZE_ANY_EXHDR(key),
+&vbuf);
+
+   if (v != NULL)
+   {
+   if (v->type == jbvNumeric)
+   PG_RETURN_NUMERIC(v->val.numeric);
+
+   cannotCastJsonbValue(v->type, "numeric");
+   }
+
+   PG_RETURN_NULL();
+}
+
+Datum
+jsonb_numeric_support(PG_FUNCTION_ARGS)
+{
+   Node   *rawreq = (Node *) PG_GETARG_POINTER(0);
+   Node *ret = NULL;
+
+   if (IsA(rawreq, SupportRequestSimplify))
+   {
+   SupportRequestSimplify *req = (SupportRequestSimplify *) rawreq;
+   FuncExpr *func = req->fcall;
+   OpExpr *opexpr = list_nth(func->args, 0);
+
+   /*
+* Transform jsonb_object_field calls that directly cast to 
numeric
+* into a direct call to jsonb_object_field_numeric.  This 
allows us
+* to directly access the numeric field and return it directly 
thus
+* saving casting from jsonb to numeric.
+*/
+   if (IsA(opexpr, OpExpr) && opexpr->opfuncid == 
F_JSONB_OBJECT_FIELD)
+   {
+   opexpr->opfuncid = F_JSONB_OBJECT_FIELD_NUMERIC;
+   PG_RETURN_POINTER(opexpr);
+   }
+
+   PG_RETURN_POINTER(ret);
+   }
+
+   PG_RETURN_POINTER(ret);
+}
+
+
 Datum
 jsonb_int2(PG_FUNCTION_ARGS)
 {
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index ff5436acac..10aed0b0b1 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -4642,8 +4642,12 @@
   proname => 'bool', prorettype => 'bool', proargtypes => 'jsonb',
   prosrc => 'jsonb_bool' },
 { oid => '3449', descr => 'convert jsonb to numeric',
-  proname => 'numeric', prorettype => 'numeric', proargtypes => 'jsonb',
+  proname => 'numeric', prosupport => 'jsonb_numeric_support',
+  prorettype => 'numeric', proargtypes => 'jsonb',
   prosrc => 'jsonb_numeric' },
+{ oid => '8394', descr => 'planner support for jsonb casting support',
+  proname => 'jsonb_numeric_support',  prorettype => 'internal',
+  proargtypes => 'internal', prosrc => 'jsonb_numeric_support' },
 { oid => '3450', descr => 'convert jsonb to int2',
   proname => 'int2', prorettype => 'int2', proargtypes => 'jsonb',
   prosrc => 'jsonb_int2' },
@@ -10083,6 +10087,10 @@
   proargtypes => 'jsonb _text', proallargtypes => '{jsonb,_text}',
   proargmodes => '{i,v}', proargnames => '{from_json,path_elems}',
   prosrc => 'jsonb_extract_path' },
+{ oid => '8395',
+  proname => 'jsonb_object_field_numeric', prorettype => 'numeric',
+  proargtypes => 'jsonb text', proargnames => '{from_json, field_name}',
+  prosrc => 'jsonb_object_field_numeric' },
 { oid => '3940', descr => 'get value from jsonb as text with path elements',
   proname => 'jsonb_extract_path_text', provariadic => 'text',
   prorettype => 'text', proargtypes => 'jsonb _text',

Re: Support run-time partition pruning for hash join

On Fri, 6 Sept 2024 at 19:19, Richard Guo  wrote:
> * Currently, the costing code does not take run-time pruning into
> consideration.  How should we calculate the costs of the parameterized
> paths on partitioned tables?

Couldn't we assume total_cost = total_cost / n_apppend_children for
equality conditions and do something with DEFAULT_INEQ_SEL and
DEFAULT_RANGE_INEQ_SEL for more complex cases.  I understand we
probably need to do something about this to have the planner have any
chance of actually choose these Paths, so hacking something in there
to test the idea is sound before going to the trouble of refining the
cost model seems like a good idea.

> * This approach generates additional paths at the scan level, which
> may not be easily compared with regular scan paths.  As a result, we
> might need to retain these paths at every level of the join tree.  I'm
> afraid this could lead to a significant increase in planning time in
> some cases.  We need to find a way to avoid regressions in planning
> time.

How about just creating these Paths for partitioned tables (and
partitions) when there's an EquivalenceClass containing multiple
relids on the partition key?  I think those are about the only cases
that could benefit, so I think it makes sense to restrict making the
additional Paths for that case.

David

Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible

2024-09-11 Thread Jacob Champion

On Mon, Sep 9, 2024 at 10:30 PM Michael Paquier  wrote:
> No.  My question was about splitting pgstat_bestart() and
> pgstat_bestart_pre_auth() in a cleaner way, because authenticated
> connections finish by calling both, meaning that we do twice the same
> setup for backend entries depending on the authentication path taken.
> That seems like a waste.

I can try to separate them out. I'm a little wary of messing with the
CRITICAL_SECTION guarantees, though. I thought the idea was that you
filled in the entire struct to prevent tearing. (If I've misunderstood
that, please let me know :D)

> Perhaps just use a new
> "Authentication" class, as in "The server is waiting for an
> authentication operation to complete"?

Sounds good.

> Couldn't it be better to have a one-one mapping
> instead, adding twelve entries in wait_event_names.txt?

(I have no strong opinions on this myself, but while the debate is
ongoing, I'll work on a version of the patch with more detailed wait
events. It's easy to collapse them again if that gets the most votes.)

> I am not really on board with the test based on injection points
> proposed, though.  It checks that the "authenticating" flag is set in
> pg_stat_activity, but it does nothing else.  That seems limited.  Or
> are you planning for more?

I can test for specific contents of the entry, if you'd like. My
primary goal was to test that an entry shows up if that part of the
code hangs. I think a regression would otherwise go completely
unnoticed.

Thanks!
--Jacob

Re: AIX support

2024-09-11 Thread Heikki Linnakangas


On 11/09/2024 15:38, Srirama Kucherlapati wrote:

I still don't understand. We have Linux powerpc systems running
happily in the buildfarm. They are happy with the current spinlock
implementation. Why is this needed? What happens without it?


Not sure, by the time the below commits were made if there was a 
consideration to use the gcc routines.


The PPC asm code was originally written in 2002, and the first use of 
__sync_lock_test_and_set(), for ARM, appeared in 2012. The commit that 
made __sync_lock_test_and_set() be chosen automatically for platforms 
that don't specify anything else was added in 2022.



I tried to replace the AIX asm (under__ppc__ macro) with the gcc
routine __sync_lock_test_and_set(), and all the buildfarm tests
passed. Attached patch and the buildfarm output. Please let me know
your feedback.
Ok, if we don't need the assembler code at all, that's good. A patch to 
introduce AIX support should not change it for non-AIX powerpc systems 
though. That might be a good change, but would need to be justified 
separately, e.g.  by some performance testing, and should be a separate 
patch.


If you make no changes to s_lock.h at all, will it work? Why not?

You said earlier:


I mean this part of the code is needed as this is specific to AIX kernel memory
operation which is different from __sync_lock_test_and_set().

I would like to mention that the changes made in src/include/storage/s_lock.h
are pretty much required and need to be operated in assemble specific to IBM
Power architecture.


Was that earlier statement incorrect? Is the manual wrong or outdated or 
not applicable to us?



Moving on..

Do you still need mkldexport.sh? Surely there's a better way to do that 
in year 2024. Some quick googling says there's a '-bexpall' option to 
'ld', which kind of sounds like what we want. Will that work? How do 
other programs do this?


--
Heikki Linnakangas
Neon (https://neon.tech)

Re: query_id, pg_stat_activity, extended query protocol

2024-09-11 Thread Sami Imseih

I took a look at your patches and here are my comments

> 1) ExecutorRun() misses the reports, which happens when a query
> does an ExecutorStart(), then a series of ExecutorRun() through a
> portal with bind messages.  Robert has mentioned that separately a few
> days ago at [1].  But that's not everything.
> 2) A query executed through a portal with tuples to return in a
> tuplestore also miss the query ID report.  For example, a DML
> RETURNING with the extended protocol would use an execute (with
> ExecutorStart and ExecutorRun) followed by a series of execute fetch.
> pg_stat_activity would report the query ID for the execute, not for
> the fetches, while pg_stat_activity has the query string.  That's
> confusing.

1/ 
In your 0003-Report-query-ID-for-execute-fetch-in-extended-query-.patch 
patch, you are still setting the queryId inside exec_execute_message 
if (execute_is_fetch). This condition could be removed and don't need to set 
the queryId inside ExecutorRun. This is exactly what v5-0001 does. 

V5-0001 also sets the queryId inside the exec_bind_message.
We must do that otherwise we will have a NULL queryId during bind.

also tested it against this for the case that was raised by Robert [1].

I also think we need to handle RevalidateCachedQuery. This is the case where we 
have a new queryId after a cached query revalidation. 

I addressed the comments by Andrei [3] in v5-0002. For RevalidateCachedQuery, 
we can simple call pgstat_report_query_id with "force" = "false" so it will 
take care 
of updating  a queryId only if it's a top level query. 

2/ 
As far as 0002-Add-sanity-checks-related-to-query-ID-reporting-in-p.patch, 
I do like the pg_stat_statements extended tests to perform these tests. 

What about adding the Assert(pgstat_get_my_query_id() != 0) inside 
exec_parse_message, exec_bind_message and exec_execute_message as well?

I think having the Asserts inside the hooks in pg_stat_statements are good
as well.

I am not sure how we can add tests for RevalidateCachedQuery though using
pg_stat_statements. We could skip testing this scenario, maybe??

Let me know what you think.


[1] 
https://www.postgresql.org/message-id/CA+TgmoZxtnf_jZ=vqbsyau8hfukkwojcj6ufy4lgpxaunkr...@mail.gmail.com
[2] 
https://www.postgresql.org/message-id/flat/2beb1a00-3060-453a-90a6-7990d6940d62%40gmail.com#fffec59b563dbf49910e8b6d9f855e5a
[3] 
https://www.postgresql.org/message-id/F001F959-400F-41C6-9886-C9665A4DE0A3%40gmail.com


Regards,

Sami 




v5-0001-Add-missing-queryId-reporting-in-extended-query.patch
Description: Binary data


v5-0002-Report-new-queryId-after-plancache-re-validation.patch
Description: Binary data

Re: Pgstattuple on Sequences: Seeking Community Feedback on Potential Patch

2024-09-11 Thread Matthias van de Meent

On Wed, 11 Sept 2024 at 21:36, Nathan Bossart  wrote:
>
> Barring objections, I'm planning to commit v3 soon.  Robert/Matthias, I'm
> not sure you are convinced this is the right thing to do (or worth doing,
> rather), but I don't sense that you are actually opposed to it, either.
> Please correct me if I am wrong.

Correct: I do think making heapam-related inspection functions have a
consistent behaviour when applied to sequences is beneficial, with no
preference towards specifically supporting or not supporting sequences
in these functions. If people that work on sequences think it's better
to also support inspection of sequences, then I think that's a good
reason to add that support where it doesn't already exist.

As for patch v3, that seems fine with me.

Matthias van de Meent
Neon (https://neon.tech)

Re: AIX support

2024-09-11 Thread Thomas Munro

On Thu, Sep 12, 2024 at 9:57 AM Heikki Linnakangas  wrote:
> If you make no changes to s_lock.h at all, will it work? Why not?

It's good to keep the work independent and I don't want to hold up
anything happening in this thread, but just for information: I have
been poking around at the idea of entirely removing the old spinlock
code and pointing spin.h's function-like-macros to the atomics code.
We couldn't do that before, because atomics were sometimes implemented
with spinlocks, but now that pg_atomic_flag is never implemented with
spinlocks we can flip that around, and then have only one place where
we know how to do this stuff.  What is needed for that to progress is,
I guess, to determine though assembler analysis or experimentation
across a bunch of targets that it works out at least as good...

https://www.postgresql.org/message-id/flat/CA%2BhUKGJ%2BoA%2B62iUZ-EQb5R2cAOW3Y942ZoOtzOD%3D1sQ05iNg6Q%40mail.gmail.com#23598cafac3dd08ca94fa9e2228a4764

Using per-transaction memory contexts for storing decoded tuples

2024-09-11 Thread Masahiko Sawada

Hi all,

We have several reports that logical decoding uses memory much more
than logical_decoding_work_mem[1][2][3]. For instance in one of the
reports[1], even though users set logical_decoding_work_mem to
'256MB', a walsender process was killed by OOM because of using more
than 4GB memory.

As per the discussion in these threads so far, what happened was that
there was huge memory fragmentation in rb->tup_context.
rb->tup_context uses GenerationContext with 8MB memory blocks. We
cannot free memory blocks until all memory chunks in the block are
freed. If there is a long-running transaction making changes, its
changes could be spread across many memory blocks and we end up not
being able to free memory blocks unless the long-running transaction
is evicted or completed. Since we don't account fragmentation, block
header size, nor chunk header size into per-transaction memory usage
(i.e. txn->size), rb->size could be less than
logical_decoding_work_mem but the actual allocated memory in the
context is much higher than logical_decoding_work_mem.

We can reproduce this issue with the attached patch
rb_excessive_memory_reproducer.patch (not intended to commit) that
adds a memory usage reporting and includes the test. After running the
tap test contrib/test_decoding/t/002_rb_memory.pl, we can see a memory
usage report in the server logs like follows:

LOG: reorderbuffer memory: logical_decoding_work_mem=268435456
rb->size=17816832 rb->tup_context=1082130304 rb->context=1086267264

Which means that the logical decoding allocated 1GB memory in spite of
logical_decoding_work_mem being 256MB.

One idea to deal with this problem is that we use
MemoryContextMemAllocated() as the reorderbuffer's memory usage
instead of txn->total_size. That is, we evict transactions until the
value returned by MemoryContextMemAllocated() gets lower than
logical_decoding_work_mem. However, it could end up evicting a lot of
(possibly all) transactions because the transaction whose decoded
tuples data are spread across memory context blocks could be evicted
after all other larger transactions are evicted (note that we evict
transactions from largest to smallest). Therefore, if we want to do
that, we would need to change the eviction algorithm to for example
LSN order instead of transaction size order. Furthermore,
reorderbuffer's changes that are counted in txn->size (and therefore
rb->size too) are stored in different memory contexts depending on the
kinds. For example, decoded tuples are stored in rb->context,
ReorderBufferChange are stored in rb->change_context, and snapshot
data are stored in builder->context. So we would need to sort out
which data is stored into which memory context and which memory
context should be accounted for in the reorderbuffer's memory usage.
Which could be a large amount of work.

Another idea is to have memory context for storing decoded tuples per
transaction. That way, we can ensure that all memory blocks of the
context are freed when the transaction is evicted or completed. I
think that this idea would be simpler and worth considering, so I
attached the PoC patch, use_tup_context_per_ctx_v1.patch. Since the
decoded tuple data is created individually when the corresponding WAL
record is decoded but is released collectively when it is released
(e.g., transaction eviction), the bump memory context would fit the
best this case. One exception is that we immediately free the decoded
tuple data if the transaction is already aborted. However, in this
case, I think it is possible to skip the WAL decoding in the first
place.

One thing we need to consider is that the number of transaction
entries and memory contexts in the reorderbuffer could be very high
since it has entries for both top-level transaction entries and
sub-transactions. To deal with that, the patch changes that decoded
tuples of a subtransaction are stored into its top-level transaction's
tuple context. We always evict sub-transactions and its top-level
transaction at the same time, I think it would not be a problem.
Checking performance degradation due to using many memory contexts
would have to be done.

Even with this idea, we would still have a mismatch between the actual
amount of allocated memory and rb->size, but it would be much better
than today. And something like the first idea would be necessary to
address this mismatch, and we can discuss it in a separate thread.

Regards,

[1]
https://www.postgresql.org/message-id/CAMnUB3oYugXCBLSkih%2BqNsWQPciEwos6g_AMbnz_peNoxfHwyw%40mail.gmail.com
[2]
https://www.postgresql.org/message-id/17974-f8c9d353a62f414d%40postgresql.org
[3]
https://www.postgresql.org/message-id/DB9PR07MB71808AC6C7770AF2FB36B95BCB252%40DB9PR07MB7180.eurprd07.prod.outlook.com

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

use_tup_context_per_tx_v1.patch
Description: Binary data

rb_excessive_memory_reproducer.patch
Description: Binary data

Re: [PoC] Federated Authn/z with OAUTHBEARER

2024-09-11 Thread Jacob Champion

(Thanks for the commit, Peter!)

On Wed, Sep 11, 2024 at 6:44 AM Daniel Gustafsson  wrote:
>
> > On 11 Sep 2024, at 09:37, Peter Eisentraut  wrote:
>
> > Is there any sense in dealing with the libpq and backend patches separately 
> > in sequence, or is this split just for ease of handling?
>
> I think it's just make reviewing a bit easier.  At this point I think they can
> be merged together, it's mostly out of historic reasons IIUC since the 
> patchset
> earlier on supported more than one library.

I can definitely do that (and yeah, it was to make the review slightly
less daunting). The server side could potentially be committed
independently, if you want to parallelize a bit, but it'd have to be
torn back out if the libpq stuff didn't land in time.

> > (I suppose the 0004 "review comments" patch should be folded into the 
> > respective other patches?)

Yes. I'm using that patch as a holding area while I write tests for
the hunks, and then moving them backwards.

> I added a warning to autconf in case --with-oauth is used without 
> --with-python
> since this combination will error out in running the tests.  Might be
> superfluous but I had an embarrassingly long headscratcher myself as to why 
> the
> tests kept failing =)

Whoops, sorry. I guess we should just skip them if Python isn't there?

> CURL_IGNORE_DEPRECATION(x;) broke pgindent, it needs to keep the semicolon on
> the outside like CURL_IGNORE_DEPRECATION(x);.  This doesn't really work well
> with how the macro is defined, not sure how we should handle that best (the
> attached makes the style as per how pgindent want's it with the semicolon
> returned).

Ugh... maybe a case for a pre_indent rule in pgindent?

> The oauth_validator test module need to load Makefile.global before exporting
> the symbols from there.

Hm. Why was that passing the CI, though...?

> There is a first stab at documenting the validator module API, more to come 
> (it
> doesn't compile right now).
>
> It contains a pgindent and pgperltidy run to keep things as close to in final
> sync as we can to catch things like the curl deprecation macro mentioned above
> early.

Thanks!

> > What could be the next steps to keep this moving along, other than stare at 
> > the remaining patches until we're content with them? ;-)
>
> I'm in the "stare at things" stage now to try and get this into the tree =)

Yeah, and I still owe you all an updated roadmap.

While I fix up the tests, I've also been picking away at the JSON
encoding problem that was mentioned in [1]; the recent SASLprep fix
was fallout from that, since I'm planning to pull in pieces of its
UTF-8 validation. I will eventually want to fuzz the heck out of this.

> To further pick away at this huge patch I propose to merge the SASL message
> length hunk which can be extracted separately.  The attached .txt (to keep the
> CFBot from poking at it) contains a diff which can be committed ahead of the
> rest of this patch to make it a tad smaller and to keep the history of that
> change a bit clearer.

LGTM!

--

Peter asked me if there were plans to provide a "standard" validator
module, say as part of contrib. The tricky thing is that Bearer
validation is issuer-specific, and many providers give you an opaque
token that you're not supposed to introspect at all.

We could use token introspection (RFC 7662) for online verification,
but last I looked at it, no one had actually implemented those
endpoints. For offline verification, I think the best we could do
would be to provide a generic JWT Profile (RFC 9068) validator, but
again I don't know if anyone is actually providing those token formats
in practice. I'm inclined to push that out into the future.

Thanks,
--Jacob

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

Re: libpq: Process buffered SSL read bytes to support records >8kB on async API

2024-09-11 Thread Jacob Champion

On Wed, Sep 11, 2024 at 12:08 PM Lars Kanis  wrote:
> How did you verify the issue on the server side - with YugabyteDB or
> with a modified Postgres server? I'd like to verify the GSSAPI part and
> I'm familiar with the Postgres server only.

Neither, unfortunately -- I have a protocol testbed that I use for
this kind of stuff. I'm happy to share once I get it cleaned up, but
it's unlikely to help you in this case since I haven't implemented
gssenc support. Patching the Postgres server itself seems like a good
way to go.

> > And are there any other sites that
> > need to make the same guarantee before returning?
>
> Which other sites do you mean?

I'm mostly worried that other parts of libpq might assume that a
single call to pqReadData will drain the buffers. If not, great! --
but I haven't had time to check all the call sites.

> > I need to switch away from this for a bit. Would you mind adding this
> > to the next Commitfest as a placeholder?
>
> No problem; registered: https://commitfest.postgresql.org/50/5251/

Thank you!

--Jacob

Re: query_id, pg_stat_activity, extended query protocol

On Wed, Sep 11, 2024 at 05:02:07PM -0500, Sami Imseih wrote:
> In your 0003-Report-query-ID-for-execute-fetch-in-extended-query-.patch 
> patch, you are still setting the queryId inside exec_execute_message 
> if (execute_is_fetch). This condition could be removed and don't need to set 
> the queryId inside ExecutorRun. This is exactly what v5-0001 does. 
> 
> V5-0001 also sets the queryId inside the exec_bind_message.
> We must do that otherwise we will have a NULL queryId during bind.
> 
> also tested it against this for the case that was raised by Robert [1].

There are a few ways to do things:
- Add an extra report in ExecutorRun(), because we know that it is
going to be what we are going to cross when using a portal with
multiple execution calls.  This won't work for the case of multiple
fetch messages where there is only one initial ExecutorRun() call
followed by the tuple fetches, as you say.
- Add these higher in the stack, when processing the messages.  In
which case, we can also argue about removing the calls in
ExecutorRun() and ExecutorStart(), entirely, because these are
unnecessary duplicates as long as the query ID is set close to where
it is reset when we are processing the kind and execute messages.
ExecutorStart() as report location is ill-thought from the start.
- Keep all of them, relying on the first one set as the follow-up ones
are harmless.  Perhaps also just reduce the number of calls on HEAD.

After sleeping on it, I'd tend to slightly favor the last option in
the back-branches and the second option on HEAD where we reduce the
number of report calls.  This way, we are a bit more careful in
released branches by being more aggressive in reporting the query ID.
That's also why I have ordered the previous patch set this way but
that was badly presented, even if it does not take care of the
processing of the execute_is_fetch case for execute messages.

The tests in pg_stat_statements are one part I'm pretty sure is one
good way forward.  It is not perfect, but with the psql meta-commands
we have a good deal of coverage on top of the other queries already in
the test suite.  That's also the only place in core where we force the
query ID across all these hooks, and this does not impact switching
the way stats are stored if we were to switch to pgstats in shmem with
the custom stats APIs.

> I am not sure how we can add tests for RevalidateCachedQuery though using
> pg_stat_statements. We could skip testing this scenario, maybe??

Perhaps.  I'd need to think through this one.  Let's do things in
order and see about the reports for the bind/execute messages, first,
please?
--
Michael

signature.asc
Description: PGP signature

Re: Separate HEAP WAL replay logic into its own file

On Wed, Sep 11, 2024 at 04:41:49PM +0900, Michael Paquier wrote:
> +#include "access/heapam_xlog.h"
> 
> This is included in heapam.h, but missing from the patch.  I guess
> that you fat-fingered a `git add`.

It looks like my mind was wondering away when I wrote this part.
Sorry for the useless noise.
--
Michael


signature.asc
Description: PGP signature

Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible

On Wed, Sep 11, 2024 at 02:29:49PM -0700, Jacob Champion wrote:
> On Mon, Sep 9, 2024 at 10:30 PM Michael Paquier  wrote:
>> No.  My question was about splitting pgstat_bestart() and
>> pgstat_bestart_pre_auth() in a cleaner way, because authenticated
>> connections finish by calling both, meaning that we do twice the same
>> setup for backend entries depending on the authentication path taken.
>> That seems like a waste.
> 
> I can try to separate them out. I'm a little wary of messing with the
> CRITICAL_SECTION guarantees, though. I thought the idea was that you
> filled in the entire struct to prevent tearing. (If I've misunderstood
> that, please let me know :D)

Hm, yeah.  We surely should be careful about the consequences of that.
Setting up twice the structure as the patch proposes is kind of
a weird concept, but it feels to me that we should split that and set
the fields in the pre-auth step and ignore the irrelevant ones, then
complete the rest in a second step.  We are going to do that anyway if
we want to be able to have backend entries earlier in the
authentication phase.

>> Couldn't it be better to have a one-one mapping
>> instead, adding twelve entries in wait_event_names.txt?
> 
> (I have no strong opinions on this myself, but while the debate is
> ongoing, I'll work on a version of the patch with more detailed wait
> events. It's easy to collapse them again if that gets the most votes.)

Thanks.  Robert is arguing upthread about more granularity, which is
also what I understand is the original intention of the wait events.
Noah has a different view.  Let's see where it goes but I've given my
opinion.

> I can test for specific contents of the entry, if you'd like. My
> primary goal was to test that an entry shows up if that part of the
> code hangs. I think a regression would otherwise go completely
> unnoticed.

Perhaps that would be useful, not sure.  Based on my first
impressions, I'd tend to say no to these extra test cycles, but I'm
okay to be proved wrong, as well.
--
Michael

signature.asc
Description: PGP signature

Re: [PATCH] Refactor SLRU to always use long file names

On Wed, Sep 11, 2024 at 04:07:06PM +0300, Aleksander Alekseev wrote:
> Commit 4ed8f0913bfd introduced long SLRU file names. The proposed
> patch removes SlruCtl->long_segment_names flag and makes SLRU always
> use long file names. This simplifies both the code and the API.
> Corresponding changes to pg_upgrade are included.

That's leaner, indeed.

> One drawback I see is that technically SLRU is an exposed API and
> changing it may affect third-party code. I'm not sure if we should
> seriously worry about this. Firstly, the change is trivial and
> secondly, it's not clear whether such third-party code even exists (we
> broke this API just recently in 4ed8f0913bfd and no one complained).

Any third-party code using custom SLRUs would need to take care of
handling their upgrade path outside pg_upgrade.  Not sure there are
any of them, TBH, but let's see.

> I didn't include any tests for the new pg_upgrade code. To my
> knowledge we test it manually, with buildfarm members and during
> alpha- and beta-testing periods. Please let me know if you think there
> should be a corresponding TAP test.

Removing the old API means that it is impossible to test a move from
short to long file names.  That's OK by me to rely on the pg_upgrade
paths in the buildfarm code.  We have a few of them.

There is one thing I am wondering, here, though, which is to think
harder about a validity check at the end of 002_pg_upgrade.pl to make
sure that all the SLRU use long file names after running the tests.
That would mean thinking about a mechanism to list all of them from a
backend, rather than hardcode a list of them.  Perhaps that's not
worth it, just dropping an idea in the bucket of ideas.  I would guess
in the shape of a catalog that's able to represent at SQL level all
the SLRUs that exist in a backend.
--
Michael


signature.asc
Description: PGP signature

Re: Add 64-bit XIDs into PostgreSQL 15

On Thu, Jul 25, 2024 at 02:09:10PM +0300, Heikki Linnakangas wrote:
> On 25/07/2024 13:19, Peter Eisentraut wrote:
>> Conversely, if there is still some threshold (not disaster, but
>> efficiency or something else), would it still be useful to keep these
>> settings well below 2^31?  In which case, we might not need 64-bit GUCs.
> 
> Yeah, I don't think it's critical. It makes sense to switch to 64 bit GUCs,
> so that you can make those settings higher, but it's not critical or
> strictly required for the rest of the work.

It looks like we have some sort of consensus to introduce these GUC
APIs, then?  I'd suggest to split that into its own thread because it
can be treated as an independent subject.  (I know that this was
discussed previously, but it's been some time and this is going to
require a rebased version anyway, so..)

> Another approach is to make the GUCs to mean "thousands of XIDs", similar to
> how many of our memory settings are in kB rather than bytes. that might be a
> super confusing change for existing settings though.

I find that a bit confusing as well, still that would be OK in terms
of compatibility as long as you enforce the presence of a unit and
leave the default behavior alone.  So it does not sound that bad to
be, either.  It is also a bit simpler to set for users.  Less zeros to
deal with.

Aleksander Alekseev has proposed to remove short file names from SLRUs
to cimplify its internals, as of this thread, so that's one less topic
to deal with here:
https://www.postgresql.org/message-id/CAJ7c6TOy7fUW9MuNeOWor3cSFnQg9tgz%3DmjXHDb94GORtM_Eyg%40mail.gmail.com

I am unclear about the rest of the thread.  Considering v55-0004 and
v55-0003 as two different topics, what should we do with the other
things?  It does not seem to me that we have a clear consensus on if
we are going to do something about some epoch:xid -> 64b XID switch,
and what are the arguments in play here.  The thread has been long, so
I may be just missing the details but a summary would be nice.
--
Michael


signature.asc
Description: PGP signature

Remove shadowed declaration warnings

2024-09-11 Thread Peter Smith

Hi,

I normally build the code with warnings enabled (specifically,
-Wshadow) which exposes many "shadowed" declarations.

It would be better to reduce warnings wherever it's easy to do so,
because if we always see/ignore lots of warnings then sooner or later
something important may escape attention. In any case, just removing
these warnings sometimes makes the code more readable by removing any
ambiguity.

Currently, I'm seeing 350+ shadow warnings. See the attached summary.

It will be WIP to eliminate them all, but here is a patch to address
some of the low-hanging fruit. If the patch is accepted, then I can
try later to deal with more of them.

==

The following are fixed by changing the param/var from 'text' to 'txt'.

execute.c:111:19: warning: declaration of ‘text’ shadows a global
declaration [-Wshadow]
../../../../src/include/c.h:700:24: warning: shadowed declaration is
here [-Wshadow]

~

prepare.c:104:26: warning: declaration of ‘text’ shadows a global
declaration [-Wshadow]
../../../../src/include/c.h:700:24: warning: shadowed declaration is
here [-Wshadow]
prepare.c:270:12: warning: declaration of ‘text’ shadows a global
declaration [-Wshadow]
../../../../src/include/c.h:700:24: warning: shadowed declaration is
here [-Wshadow]

~

c_keywords.c:36:32: warning: declaration of ‘text’ shadows a global
declaration [-Wshadow]
../../../../src/include/c.h:700:24: warning: shadowed declaration is
here [-Wshadow]
ecpg_keywords.c:39:35: warning: declaration of ‘text’ shadows a global
declaration [-Wshadow]
../../../../src/include/c.h:700:24: warning: shadowed declaration is
here [-Wshadow]

~

tab-complete.c:1638:29: warning: declaration of ‘text’ shadows a
global declaration [-Wshadow]
../../../src/include/c.h:700:24: warning: shadowed declaration is here
[-Wshadow]
tab-complete.c:5082:46: warning: declaration of ‘text’ shadows a
global declaration [-Wshadow]
../../../src/include/c.h:700:24: warning: shadowed declaration is here
[-Wshadow]
tab-complete.c:5111:38: warning: declaration of ‘text’ shadows a
global declaration [-Wshadow]
../../../src/include/c.h:700:24: warning: shadowed declaration is here
[-Wshadow]
tab-complete.c:5120:36: warning: declaration of ‘text’ shadows a
global declaration [-Wshadow]
../../../src/include/c.h:700:24: warning: shadowed declaration is here
[-Wshadow]
tab-complete.c:5129:37: warning: declaration of ‘text’ shadows a
global declaration [-Wshadow]
../../../src/include/c.h:700:24: warning: shadowed declaration is here
[-Wshadow]
tab-complete.c:5140:33: warning: declaration of ‘text’ shadows a
global declaration [-Wshadow]
../../../src/include/c.h:700:24: warning: shadowed declaration is here
[-Wshadow]
tab-complete.c:5148:43: warning: declaration of ‘text’ shadows a
global declaration [-Wshadow]
../../../src/include/c.h:700:24: warning: shadowed declaration is here
[-Wshadow]
tab-complete.c:5164:40: warning: declaration of ‘text’ shadows a
global declaration [-Wshadow]
../../../src/include/c.h:700:24: warning: shadowed declaration is here
[-Wshadow]
tab-complete.c:5172:50: warning: declaration of ‘text’ shadows a
global declaration [-Wshadow]
../../../src/include/c.h:700:24: warning: shadowed declaration is here
[-Wshadow]
tab-complete.c:5234:19: warning: declaration of ‘text’ shadows a
global declaration [-Wshadow]
../../../src/include/c.h:700:24: warning: shadowed declaration is here
[-Wshadow]
tab-complete.c:5605:32: warning: declaration of ‘text’ shadows a
global declaration [-Wshadow]
../../../src/include/c.h:700:24: warning: shadowed declaration is here
[-Wshadow]
tab-complete.c:5685:33: warning: declaration of ‘text’ shadows a
global declaration [-Wshadow]
../../../src/include/c.h:700:24: warning: shadowed declaration is here
[-Wshadow]
tab-complete.c:5733:37: warning: declaration of ‘text’ shadows a
global declaration [-Wshadow]
../../../src/include/c.h:700:24: warning: shadowed declaration is here
[-Wshadow]
tab-complete.c:5778:33: warning: declaration of ‘text’ shadows a
global declaration [-Wshadow]
../../../src/include/c.h:700:24: warning: shadowed declaration is here
[-Wshadow]
tab-complete.c:5910:27: warning: declaration of ‘text’ shadows a
global declaration [-Wshadow]
../../../src/include/c.h:700:24: warning: shadowed declaration is here
[-Wshadow]

==

The following was fixed by changing the name of the global static from
'progname' to 'prog_name'.

pg_createsubscriber.c:341:46: warning: declaration of ‘progname’
shadows a global declaration [-Wshadow]
pg_createsubscriber.c:114:20: warning: shadowed declaration is here [-Wshadow]

~

The following was fixed by changing the name of the global static from
'dbinfo' to 'db_info'.

pg_createsubscriber.c:437:25: warning: declaration of ‘dbinfo’ shadows
a global declaration [-Wshadow]
pg_createsubscriber.c:121:31: warning: shadowed declaration is here [-Wshadow]
pg_createsubscriber.c:734:40: warning: declaration of ‘dbinfo’ shadows
a global declaration [-Wshadow]
pg_createsubscriber.c:121:31: warning: sh

Re: Remove shadowed declaration warnings

On Thu, 12 Sept 2024 at 12:33, Peter Smith  wrote:
> I normally build the code with warnings enabled (specifically,
> -Wshadow) which exposes many "shadowed" declarations.
>
> It would be better to reduce warnings wherever it's easy to do so,
> because if we always see/ignore lots of warnings then sooner or later
> something important may escape attention. In any case, just removing
> these warnings sometimes makes the code more readable by removing any
> ambiguity.

0fe954c28 did add -Wshadow=compatible-local to the standard set of
complication flags.  I felt it was diminishing returns after that, but
-Wshadow=local would be the next step before going full -Wshadow.

There was justification for -Wshadow=compatible-local because there
has been > 1 bug (see af7d270dd) fixed that would have been found if
we'd had that sooner. Have we ever had any bugs that would have been
highlighted by -Wshadow but not -Wshadow=compatible-local? I'd be
curious to know if you do go through this process of weeding these out
if you do find any bugs as a result.

I also wonder if we could ever get this to a stable point. I didn't
take the time to understand what 388e80132 did. Is that going to
protect us from getting warnings where fixing them is beyond our
control for full -Wshadow?

David

Re: Incremental Sort Cost Estimation Instability

On Thu, 27 Jun 2024 at 03:00, Andrei Lepikhov  wrote:
> I tried to invent a simple solution to fight this minor case. But the
> most clear and straightforward way here is to save a reference to the
> expression that triggered the PathKey creation inside the PathKey itself.
> See the sketch of the patch in the attachment.
> I'm not sure this instability is worth fixing this way, but the
> dependence of the optimisation outcome on the query text looks buggy.

I don't think that's going to work as that'll mean it'll just choose
whichever expression was used when the PathKey was first created.  For
your example query, both PathKey's are first created for the GROUP BY
clause in standard_qp_callback(). I only have to change the GROUP BY
in your query to use the equivalent column in the other table to get
it to revert back to the plan you complained about.

postgres=# EXPLAIN (costs off) SELECT count(*) FROM test t1, test t2
WHERE t1.x=t2.y AND t1.y=t2.x GROUP BY t2.y,t2.x;
QUERY PLAN
--
 GroupAggregate
   Group Key: t2.y, t2.x
   ->  Sort
 Sort Key: t2.y, t2.x
 ->  Merge Join
   Merge Cond: (t1.y = t2.x)
   Join Filter: (t2.y = t1.x)
   ->  Index Scan using test_y_idx on test t1
   ->  Index Scan using test_x_idx on test t2
(9 rows)

Maybe doing something with estimate_num_groups() to find the
EquivalenceClass member with the least distinct values would be
better. I just can't think how that could be done in a performant way.

David

Re: CI, macports, darwin version problems

2024-09-11 Thread Thomas Munro

On Tue, Sep 10, 2024 at 3:04 PM Thomas Munro  wrote:
> On Mon, Sep 9, 2024 at 1:37 PM Joe Conway  wrote:
> > Seems the mounted drive got unmounted somehow ¯\_(ツ)_/¯
> >
> > Please check it out and let me know if it is working properly now.
>
> Looks good, thanks!

... but it's broken again.

Re: Add RESPECT/IGNORE NULLS and FROM FIRST/LAST options

>> >> On Sat, 6 May 2023, 04:57 Tatsuo Ishii,  wrote:
>> >>>
>> >>> Attached is the patch to implement this (on top of your patch).
>> >>>
>> >>> test=# SELECT row_number() RESPECT NULLS OVER () FROM (SELECT 1) AS s;
>> >>> ERROR:  window function row_number cannot have RESPECT NULLS or IGNORE 
>> >>> NULLS
>> >>
>> >>
>> >> The last time this was discussed 
>> >> (https://www.postgresql.org/message-id/1037735.1610402426%40sss.pgh.pa.us)
>> >>  it was suggested to make the feature generalizable, beyond what the 
>> >> standard says it should be limited to.
>> >>
>> >> With it generalizable, there would need to be extra checks for custom 
>> >> functions, such as if they allow multiple column arguments (which I'll 
>> >> add in v2 of the patch if the design's accepted).
>> >>
>> >> So I think we need a consensus on whether to stick to limiting it to 
>> >> several specific functions, or making it generalized yet agreeing the 
>> >> rules to limit it (such as no agg functions, and no functions with 
>> >> multiple column arguments).

It seems you allow to use IGNORE NULLS for all window functions. If
the case, you should explicitely stat that in the docs. Otherwise
users will be confused because;

1) The SQL standard says IGNORE NULLS only for lead, lag, first_value,
last_value and nth_value.

2) Some window function returns same rows with IGNORE NULLS/RESPECT
NULLS. Consider following case.

test=# create table t1(i int);
CREATE TABLE
test=# insert into t1 values(NULL),(NULL);
INSERT 0 2
test=# select * from t1;
 i 
---

(2 rows)

test=# SELECT row_number() IGNORE NULLS OVER w FROM t1 WINDOW w AS (ORDER BY i);
 row_number 

  1
  2
(2 rows)

The t1 table only contains NULL rows. By using IGNORE NULLS, I think
it's no wonder that a user expects 0 rows returned, if there's no
mention in the docs that actually IGNORE NULLS/RESPECT NULLS are just
ignored in some window functions.

Instead I think it's better that other than lead, lag, first_value,
last_value and nth_value each window function errors out if IGNORE
NULLS/RESPECT NULL are passed to these window functions.

I take a look at the patch and noticed that following functions have
no comments on what they are doing and what are the arguments. Please
look into other functions in nodeWindowAgg.c and add appropriate
comments to those functions.

+static void increment_notnulls(WindowObject winobj, int64 pos)

+static Datum ignorenulls_getfuncarginpartition(WindowObject winobj, int argno,
+   int relpos, int seektype, bool 
set_mark, bool *isnull, bool *isout) {

+static Datum ignorenulls_getfuncarginframe(WindowObject winobj, int argno,
+   int relpos, int seektype, bool 
set_mark, bool *isnull, bool *isout) {

Also the coding style does not fit into our coding standard. They should be 
written something like:

static void
increment_notnulls(WindowObject winobj, int64 pos)

static Datum
ignorenulls_getfuncarginpartition(WindowObject winobj, int argno,
int relpos, int seektype, bool 
set_mark, bool *isnull, bool *isout)
{

static Datum
ignorenulls_getfuncarginframe(WindowObject winobj, int argno,
int relpos, int seektype, bool 
set_mark, bool *isnull, bool *isout)
{

See also:
https://www.postgresql.org/docs/current/source-format.html

+   int ignore_nulls;   /* ignore nulls */

You should add more comment here. I.e. what values are possible for
ignore_nulls.

I also notice that you have an array in memory which records non-null
row positions in a partition. The position is represented in int64,
which means 1 entry consumes 8 bytes. If my understanding is correct,
the array continues to grow up to the partition size. Also the array
is created for each window function (is it really necessary?). I worry
about this because it might consume excessive memory for big
partitions.

Best reagards,
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp

Re: Remove useless GROUP BY columns considering unique index

On Sat, 30 Dec 2023 at 04:05, Zhang Mingli  wrote:
> So my patch make it easy: check unique index’s columns, it’s a valid 
> candidate if all of that have NOT NULL constraint.
> And we choose a best one who has the least column numbers in 
> get_min_unique_not_null_attnos(), as the reason: less columns mean that more 
> group by columns could be removed.

This patch no longer applies.  We no longer catalogue NOT NULL
constraints, which this patch is coded to rely upon. (Likely it could
just look at pg_attribute.attnotnull instead)

Aside from that, I'm not sure about the logic to prefer removing
functionally dependent columns using the constraint with the least
columns.  If you had the PRIMARY KEY on two int columns and a unique
index on a 1MB varlena column, I think using the primary key would be
a better choice to remove functionally dependent columns of.  Maybe
it's ok to remove any functionally dependant columns on the primary
key first and try removing functionally dependent columns on unique
constraints and a 2nd step (or maybe only if none can be removed using
the PK?)

Also, why constraints rather than unique indexes?  You'd need to check
for expression indexes and likely ignore those due to no ability to
detect NOT NULL for expressions.

Also, looking at the patch to how you've coded
get_min_unique_not_null_attnos(), it looks like you're very optimistic
about that being a constraint that has columns mentioned in the GROUP
BY clause. It looks like it  won't work if the UNIQUE constraint with
the least columns gets no mention in the GROUP BY clause. That'll
cause performance regressions from what we have today when the primary
key is mentioned and columns can be removed using that but a UNIQUE
constraint exists which has no corresponding GROUP BY columns and
fewer unique columns than the PK. That's not good and it'll need to be
fixed.

Set to waiting on author.

David

Re: Remove shadowed declaration warnings

David Rowley  writes:
> On Thu, 12 Sept 2024 at 12:33, Peter Smith  wrote:
>> I normally build the code with warnings enabled (specifically,
>> -Wshadow) which exposes many "shadowed" declarations.

> 0fe954c28 did add -Wshadow=compatible-local to the standard set of
> complication flags.  I felt it was diminishing returns after that, but
> -Wshadow=local would be the next step before going full -Wshadow.

I think that insisting that local declarations not shadow globals
is an anti-pattern, and I'll vote against any proposal to make
that a standard warning.  Impoverished as C is, it does have block
structure; why would we want to throw that away by (in effect)
demanding a single flat namespace for the entire program?

I do grant that sometimes shadowing of locals can cause bugs.  I don't
recall right now why we opted for -Wshadow=compatible-local over
-Wshadow=local, but we could certainly take another look at that.

regards, tom lane

Re: Add memory/disk usage for WindowAgg nodes in EXPLAIN

> On Fri, 6 Sept 2024 at 19:08, Ashutosh Bapat
>  wrote:
>> The changes look better. A nitpick though. With their definitions
>> changed, I think it's better to change the names of the functions
>> since their purpose has changed. Right now they report the storage
>> type and size used, respectively, at the time of calling the function.
>> With this patch, they report maximum space ever used and the storage
>> corresponding to the maximum space. tuplestore_space_used() may be
>> changed to tuplestore_maxspace_used(). I am having difficulty with
>> tuplestore_storage_type_name(); tuplestore_largest_storage_type_name()
>> seems mouthful and yet not doing justice to the functionality. It
>> might be better to just have one funciton tuplestore_maxspace_used()
>> which returns both the maximum space used as well as the storage type
>> when maximum space was used.
> 
> How about just removing tuplestore_storage_type_name() and
> tuplestore_space_used() and adding tuplestore_get_stats(). I did take
> some inspiration from tuplesort.c for this, so maybe we can defer back
> there for further guidance. I'm not so sure it's worth having a stats
> struct type like tuplesort.c has. All we need is a char ** and an
> int64 * output parameter to pass to the stats function.  I don't think
> we need to copy the tuplesort_method_name(). It seems fine just to
> point the output parameter of the stats function at the statically
> allocated constant.

Are you going to push the changes to tuplestore.c anytime soon? I
would like to rebase my patch[1] but the patch could be affected by
the tuplestore API change.

Best reagards,

[1] 
https://www.postgresql.org/message-id/CAApHDvoY8cibGcicLV0fNh%3D9JVx9PANcWvhkdjBnDCc9Quqytg%40mail.gmail.com
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp

Re: Opinion poll: Sending an automated email to a thread when it gets added to the commitfest

On Thu, 15 Aug 2024 at 20:00, Jelte Fennema-Nio  wrote:
>
> On Thu, 15 Aug 2024 at 05:17, Euler Taveira  wrote:
> > If I understand your proposal correctly, there will be another email to the
> > thread if the previous CF was closed and someone opened a new CF entry.
> > Sometimes some CF entries are about the same thread.
>
> Yeah, that's correct. If a new CF entry is created for an existing
> thread a new email would be sent. But to be clear, if CF entry is
> pushed to the next commitfest, **no** new email would be sent.

I was thinking about this today when looking at [1], you can see at
the end of that email someone posted a link to the CF entry.
Unfortunately, that was for the Jan-2024 CF, which is not really that
useful to look at today.

It looks like only about 35% of patches in this CF have "num CFs" = 1,
so it might be annoying to be taken to an old entry 65% of the time
you click the proposed URL. Maybe instead of the URLs showing the CF
number that the patch was first added to, if it could have a reference
in the URL that looks up the maximum CF which contains the patch and
show that one. e.g. https://commitfest.postgresql.org/latest/4751/ .
Without that, if you try to modify the status of a patch in an older
CF, you'll just be told that you can't do that.  (it is true you can
click the latest CF in the status column, but that's at least one more
click than I'd have hoped)

David

[1] 
https://postgr.es/m/cacjufxedavjhkudhj1jraxnz9aynqu+tvjuqjzqbugs06on...@mail.gmail.com

Re: Add RESPECT/IGNORE NULLS and FROM FIRST/LAST options

2024-09-11 Thread David G. Johnston

On Wednesday, September 11, 2024, Tatsuo Ishii  wrote:

>
> test=# SELECT row_number() IGNORE NULLS OVER w FROM t1 WINDOW w AS (ORDER
> BY i);
>  row_number
> 
>   1
>   2
> (2 rows)
>
> The t1 table only contains NULL rows. By using IGNORE NULLS, I think
> it's no wonder that a user expects 0 rows returned, if there's no
> mention in the docs that actually IGNORE NULLS/RESPECT NULLS are just
> ignored in some window functions.
>

My nieve understanding of the nulls treatment is computations are affected,
therefore a zero-argument function is incapable of abiding by this clause
(it should error…).  Your claim that this should somehow produce zero rows
confuses me on two fronts.  One, window function should be incapable of
affecting how many rows are returned.  The query must output two rows
regardless of the result of the window expression (it should at worse
produce the null value).  Two, to produce said null value you have to be
ignoring the row due to the order by clause seeing a null.  But the order
by isn’t part of the computation.

David J.

Re: Pgstattuple on Sequences: Seeking Community Feedback on Potential Patch

On Wed, Sep 11, 2024 at 11:02:43PM +0100, Matthias van de Meent wrote:
> On Wed, 11 Sept 2024 at 21:36, Nathan Bossart  
> wrote:
> >
> > Barring objections, I'm planning to commit v3 soon.  Robert/Matthias, I'm
> > not sure you are convinced this is the right thing to do (or worth doing,
> > rather), but I don't sense that you are actually opposed to it, either.
> > Please correct me if I am wrong.
> 
> Correct: I do think making heapam-related inspection functions have a
> consistent behaviour when applied to sequences is beneficial, with no
> preference towards specifically supporting or not supporting sequences
> in these functions. If people that work on sequences think it's better
> to also support inspection of sequences, then I think that's a good
> reason to add that support where it doesn't already exist.
> 
> As for patch v3, that seems fine with me.

+1 from here as well, after looking at what v3 is doing for these two
modules.
--
Michael


signature.asc
Description: PGP signature

Re: json_query conditional wrapper bug

2024-09-11 Thread Amit Langote

On Wed, Sep 11, 2024 at 8:56 PM Peter Eisentraut  wrote:
> On 11.09.24 13:25, Amit Langote wrote:
> > On Wed, Sep 11, 2024 at 6:57 PM Peter Eisentraut  
> > wrote:
> >> On 11.09.24 09:51, Amit Langote wrote:
> > I've updated your patch to include updated test outputs and a nearby
> > code comment expanded.  Do you intend to commit it or do you prefer
> > that I do?
> 
>  This change looks unrelated:
> 
>  -ERROR:  new row for relation "test_jsonb_constraints" violates check
>  constraint "test_jsonb_constraint4"
>  +ERROR:  new row for relation "test_jsonb_constraints" violates check
>  constraint "test_jsonb_constraint5"
> 
>  Is this some randomness in the way these constraints are evaluated?
> >>>
> >>> The result of JSON_QUERY() in the CHECK constraint changes, so the
> >>> constraint that previously failed now succeeds after this change,
> >>> because the comparison looked like this before and after:
> >>>
> >>> -- before
> >>> postgres=# select jsonb '[10]' < jsonb '[10]';
> >>>?column?
> >>> --
> >>>f
> >>> (1 row)
> >>>
> >>> -- after
> >>> postgres=# select jsonb '10' < jsonb '[10]';
> >>>?column?
> >>> --
> >>>t
> >>> (1 row)
> >>>
> >>> That causes the next constraint to be evaluated and its failure
> >>> reported instead.
> >>>
> >>> In the attached, I've adjusted the constraint for the test case to be
> >>> a bit more relevant and removed a nearby somewhat redundant test,
> >>> mainly because its output changes after the adjustment.
> >>
> >> Ok, that looks good.  Good that we could clear that up a bit.
> >
> > Thanks for checking.  Would you like me to commit it?
>
> Please do.

Done.  Thanks for the report and the patch.

-- 
Thanks, Amit Langote

Re: Remove shadowed declaration warnings

On Thu, 12 Sept 2024 at 14:03, Tom Lane  wrote:
> I do grant that sometimes shadowing of locals can cause bugs.  I don't
> recall right now why we opted for -Wshadow=compatible-local over
> -Wshadow=local, but we could certainly take another look at that.

I don't recall if it was discussed, but certainly, it was an easier
goal to achieve.

Looks like there are currently 47 warnings with -Wshadow=local. I'm
not quite sure what the definition of "compatible" is for this flag,
but looking at one warning in pgbench.c:4548, I see an int shadowing a
bool. So maybe -Wshadow=local is worthwhile.

David

Re: Add memory/disk usage for WindowAgg nodes in EXPLAIN

On Thu, 12 Sept 2024 at 14:04, Tatsuo Ishii  wrote:
> Are you going to push the changes to tuplestore.c anytime soon? I
> would like to rebase my patch[1] but the patch could be affected by
> the tuplestore API change.

Ok, I'll look at that. I had thought you were taking care of writing the patch.

David

Re: Inconsistency in lock structure after reporting "lock already held" error

2024-09-11 Thread 高增琦

The attached patch attempts to fix this.

高增琦  于2024年9月11日周三 14:30写道：

> At the end of SetupLockInTable(), there is a check for the "lock already
> held" error.
> Because the nRequested and requested[lockmode] value of a lock is bumped
> before "lock already held" error, and there is no way to reduce them later
> for
> this situation, then it will keep the inconsistency in lock structure until
> cluster restart or reset.
>
> The inconsistency is:
> * nRequested will never reduce to zero, the lock will never be
> garbage-collected
> * if there is a waitMask for this lock, the waitMast will never be
> removed, then
>   new proc will be blocked to wait for a lock with zero holder
>   (looks weird in the pg_locks table)
>
> I think moving the "lock already held" error before the bump operation of
> nRequested
> and requested[lockmode] value in SetupLockInTable() will fix it.
> (maybe also fix the lock_twophase_recover() function)
>
> To recreate the inconsistency:
> 1. create a backend 1 to lock table a, keep it idle in transaction
> 2. terminate backend 1 and hack it to skip the LockReleaseAll() function
> 3. create another backend 2 to lock table a, it will wait for the lock to
> release
> 4. reuse the backend 1 (reuse the same proc) to lock table a again,
>it will trigger the "lock already held" error
> 5. quit both backend 1 and 2
> 6. create backend 3 to lock table a, it will wait for the lock's waitMask
> 7. check the pg_locks table
>
> --
> GaoZengqi
> pgf...@gmail.com
> zengqi...@gmail.com
>


-- 
GaoZengqi
pgf...@gmail.com
zengqi...@gmail.com


fix-lock-already-held.patch
Description: Binary data

Re: Add RESPECT/IGNORE NULLS and FROM FIRST/LAST options

> On Wednesday, September 11, 2024, Tatsuo Ishii  wrote:
> 
>>
>> test=# SELECT row_number() IGNORE NULLS OVER w FROM t1 WINDOW w AS (ORDER
>> BY i);
>>  row_number
>> 
>>   1
>>   2
>> (2 rows)
>>
>> The t1 table only contains NULL rows. By using IGNORE NULLS, I think
>> it's no wonder that a user expects 0 rows returned, if there's no
>> mention in the docs that actually IGNORE NULLS/RESPECT NULLS are just
>> ignored in some window functions.
>>
> 
> My nieve understanding of the nulls treatment is computations are affected,
> therefore a zero-argument function is incapable of abiding by this clause
> (it should error…).

Yes. I actually claimed that row_number() should error out if the
clause is provided.

> Instead I think it's better that other than lead, lag, first_value,
> last_value and nth_value each window function errors out if IGNORE
> NULLS/RESPECT NULL are passed to these window functions.

> Your claim that this should somehow produce zero rows
> confuses me on two fronts.  One, window function should be incapable of
> affecting how many rows are returned.  The query must output two rows
> regardless of the result of the window expression (it should at worse
> produce the null value).  Two, to produce said null value you have to be
> ignoring the row due to the order by clause seeing a null.  But the order
> by isn’t part of the computation.

Well I did not claim that. I just gave a possible example what users
could misunderstand. Probably my example was not so good.

Best reagards,
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp

Re: Trim the heap free memory

2024-09-11 Thread shawn wang

Hi Rafia,

I have made the necessary adjustment by replacing the inclusion of malloc.h
with stdlib.h in the relevant codebase. This change should address the
previous concerns regarding memory allocation functions.

Could you please perform another round of testing to ensure that everything
is functioning as expected with this modification?

Thank you for your assistance.

Best regards, Shawn


Rafia Sabih  于2024年9月11日周三 18:25写道：

> Unfortunately, I still see a compiling issue with this patch,
>
> memtrim.c:15:10: fatal error: 'malloc.h' file not found
> #include 
>  ^~
> 1 error generated.
>
> On Wed, 28 Aug 2024 at 12:54, shawn wang  wrote:
>
>> Hi Ashutosh,
>>
>> Ashutosh Bapat  于2024年8月26日周一 19:05写道：
>>
>>> Hi Shawn,
>>> It will be good to document usage of this function. Please add
>>> document changes in your patch. We need to document the impact of this
>>> function so that users can judiciously decide whether or not to use
>>> this function and under what conditions. Also they would know what to
>>> expect when they use this function.
>>
>>
>> I have already incorporated the usage of this function into the new patch.
>>
>>
>> Currently, there is no memory information that can be extremely accurate
>> to
>> reflect whether a trim operation should be performed. Here are two
>> conditions
>> that can be used as references:
>> 1. Check the difference between the process's memory usage (for example,
>> the top command, due to the relationship with shared memory, it is
>> necessary
>> to subtract SHR from RES) and the statistics of the memory context. If the
>> difference is very large, this function should be used to release memory;
>> 2. Execute malloc_stats(). If the system bytes are greater than the
>> in-use bytes, this indicates that this function can be used to release
>> memory.
>>
>>>
>>>
>> Running it after a query finishes is one thing but that can't be
>>> guaranteed because of the asynchronous nature of signal handlers.
>>> malloc_trim() may be called while a query is being executed. We need
>>> to assess that impact as well.
>>>
>>> Can you please share some numbers - TPS, latency etc. with and without
>>> this function invoked during a benchmark run?
>>>
>>
>> I have placed malloc_trim() at the end of the exec_simple_query function,
>> so that malloc_trim() is executed once for each SQL statement executed. I
>> used pgbench to reproduce the performance impact,
>> and the results are as follows.
>> *Database preparation：*
>>
>>> create database testc;
>>> create user t1;
>>> alter database testc owner to t1;
>>> ./pgbench testc -U t1 -i -s 100
>>> ./pgbench testc -U t1 -S -c 100 -j 100 -T 600
>>
>> *Without Trim*:
>>
>>> $./pgbench testc -U t1 -S -c 100 -j 100 -T 600
>>> pgbench (18devel)
>>> starting vacuum...end.
>>> transaction type: 
>>> scaling factor: 100
>>> query mode: simple
>>> number of clients: 100
>>> number of threads: 100
>>> maximum number of tries: 1
>>> duration: 600 s
>>> number of transactions actually processed: 551984376
>>> number of failed transactions: 0 (0.000%)
>>> latency average = 0.109 ms
>>> initial connection time = 23.569 ms
>>> tps = 920001.842189 (without initial connection time)
>>
>> *With Trim :*
>>
>>> $./pgbench testc -U t1 -S -c 100 -j 100 -T 600
>>> pgbench (18devel)
>>> starting vacuum...end.
>>> transaction type: 
>>> scaling factor: 100
>>> query mode: simple
>>> number of clients: 100
>>> number of threads: 100
>>> maximum number of tries: 1
>>> duration: 600 s
>>> number of transactions actually processed: 470690787
>>> number of failed transactions: 0 (0.000%)
>>> latency average = 0.127 ms
>>> initial connection time = 23.632 ms
>>> tps = 784511.901558 (without initial connection time)
>>
>>
>
> --
> Regards,
> Rafia Sabih
>


v4-0001-Trim-the-free-heap-Memory.patch
Description: Binary data

Re: query_id, pg_stat_activity, extended query protocol

2024-09-11 Thread Sami Imseih

> After sleeping on it, I'd tend to slightly favor the last option in
> the back-branches and the second option on HEAD where we reduce the
> number of report calls. This way, we are a bit more careful in
>released branches by being more aggressive in reporting the query ID.

I agree with this because it will safely allow us to backpatch this
fix. 

> The tests in pg_stat_statements are one part I'm pretty sure is one
> good way forward. It is not perfect, but with the psql meta-commands

I played around with BackgrounsPsql. It works and gives us more flexibility
in testing, but I think the pg_stat_statements test are good enough for this
purpose. 

My only concern is this approach tests core functionality ( reporting of 
queryId )
in the tests of a contrib module ( pg_stat_statements ). Is that a valid
concern?

> Perhaps. I'd need to think through this one. Let's do things in
> order and see about the reports for the bind/execute messages, first,
> please?

Sure, that is fine.


Regards,

Sami

Re: Add memory/disk usage for WindowAgg nodes in EXPLAIN

> On Thu, 12 Sept 2024 at 14:04, Tatsuo Ishii  wrote:
>> Are you going to push the changes to tuplestore.c anytime soon? I
>> would like to rebase my patch[1] but the patch could be affected by
>> the tuplestore API change.
> 
> Ok, I'll look at that.

Thanks.

I had thought you were taking care of writing the patch.

Sorry, I should have asked you first if you are going to write the API
change patch.
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp

Re: Extract numeric filed in JSONB more effectively

2024-09-11 Thread Andy Fan



Hello David,

  Thanks for checking this!
  
> There's a lot of complexity in the v18 patch that I don't understand
> the need for.
>
> I imagined you'd the patch should create a SupportRequestSimplify
> support function for jsonb_numeric() that checks if the input
> expression is an OpExpr with funcid of jsonb_object_field().  All you
> do then is ditch the cast and change the OpExpr to call a new function
> named jsonb_object_field_numeric() which returns the val.numeric
> directly.  Likely the same support function could handle jsonb casts
> to other types too, in which case you'd just call some other function,
> e.g jsonb_object_field_timestamp() or jsonb_object_field_boolean().

Basically yes. The reason complexity comes when we many operators we
want to optimize AND my patch I want to reduce the number of function
created. 

The optimized functions and operators includes:
1. jsonb_object_field / ->
2. jsonb_array_element / ->
3. jsonb_extract_path / #>
4. jsonb_path_query
5. jsonb_path_query_first

   
> ..., in which case you'd just call some other function,
> e.g jsonb_object_field_timestamp() or jsonb_object_field_boolean().

This works, but We need to create 2 functions for each operator. In the
patched case, we have 5 operators, so we need to create 10 functions.

op[1,2,3,4,5]_bool
op[1,2,3,4,5]_numeric.

Within the start / finish function, we need to create *7* functions.

op[1,2,3,4,5]_start :  return the "JsonbVaue".

jsonb_finish_numeric:  convert jsonbvalue to numeric
jsonb_finish_bool   :  convert jsonbvalue to bool.

I think the above is the major factor for the additional complexity. 

Some other factors contribute to complexity a bit.

1. we also have jsonb_int{2,4,8}/float{4,8} in pg_proc for '->'
   operator, not only numeric.
2. user may use OpExpr, like (jb->'x')::numeric, user may also use
   FuncExpr, like (jsonb_object_field(a, 'x'))::numeric. 


-- 
Best Regards
Andy Fan

Re: query_id, pg_stat_activity, extended query protocol

On Wed, Sep 11, 2024 at 09:41:58PM -0500, Sami Imseih wrote:
>> The tests in pg_stat_statements are one part I'm pretty sure is one
>> good way forward. It is not perfect, but with the psql meta-commands
> 
> I played around with BackgrounsPsql. It works and gives us more flexibility
> in testing, but I think the pg_stat_statements test are good enough for this
> purpose. 
> 
> My only concern is this approach tests core functionality ( reporting of 
> queryId )
> in the tests of a contrib module ( pg_stat_statements ). Is that a valid
> concern?

Do you think that we'd better replace the calls reporting the query ID
in execMain.c by some assertions on HEAD?  This won't work for
ExecutorStart() because PREPARE statements (or actually EXECUTE,
e.g. I bumped on that yesterday but I don't recall which one) would
blow up on that with compute_query_id enabled.  We could do something
like that in ExecutorRun() at least as that may be helpful for
extensions?  An assertion would be like:
Assert(!IsQueryIdEnabled() || pgstat_get_my_query_id() != 0);

ExecutorFinish() and ExecutorEnd() are not that mandatory, so there's
a risk that this causes the backend to complain because a planner or
post-analyze hook decides to force the hand of the backend entry in an
extension.  With such checks, we're telling them to just not do that.
So your point would be to force this rule within the core executor on
HEAD?  I would not object to that in case we're missing more spots
with the extended query protocol, actually.  That would help us detect
cases where we're still missing the query ID to be set and the
executor should know about that.  The execute/fetch has been missing
for years without us being able to detect it automatically.

Note that I'm not much worried about the dependency with
pg_stat_statements.  We already rely on it for query jumbling
normalization for some parse node patterns like DISCARD, and query
jumbling requires query IDs to be around.  So that's not new.
--
Michael

signature.asc
Description: PGP signature

Re: Accept invalidation messages before the query starts inside a transaction

2024-09-11 Thread Andrey M. Borodin

> On 12 Sep 2024, at 00:50, Andrei Lepikhov  wrote:
> 
> It happens because of documented behaviour [1], which doesn't guarantee 
> isolation levels for internal access to the system catalogues. 

As far as I understood you are proposing not isolation guaranties, but allowed 
isolation anomaly.
I think we do not guaranty anomalies.

Best regards, Andrey Borodin.

Re: Add memory/disk usage for WindowAgg nodes in EXPLAIN

On Thu, 12 Sept 2024 at 14:42, Tatsuo Ishii  wrote:
> Sorry, I should have asked you first if you are going to write the API
> change patch.

I pushed a patch to change the API.

David

Re: Add memory/disk usage for WindowAgg nodes in EXPLAIN