date:20240627

Hi Alexander,

On Wed, Jun 26, 2024 at 8:00 PM Alexander Lakhin  wrote:
>
> Hello,
>
> I'm not sure I've chosen the most appropriate thread for reporting the
> issue, but maybe you would like to look at code comments related to
> SQL/JSON constructors:
>
>   * Transform JSON_ARRAY() constructor.
>   *
>   * JSON_ARRAY() is transformed into json[b]_build_array[_ext]() call
>   * depending on the output JSON format. The first argument of
>   * json[b]_build_array_ext() is absent_on_null.
>
>
>   * Transform JSON_OBJECT() constructor.
>   *
>   * JSON_OBJECT() is transformed into json[b]_build_object[_ext]() call
>   * depending on the output JSON format. The first two arguments of
>   * json[b]_build_object_ext() are absent_on_null and check_unique.
>
> But the referenced functions were removed at [1]; Nikita Glukhov wrote:
> > I have removed json[b]_build_object_ext() and json[b]_build_array_ext().
>
> (That thread seems too old for the current discussion.)
>
> Also, a comment above transformJsonObjectAgg() references
> json[b]_objectagg[_unique][_strict](key, value), but I could find
> json_objectagg() only.
>
> [1] 
> https://www.postgresql.org/message-id/be40362b-7821-7422-d33f-fbf1c61bb3e3%40postgrespro.ru

Thanks for the report.  Yeah, those comments that got added in
7081ac46ace are obsolete.

Attached is a patch to fix that.  Should be back-patched to v16.

-- 
Thanks, Amit Langote


v1-0001-SQL-JSON-Fix-some-obsolete-comments.patch
Description: Binary data

Re: pgsql: Add more SQL/JSON constructor functions

2024-06-27 Thread jian he

On Thu, Jun 27, 2024 at 7:48 PM Amit Langote  wrote:
> >
> > I've attempted that in the attached 0001, which removes
> > JsonExpr.coercion_expr and a bunch of code around it.
> >
> > 0002 is now the original patch minus the changes to make
> > JSON_EXISTS(), JSON_QUERY(), and JSON_VALUE() behave as we would like,
> > because the changes in 0001 covers them. The changes for JsonBehavior
> > expression coercion as they were in the last version of the patch are
> > still needed, but I decided to move those into 0001 so that the
> > changes for query functions are all in 0001 and those for constructors
> > in 0002.  It would be nice to get rid of that coerce_to_target_type()
> > call to coerce the "behavior expression" to RETURNING type, but I'm
> > leaving that as a task for another day.
>
> Updated 0001 to remove outdated references, remove some more unnecessary code.
>

i found some remaining references of "coercion_expr" should be removed.

src/include/nodes/primnodes.h
/* JsonExpr's collation, if coercion_expr is NULL. */

src/include/nodes/execnodes.h
/*
* Address of the step to coerce the result value of jsonpath evaluation
* to the RETURNING type using JsonExpr.coercion_expr.  -1 if no coercion
* is necessary or if either JsonExpr.use_io_coercion or
* JsonExpr.use_json_coercion is true.
*/
int jump_eval_coercion;

src/backend/jit/llvm/llvmjit_expr.c
/* coercion_expr code */
LLVMPositionBuilderAtEnd(b, b_coercion);
if (jsestate->jump_eval_coercion >= 0)
LLVMBuildBr(b, opblocks[jsestate->jump_eval_coercion]);
else
LLVMBuildUnreachable(b);

src/backend/executor/execExprInterp.c
/*
 * Checks if an error occurred either when evaluating JsonExpr.coercion_expr or
 * in ExecEvalJsonCoercion().  If so, this sets JsonExprState.error to trigger
 * the ON ERROR handling steps.
 */
void
ExecEvalJsonCoercionFinish(ExprState *state, ExprEvalStep *op)
{
}

if (jbv == NULL)
{
/* Will be coerced with coercion_expr, if any. */
*op->resvalue = (Datum) 0;
*op->resnull = true;
}

src/backend/executor/execExpr.c
/*
* Jump to coerce the NULL using coercion_expr if present.  Coercing NULL
* is only interesting when the RETURNING type is a domain whose
* constraints must be checked.  jsexpr->coercion_expr containing a
* CoerceToDomain node must have been set in that case.
*/

/*
* Jump to coerce the NULL using coercion_expr if present.  Coercing NULL
* is only interesting when the RETURNING type is a domain whose
* constraints must be checked.  jsexpr->coercion_expr containing a
* CoerceToDomain node must have been set in that case.
*/

Re: Doc Rework: Section 9.16.13 SQL/JSON Query Functions

Hi David,

On Tue, Jun 25, 2024 at 3:47 PM David G. Johnston
 wrote:
>
> Hey!
>
> Lots of SQL/JSON threads going about. This one is less about technical 
> correctness and more about usability of the documentation.  Though in writing 
> this I am finding some things that aren't quite clear.  I'm going to come 
> back with those on a follow-on post once I get a chance to make my second 
> pass on this.  But for the moment just opening it up to a content and 
> structure review.
>
> Please focus on the text changes.  It passes "check-docs" but I still need to 
> work on layout and stuff in html (markup, some more links).
>
> Thanks!
>
> David J.
>
> p.s. v1 exists here (is just the idea of using basically variable names in 
> the function signature and minimizing direct syntax in the table);
>
> https://www.postgresql.org/message-id/CAKFQuwbYBvUZasGj_ZnfXhC2kk4AT%3DepwGkNd2%3DRMMVXkfTNMQ%40mail.gmail.com

Thanks for writing the patch.  I'll take a look at this next Monday.

-- 
Thanks, Amit Langote

Re: SQL/JSON query functions context_item doc entry and type requirement

2024-06-27 Thread David G. Johnston

On Thursday, June 20, 2024, David G. Johnston 
wrote:

>
>> >
>> > > As for table 9.16.3 - it is unwieldy already.  Lets try and make the
>> core syntax shorter, not longer.  We already have precedence in the
>> subsequent json_table section - give each major clause item a name then
>> below the table define the syntax and meaning for those names.  Unlike in
>> that section - which probably should be modified too - context_item should
>> have its own description line.
>> >
>> > I had posted a patch a little while ago at [1] to render the syntax a
>> > bit differently with each function getting its own syntax synopsis.
>> > Resending it here; have addressed Jian He's comments.
>> >
>> > --
>>
>
> I was thinking more like:
>
> diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
> index c324906b22..b9d157663a 100644
> --- a/doc/src/sgml/func.sgml
> +++ b/doc/src/sgml/func.sgml
> @@ -18692,8 +18692,10 @@ $.* ? (@ like_regex "^\\d+$")
>
>  json_exists
>  json_exists (
> -context_item,
> path_expression 
> PASSING { value
> AS varname } ,
> ...
> - { TRUE | FALSE
> | UNKNOWN | ERROR } ON
> ERROR )
> +context_item,
> +path_expression
> +variable_definitions
> +on_error_boolean)
>  empty semantics should be the same as json_query.
>
>>
>>
The full first draft patch for this is here:

https://www.postgresql.org/message-id/cakfquwznxnhupk44zdf7z8qzec1aof10aa9twvbu5cmheke...@mail.gmail.com

David J.

Re: SQL/JSON query functions context_item doc entry and type requirement

On Thu, Jun 27, 2024 at 9:01 PM Amit Langote  wrote:
> On Sat, Jun 22, 2024 at 6:39 PM jian he  wrote:
> > On Fri, Jun 21, 2024 at 8:18 PM Amit Langote  
> > wrote:
> > >
> > > >> JSON_VALUE on error, on empty semantics should be the same as 
> > > >> json_query.
> > > >> like:
> > > >> [ { ERROR | NULL | EMPTY { [ ARRAY ] | OBJECT } | DEFAULT expression }
> > > >> ON EMPTY ]
> > > >> [ { ERROR | NULL | EMPTY { [ ARRAY ] | OBJECT } | DEFAULT expression }
> > > >> ON ERROR ])
> > > >>
> > > >> examples:
> > > >> select JSON_value(jsonb '[]' , '$'  empty array on error);
> > > >> select JSON_value(jsonb '[]' , '$'  empty object on error);
> > > >
> > > > Again the documented behavior seems to make sense though and the 
> > > > ability to specify empty in the value function seems like a bug.  If 
> > > > you really want an empty array or object you do have access to default. 
> > > >  The reason json_query provides for an empty array/object is that it is 
> > > > already expecting to produce an array (object seems a little odd).
> > > >
> > > > I agree our docs and code do not match which needs to be fixed, ideally 
> > > > in the direction of the standard which I'm guessing our documentation 
> > > > is based off of.  But let's not go off of my guess.
> > >
> > > Oops, that is indeed not great and, yes, the problem is code not
> > > matching the documentation, the latter of which is "correct".
> > >
> > > Basically, the grammar allows specifying any of the all possible ON
> > > ERROR/EMPTY behavior values irrespective of the function, so parse
> > > analysis should be catching and flagging an attempt to specify
> > > incompatible value for a given function, which it does not.
> > >
> > > I've attached a patch to fix that, which I'd like to push before
> > > anything else we've been discussing.
> > >
> >
> > + errcode(ERRCODE_SYNTAX_ERROR),
> > + errmsg("invalid ON ERROR behavior"),
> > + errdetail("Only ERROR, NULL, EMPTY [ ARRAY | OBJECT }, or DEFAULT
> >  is allowed in ON ERROR for JSON_QUERY()."),
> > + parser_errposition(pstate, func->on_error->location));
> >
> > `EMPTY [ ARRAY | OBJECT }` seems not correct,
> > maybe just EMPTY, EMPTY ARRAY, EMPTY OBJECT.
> > (apply to other places)
>
> Or EMPTY [ ARRAY ], EMPTY OBJECT
>
> > `DEFAULT `
> > `DEFAULT ` or just `DEFAULT expression` would be more correct?
> > (apply to other places)
>
> "DEFAULT expression" sounds good.
>
> > I think we should make json_query, json_value on empty, on error
> > behave the same way.
> > otherwise, it will have consistency issues for scalar jsonb.
> > for example, we should expect the following two queries to  return the
> > same result?
> > SELECT * FROM JSON_query(jsonb '1', '$.a' returning jsonb empty on empty);
> > SELECT * FROM JSON_value(jsonb '1', '$.a' returning jsonb empty on empty);
> >
> > Also the json_table function will call json_value or json_query,
> > make these two functions on error, on empty behavior the same can
> > reduce unintended complexity.
> >
> > So based on your
> > patch(v1-0001-SQL-JSON-Disallow-incompatible-values-in-ON-ERROR.patch)
> > and the above points, I have made some changes, attached.
> > it will make json_value, json_query not allow {true | false | unknown
> > } on error, {true | false | unknown } on empty.
> > json_table error message deal the same way as
> > b4fad46b6bc8a9bf46ff689bcb1bd4edf8f267af
>
> Here is an updated patch that I think takes care of these points.
>
> > BTW,
> > i found one JSON_TABLE document deficiency
> > [ { ERROR | NULL | EMPTY { ARRAY | OBJECT } | DEFAULT
> > expression } ON EMPTY ]
> > [ { ERROR | NULL | EMPTY { ARRAY | OBJECT } | DEFAULT
> > expression } ON ERROR ]
> >
> > it should be
> >
> > [ { ERROR | NULL | EMPTY { [ARRAY] | OBJECT } | DEFAULT
> > expression } ON EMPTY ]
> > [ { ERROR | NULL | EMPTY { [ARRAY] | OBJECT } | DEFAULT
> > expression } ON ERROR ]
>
> You're right.  Fixed.
>
> Also, I noticed that the grammar allows ON EMPTY in JSON_TABLE EXISTS
> columns which is meaningless because JSON_EXISTS() doesn't have a
> corresponding ON EMPTY clause.  Fixed grammar to prevent that in the
> attached 0002.

I've pushed this for now to close out the open item.

I know there's some documentation improvement work left to do [1],
which I'll try to find some time for next week.

-- 
Thanks, Amit Langote

[1] 
https://www.postgresql.org/message-id/CAKFQuwbYBvUZasGj_ZnfXhC2kk4AT%3DepwGkNd2%3DRMMVXkfTNMQ%40mail.gmail.com

Re: race condition in pg_class

Noah Misch  writes:
> Pushed.  Buildfarm member prion is failing the new inplace-inval.spec, almost
> surely because prion uses -DCATCACHE_FORCE_RELEASE and inplace-inval.spec is
> testing an extant failure to inval a cache entry.  Naturally, inexorable inval
> masks the extant bug.  Ideally, I'd just skip the test under any kind of cache
> clobber option.  I don't know a pleasant way to do that, so these are
> known-feasible things I'm considering:

> 1. Neutralize the test in all branches, probably by having it just not report
>the final answer.  Undo in the later fix patch.

> 2. v14+ has pg_backend_memory_contexts.  In the test, run some plpgsql that
>uses heuristics on that to deduce whether caches are getting released.
>Have a separate expected output for the cache-release scenario.  Perhaps
>also have the test treat installcheck like cache-release, since
>installcheck could experience sinval reset with similar consequences.
>Neutralize the test in v12 & v13.

> 3. Add a test module with a C function that reports whether any kind of cache
>clobber is active.  Call it in this test.  Have a separate expected output
>for the cache-release scenario.

> Preferences or other ideas?  I'm waffling between (1) and (2).  I'll give it
> more thought over the next day.

I'd just go for (1).  We were doing fine without this test case.
I can't see expending effort towards hiding its result rather
than actually fixing anything.

regards, tom lane

Re: race condition in pg_class

2024-06-27 Thread Noah Misch

On Fri, Jun 21, 2024 at 02:28:42PM -0700, Noah Misch wrote:
> On Thu, Jun 13, 2024 at 05:35:49PM -0700, Noah Misch wrote:
> > I think the attached covers all comments to date.  I gave everything v3, but
> > most patches have just a no-conflict rebase vs. v2.  The exceptions are
> > inplace031-inj-wait-event (implements the holding from that branch of the
> > thread) and inplace050-tests-inj (updated to cooperate with inplace031).  
> > Much
> > of inplace031-inj-wait-event is essentially s/Extension/Custom/ for the
> > infrastructure common to the two custom wait event types.
> 
> Starting 2024-06-27, I'd like to push
> inplace080-catcache-detoast-inplace-stale and earlier patches, self-certifying
> them if needed.  Then I'll submit the last three to the commitfest.  Does
> anyone want me to delay that step?

Pushed.  Buildfarm member prion is failing the new inplace-inval.spec, almost
surely because prion uses -DCATCACHE_FORCE_RELEASE and inplace-inval.spec is
testing an extant failure to inval a cache entry.  Naturally, inexorable inval
masks the extant bug.  Ideally, I'd just skip the test under any kind of cache
clobber option.  I don't know a pleasant way to do that, so these are
known-feasible things I'm considering:

1. Neutralize the test in all branches, probably by having it just not report
   the final answer.  Undo in the later fix patch.

2. v14+ has pg_backend_memory_contexts.  In the test, run some plpgsql that
   uses heuristics on that to deduce whether caches are getting released.
   Have a separate expected output for the cache-release scenario.  Perhaps
   also have the test treat installcheck like cache-release, since
   installcheck could experience sinval reset with similar consequences.
   Neutralize the test in v12 & v13.

3. Add a test module with a C function that reports whether any kind of cache
   clobber is active.  Call it in this test.  Have a separate expected output
   for the cache-release scenario.

Preferences or other ideas?  I'm waffling between (1) and (2).  I'll give it
more thought over the next day.

Thanks,
nm

Re: speed up a logical replica setup

2024-06-27 Thread Amit Kapila

On Wed, Jun 26, 2024 at 11:35 AM Hayato Kuroda (Fujitsu)
 wrote:
>
> Thanks for giving comment! 0001 was modified per suggestions.
>
> > Yeah, it is a good idea to add a new option for two_phase but that
> > should be done in the next version. For now, I suggest updating the
> > docs and probably raising a warning (if max_prepared_transactions !=
> > 0) as suggested by Noah. This WARNING is useful because one could
> > expect that setting max_prepared_transactions != 0 means apply will
> > happen at prepare time after the subscriber is created by this tool.
> > The WARNING will be useful even if we support two_phase option as the
> > user may have set the non-zero value of max_prepared_transactions but
> > didn't use two_phase option.
>
> I also think it should be tunable, in PG18+. 0002 adds a description in doc.
> Also, this executable warns if the max_prepared_transactions != 0 on the
> publisher.
>

I have slightly adjusted the doc update and warning message for the
0002 patch. Please see attached and let me know what do you think.

-- 
With Regards,
Amit Kapila.


v3-0001-pg_createsubscriber-Warn-the-two-phase-is-disable.patch
Description: Binary data

Re: stale comments about fastgetattr and heap_getattr

2024-06-27 Thread Michael Paquier

On Fri, Jun 28, 2024 at 11:06:05AM +0700, Stepan Neretin wrote:
> Hi! Looks good to me. Please, register it in CF.
> Best regards, Stepan Neretin.

No need for that.  I've looked at the patch, and you are right so I
have applied it now on HEAD.
--
Michael


signature.asc
Description: PGP signature

Parallel heap vacuum

2024-06-27 Thread Masahiko Sawada

Hi all,

The parallel vacuum we have today supports only for index vacuuming.
Therefore, while multiple workers can work on different indexes in
parallel, the heap table is always processed by the single process.
I'd like to propose $subject, which enables us to have multiple
workers running on the single heap table. This would be helpful to
speedup vacuuming for tables without indexes or tables with
INDEX_CLENAUP = off.

I've attached a PoC patch for this feature. It implements only
parallel heap scans in lazyvacum. We can extend this feature to
support parallel heap vacuum as well in the future or in the same
patch.

# Overall idea (for parallel heap scan in lazy vacuum)

At the beginning of vacuum, we determine how many workers to launch
based on the table size like other parallel query operations. The
number of workers is capped by max_parallel_maitenance_workers. Once
we decided to use parallel heap scan, we prepared DSM to share data
among parallel workers and leader. The information include at least
the vacuum option such as aggressive, the counters collected during
lazy vacuum such as scanned_pages, vacuum cutoff such as VacuumCutoffs
and GlobalVisState, and parallel scan description.

Before starting heap scan in lazy vacuum, we launch parallel workers
and then each worker (and the leader) process different blocks. Each
worker does HOT-pruning on pages and collects dead tuple TIDs. When
adding dead tuple TIDs, workers need to hold an exclusive lock on
TidStore. At the end of heap scan phase, workers exit and the leader
will wait for all workers to exit. After that, the leader process
gather the counters collected by parallel workers, and compute the
oldest relfrozenxid (and relminmxid). Then if parallel index vacuum is
also enabled, we launch other parallel workers for parallel index
vacuuming.

When it comes to parallel heap scan in lazy vacuum, I think we can use
the table_block_parallelscan_XXX() family. One tricky thing we need to
deal with is that if the TideStore memory usage reaches the limit, we
stop the parallel scan, do index vacuum and table vacuum, and then
resume the parallel scan from the previous state. In order to do that,
in the patch, we store ParallelBlockTableScanWorker, per-worker
parallel scan state, into DSM so that different parallel workers can
resume the scan using the same parallel scan state.

In addition to that, since we could end up launching fewer workers
than requested, it could happen that some ParallelBlockTableScanWorker
data is used once and never be used while remaining unprocessed
blocks. To handle this case, in the patch, the leader process checks
at the end of the parallel scan if there is an uncompleted parallel
scan. If so, the leader process does the scan using worker's
ParallelBlockTableScanWorker data on behalf of workers.

# Discussions

I'm somewhat convinced the brief design of this feature, but there are
some points regarding the implementation we need to discuss.

In the patch, I extended vacuumparalle.c to support parallel table
scan (and vacuum in the future). So I was required to add some table
AM callbacks such as DSM size estimation, DSM initialization, and
actual table scans etc. We need to verify these APIs are appropriate.
Specifically, if we want to support both parallel heap scan and
parallel heap vacuum, do we want to add separate callbacks for them?
It could be overkill since such a 2-pass vacuum strategy is specific
to heap AM.

As another implementation idea, we might want to implement parallel
heap scan/vacuum in lazyvacuum.c while minimizing changes for
vacuumparallel.c. That way, we would not need to add table AM
callbacks. However, we would end up having duplicate codes related to
parallel operation in vacuum such as vacuum delays.

Also, we might need to add some functions to share GlobalVisState
among parallel workers, since GlobalVisState is a private struct.

Other points I'm somewhat uncomfortable with or need to be discussed
remain in the code with XXX comments.

# Benchmark results

* Test-1: parallel heap scan on the table without indexes

I created 20GB table, made garbage on the table, and run vacuum while
changing parallel degree:

create unlogged table test (a int) with (autovacuum_enabled = off);
insert into test select generate_series(1, 6); --- 20GB table
delete from test where a % 5 = 0;
vacuum (verbose, parallel 0) test;

Here are the results (total time and heap scan time):

PARALLEL 0: 21.99 s (single process)
PARALLEL 1: 11.39 s
PARALLEL 2:   8.36 s
PARALLEL 3:   6.14 s
PARALLEL 4:   5.08 s

* Test-2: parallel heap scan on the table with one index

I used a similar table to the test case 1 but created one btree index on it:

create unlogged table test (a int) with (autovacuum_enabled = off);
insert into test select generate_series(1, 6); --- 20GB table
create index on test (a);
delete from test where a % 5 = 0;
vacuum (verbose, parallel 0) test;

I've measured the total execution time as well a

Re: stale comments about fastgetattr and heap_getattr

2024-06-27 Thread Stepan Neretin

Hi! Looks good to me. Please, register it in CF.
Best regards, Stepan Neretin.

On Fri, Jun 28, 2024 at 10:05 AM Junwang Zhao  wrote:

> fastgetattr and heap_getattr are converted to inline functions
> in e27f4ee0a701, while some comments still referring them as macros.
>
> --
> Regards
> Junwang Zhao
>

Re: Injection point locking

2024-06-27 Thread Michael Paquier

On Wed, Jun 26, 2024 at 10:56:12AM +0900, Michael Paquier wrote:
> That's true, we could delay the release of the lock to happen just
> before a callback is run.

I am not sure what else we can do for the postmaster case for now, so
I've moved ahead with the concern regarding the existing locking
release delay when running a point, and pushed a patch for it.
--
Michael


signature.asc
Description: PGP signature

RE: Improve EXPLAIN output for multicolumn B-Tree Index

2024-06-27 Thread Masahiro.Ikeda

On Thu, 27 Jun 2024 at 22:02, Peter Geoghegan  wrote:
> Unfortunately, my patch will make the situation more complicated
> for your patch. I would like to resolve the tension between the
> two patches, but I'm not sure how to do that.

OK. I would like to understand more about your proposed patch. I
have also registered as a reviewer in the commitfests entry.

On 2024-06-28 07:40, Peter Geoghegan wrote:
> On Thu, Jun 27, 2024 at 4:46 PM Jelte Fennema-Nio  wrote:
>> I do think though that in addition to a "Skip Scan Filtered" count for
>> ANALYZE, it would be very nice to also get a "Skip Scan Skipped" count
>> (if that's possible to measure/estimate somehow). This would allow
>> users to determine how effective the skip scan was, i.e. were they
>> able to skip over large swaths of the index? Or did they skip overx
>> nothing because the second column of the index (on which there was no
>> filter) was unique within the table
> 
> Yeah, EXPLAIN ANALYZE should probably be showing something about
> skipping. That provides us with a way of telling the user what really
> happened, which could help when EXPLAIN output alone turns out to be
> quite misleading.
> 
> In fact, that'd make sense even today, without skip scan (just with
> the 17 work on nbtree SAOP scans). Even with regular SAOP nbtree index
> scans, the number of primitive scans is hard to predict, and quite
> indicative of what's really going on with the scan.

I agree as well.

Although I haven't looked on your patch yet, if it's difficult to know
how it can optimize during the planning phase, it's enough for me to just
show "Skip Scan Cond (or Non-Key Filter)". This is because users can
understand that inefficient index scans *may* occur.

If users want more detail, they can execute "EXPLAIN ANALYZE". This will
allow them to understand the execution effectively and determine if there
is any room to optimize the plan by looking at the counter of
"Skip Scan Filtered (or Skip Scan Skipped)".

In terms of the concept of EXPLAIN output, I thought that runtime partition
pruning is similar. "EXPLAIN without ANALYZE" only shows the possibilities and
"EXPLAIN ANALYZE" shows the actual results.

Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION

stale comments about fastgetattr and heap_getattr

2024-06-27 Thread Junwang Zhao

fastgetattr and heap_getattr are converted to inline functions
in e27f4ee0a701, while some comments still referring them as macros.

-- 
Regards
Junwang Zhao


0001-stale-comments-about-fastgetattr-and-heap_getattr.patch
Description: Binary data

Re: Doc: fix track_io_timing description to mention pg_stat_io

2024-06-27 Thread Hajime.Matsunaga

> > On Thu, Jun 27, 2024 at 5:06 AM  wrote:
> > >
> > > Hi,
> >
> > > pg_stat_io has I/O statistics that are collected when track_io_timing is
> > > enabled, but it is not mentioned in the description of track_io_timing.
> > > I think it's better to add a description of pg_stat_io for easy reference.
> > > What do you think?
> >
> > Seems quite reasonable to me given that track_wal_io_timing mentions
> > pg_stat_wal. I noticed that the sentence about track_io_timing in the
> > statistics collection configuration section [1] only mentions reads
> > and writes -- perhaps it should also mention extends and fsyncs?
> 
> Both suggestions look good to me. If what you said will be
> implemented, maybe track_wal_io_timing too should mention fsyncs?

Thank you both for your positive comments.
I also think your suggestions seem good.


Regards,
--
Hajime Matsunaga
NTT DATA Group Corporation

Re: ON ERROR in json_query and the like

On Wed, Jun 26, 2024 at 9:10 PM Amit Langote  wrote:
> On Sat, Jun 22, 2024 at 5:43 PM Amit Langote  wrote:
> > On Fri, Jun 21, 2024 at 8:00 PM Markus Winand  
> > wrote:
> > > So updating the three options:
> > > > 1. Disallow anything but jsonb for context_item (the patch I posted 
> > > > yesterday)
> > >
> > > * Non-conforming
> > > * patch available
> > >
> > > > 2. Continue allowing context_item to be non-json character or utf-8
> > > > encoded bytea strings, but document that any parsing errors do not
> > > > respect the ON ERROR clause.
> > >
> > > * Conforming by choosing IA050 to implement GR4: raise errors independent 
> > > of the ON ERROR clause.
> > > * currently committed.
> > >
> > > > 3. Go ahead and fix implicit casts to jsonb so that any parsing errors
> > > > respect ON ERROR (no patch written yet).
> > >
> > > * Conforming by choosing IA050 not to implement GR4: Parsing happens 
> > > later, considering the ON ERROR clause.
> > > * no patch available, not trivial
> > >
> > > I guess I’m the only one in favour of 3 ;) My remaining arguments are 
> > > that Oracle and Db2 (LUW) do it that way and also that it is IMHO what 
> > > users would expect. However, as 2 is also conforming (how could I miss 
> > > that?), proper documentation is a very tempting option.
> >
> > So, we should go with 2 for v17, because while 3 may be very
> > appealing, there's a risk that it might not get done in the time
> > remaining for v17.
> >
> > I'll post the documentation patch on Monday.
>
> Here's that patch, which adds this note after table 9.16.3. SQL/JSON
> Query Functions:
>
> +  
> +   
> +The context_item expression is converted to
> +jsonb by an implicit cast if the expression is not already 
> of
> +type jsonb. Note, however, that any parsing errors that 
> occur
> +during that conversion are thrown unconditionally, that is, are not
> +handled according to the (specified or implicit) ON
> ERROR
> +clause.
> +   
> +  

I have pushed this.

-- 
Thanks, Amit Langote

Re: Virtual generated columns

2024-06-27 Thread jian he

On Thu, May 23, 2024 at 1:23 AM Peter Eisentraut  wrote:
>
> On 29.04.24 10:23, Peter Eisentraut wrote:
> > Here is a patch set to implement virtual generated columns.
>
> > The main feature patch (0005 here) generally works but has a number of
> > open corner cases that need to be thought about and/or fixed, many of
> > which are marked in the code or the tests.  I'll continue working on
> > that.  But I wanted to see if I can get some feedback on the test
> > structure, so I don't have to keep changing it around later.

the test structure you made ( generated_stored.sql,
generated_virtual.sq) looks ok to me.
but do we need to reset the search_path at the end of
generated_stored.sql, generated_virtual.sql?

most of the test tables didn't use much storage,
maybe not necessary to clean up (drop the test table) at the end of sql files.

>
> So, I think this basically works now, and the things that don't work
> should be appropriately prevented.  So if someone wants to test this and
> tell me what in fact doesn't work correctly, that would be helpful.


in https://www.postgresql.org/docs/current/catalog-pg-attrdef.html
>>>
The catalog pg_attrdef stores column default values. The main
information about columns is stored in pg_attribute. Only columns for
which a default value has been explicitly set will have an entry here.
>>
didn't mention generated columns related expressions.
Do we need to add something here? maybe a separate issue?


+ /*
+ * TODO: This could be done, but it would need a different implementation:
+ * no rewriting, but still need to recheck any constraints.
+ */
+ if (attTup->attgenerated == ATTRIBUTE_GENERATED_VIRTUAL)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("ALTER TABLE / SET EXPRESSION is not supported for virtual
generated columns"),
+ errdetail("Column \"%s\" of relation \"%s\" is a virtual generated column.",
+   colName, RelationGetRelationName(rel;

minor typo, should be
+ errmsg("ALTER TABLE SET EXPRESSION is not supported for virtual
generated columns"),

insert/update/delete/merge returning have problems:
CREATE TABLE t2 (
a int ,
b int GENERATED ALWAYS AS (a * 2),
d int default 22);
insert into t2(a) select g from generate_series(1,10) g;

insert into t2 select 100 returning *, (select t2.b), t2.b = t2.a * 2;
update t2 set a = 12 returning *, (select t2.b), t2.b = t2.a * 2;
update t2 set a = 12 returning *, (select (select t2.b)), t2.b = t2.a * 2;
delete from t2 where t2.b = t2.a * 2 returning *, 1,((select t2.b));

currently all these query, error message is "unexpected virtual
generated column reference"
we expect above these query work?


issue with merge:
CREATE TABLE t0 (a int PRIMARY KEY, b int GENERATED ALWAYS AS (a * 2) VIRTUAL);
insert into t0(a) select g from generate_series(1,10) g;
MERGE INTO t0 t USING t0 AS s ON 2 * t.a = s.b WHEN MATCHED THEN
DELETE returning *;

the above query returns zero rows, but for stored generated columns it
will return 10 rows.

in  transformMergeStmt(ParseState *pstate, MergeStmt *stmt)
add
`qry->hasGeneratedVirtual = pstate->p_hasGeneratedVirtual;`
before
`assign_query_collations(pstate, qry);`
solve the problem.

Re: walsender.c comment with no context is hard to understand

2024-06-27 Thread Peter Smith

On Thu, Jun 27, 2024 at 3:44 PM Michael Paquier  wrote:
>
> On Wed, Jun 26, 2024 at 02:30:26PM +0530, vignesh C wrote:
> > On Mon, 3 Jun 2024 at 11:21, Peter Smith  wrote:
> >> Perhaps the comment should say something like it used to:
> >> /* Fail if there is not enough WAL available. This can happen during
> >> shutdown. */
> >
> > Agree with this, +1 for this change.
>
> That would be an improvement.  Would you like to send a patch with all
> the areas you think could stand for improvements?
> --

OK, I attached a patch equivalent of the suggestion in this thread.

==
Kind Regards,
Peter Smith.
Fujitsu Australia


v1-0001-Fix-walsender-comment.patch
Description: Binary data

Re: Improve EXPLAIN output for multicolumn B-Tree Index

2024-06-27 Thread Peter Geoghegan

On Thu, Jun 27, 2024 at 4:46 PM Jelte Fennema-Nio  wrote:
> On Thu, 27 Jun 2024 at 22:02, Peter Geoghegan  wrote:
> > It's also possible that we should just do something simple, like your
> > patch, even though technically it won't really be accurate in cases
> > where skip scan is used to good effect. Maybe showing the "default
> > working assumption" about how the scan keys/clauses will behave at
> > runtime is actually the right thing to do. Maybe I am just
> > overthinking it.
>
> IIUC, you're saying that your skip scan will improve the situation
> Masahiro describes dramatically in some/most cases.

"Most cases" seems likely to be overstating it. Overall, I doubt that
it makes sense to try to generalize like that.

The breakdown of the cases that we see in the field right now
(whatever it really is) is bound to be strongly influenced by the
current capabilities of Postgres. If something is intolerably slow,
then it just isn't tolerated. If something works adequately, then
users don't usually care why it is so.

> But it still won't
> be as good as a pure index "prefix" scan.

Typically, no, it won't be. But there's really no telling for sure.
The access patterns for a composite index on '(a, b)' with a qual
"WHERE b = 5" are identical to a qual explicitly written "WHERE a =
any() AND b = 5".

> If that's the case then I do think you're overthinking this a bit.
> Because then you'd still want to see this difference between the
> prefix-scan keys and the skip-scan keys. I think the main thing that
> the introduction of the skip scan changes is the name that we should
> show, e.g. instead of "Non-key Filter" we might want to call it "Skip
> Scan Cond"

What about cases where we legitimately have to vary our strategy
during the same index scan? We might very well be able to skip over
many leaf pages when scanning through a low cardinality subset of the
index (low cardinality in respect of a leading column 'a'). Then we
might find that there are long runs on leaf pages where no skipping is
possible.

I don't expect this to be uncommon. I do expect it to happen when the
optimizer wasn't particularly expecting it. Like when a full index
scan was the fastest plan anyway. Or when a skip scan wasn't quite as
good as expected, but nevertheless turned out to be the fastest plan.

> I do think though that in addition to a "Skip Scan Filtered" count for
> ANALYZE, it would be very nice to also get a "Skip Scan Skipped" count
> (if that's possible to measure/estimate somehow). This would allow
> users to determine how effective the skip scan was, i.e. were they
> able to skip over large swaths of the index? Or did they skip over
> nothing because the second column of the index (on which there was no
> filter) was unique within the table

Yeah, EXPLAIN ANALYZE should probably be showing something about
skipping. That provides us with a way of telling the user what really
happened, which could help when EXPLAIN output alone turns out to be
quite misleading.

In fact, that'd make sense even today, without skip scan (just with
the 17 work on nbtree SAOP scans). Even with regular SAOP nbtree index
scans, the number of primitive scans is hard to predict, and quite
indicative of what's really going on with the scan.

--
Peter Geoghegan

Re: Proposal: Document ABI Compatibility

2024-06-27 Thread David E. Wheeler

On Jun 27, 2024, at 17:48, Jeremy Schneider  wrote:

> Minor nit - misspelled “considerd"

Thank you, Jeremy. V3 attached.

Best,

David



v3-0001-Add-API-an-ABI-guidance-to-the-C-language-docs.patch
Description: Binary data

Inline non-SQL SRFs using SupportRequestSimplify

2024-06-27 Thread Paul Jungwirth


Hi Hackers,

Here is a proof-of-concept patch to inline set-returning functions (SRFs) besides those written in 
SQL. We already try to inline SQL-language functions,[1] but that means you must have a static SQL 
query. There is no way to get an inline-able query by dynamically building the sql in, say, plpgsql.


We also have a SupportRequestSimplify request type for functions that use SUPPORT to declare a 
support function, and it can replace the FuncExpr with an arbitrary nodetree.[2] I think this was 
intended for constant-substitution, but we can also use it to let functions generate dynamic SQL and 
then inline it. In this patch, if a SRF replaces itself with a Query node, then 
inline_set_returning_function will use that.


So far there are no tests or docs; I'm hoping to hear feedback on the idea 
before going further.

Here is my concrete use-case: I wrote a function to do a temporal semijoin,[3] and I want it to be 
inlined. There is a support function that builds the same SQL and lets Postgres parse it into a 
Query.[4] (In practice I would rewrite the main function in C too, so it could share the 
SQL-building code there, but this is just a POC.) If you build and install that extension on its 
`inlined` branch,[5] then you can do this:


```
\i bench.sql
explain select * from temporal_semijoin('employees', 'id', 'valid_at', 'positions', 'employee_id', 
'valid_at') j(id bigint, valid_at daterange);
explain select * from temporal_semijoin('employees', 'id', 'valid_at', 'positions', 'employee_id', 
'valid_at') j(id bigint, valid_at daterange) where j.id = 10::bigint;

```

Without this patch, you get `ERROR:  unrecognized node type: 58`. But with this patch you get these 
plans:


```
postgres=# explain select * from temporal_semijoin('employees', 'id', 'valid_at', 'positions', 
'employee_id', 'valid_at') j(id bigint, valid_at daterange);

  QUERY PLAN
--
 ProjectSet  (cost=4918.47..6177.06 rows=22300 width=40)
   ->  Hash Join  (cost=4918.47..6062.77 rows=223 width=53)
 Hash Cond: (employees.id = j.employee_id)
 Join Filter: (employees.valid_at && j.valid_at)
 ->  Seq Scan on employees  (cost=0.00..1027.39 rows=44539 width=21)
 ->  Hash  (cost=4799.15..4799.15 rows=9545 width=40)
   ->  Subquery Scan on j  (cost=4067.61..4799.15 rows=9545 
width=40)
 ->  HashAggregate  (cost=4067.61..4703.70 rows=9545 
width=40)
   Group Key: positions.employee_id
   Planned Partitions: 16
   ->  Seq Scan on positions  (cost=0.00..897.99 
rows=44099 width=21)
(11 rows)

postgres=# explain select * from temporal_semijoin('employees', 'id', 'valid_at', 'positions', 
'employee_id', 'valid_at') j(id bigint, valid_at daterange) where j.id = 10::bigint;
  QUERY PLAN 


--
 ProjectSet  (cost=0.56..9.22 rows=100 width=40)
   ->  Nested Loop  (cost=0.56..8.71 rows=1 width=53)
 ->  GroupAggregate  (cost=0.28..4.39 rows=1 width=40)
   ->  Index Only Scan using idx_positions_on_employee_id on positions 
(cost=0.28..4.36 rows=5 width=21)

 Index Cond: (employee_id = '10'::bigint)
 ->  Index Only Scan using employees_pkey on employees  
(cost=0.28..4.30 rows=1 width=21)
   Index Cond: ((id = '10'::bigint) AND (valid_at && 
(range_agg(positions.valid_at
(7 rows)

```

In particular I'm excited to see in the second plan that the predicate gets 
pushed into the subquery.

If it seems good to let people use SupportRequestSimplify to make their SRFs be inlineable, I'm 
happy to add tests and docs. We should really document the idea of inlined functions in general, so 
I'll do that too.


Another approach I considered is using a separate support request, e.g. SupportRequestInlineSRF, and 
just calling it from inline_set_returning_function. I didn't like having two support requests that 
did almost exactly the same thing. OTOH my current approach means you'll get an error if you do this:


```
postgres=# select temporal_semijoin('employees', 'id', 'valid_at', 'positions', 'employee_id', 
'valid_at');

ERROR:  unrecognized node type: 66
```

I'll look into ways to fix that.

I think SupportRequestSimplify is a really cool feature. It is nearly like 
having macros.
I'm dreaming about other ways I can (ab)use it. Just making inline-able SRFs has many applications. 
From my own client work, I could use this for a big permissions query or a query with complicated 
pricing logic.


The sad part though is that SUPPORT functions must be written in C. That means few people will use 
them, especially these days when so many are in the cloud. Since they

Re: Proposal: Document ABI Compatibility

2024-06-27 Thread Jeremy Schneider

On 6/26/24 12:23 PM, David E. Wheeler wrote:
> On Jun 26, 2024, at 15:20, David E. Wheeler  wrote:
> 
>> CF: https://commitfest.postgresql.org/48/5080/
>> PR: https://github.com/theory/postgres/pull/6
> 
> Aaaand v2 without the unnecessary formatting of unrelated documentation 🤦🏻‍♂️.

Minor nit - misspelled "considerd"

-Jeremy


-- 
http://about.me/jeremy_schneider

Re: Improve EXPLAIN output for multicolumn B-Tree Index

On Thu, 27 Jun 2024 at 22:02, Peter Geoghegan  wrote:
> It's also possible that we should just do something simple, like your
> patch, even though technically it won't really be accurate in cases
> where skip scan is used to good effect. Maybe showing the "default
> working assumption" about how the scan keys/clauses will behave at
> runtime is actually the right thing to do. Maybe I am just
> overthinking it.

IIUC, you're saying that your skip scan will improve the situation
Masahiro describes dramatically in some/most cases. But it still won't
be as good as a pure index "prefix" scan.

If that's the case then I do think you're overthinking this a bit.
Because then you'd still want to see this difference between the
prefix-scan keys and the skip-scan keys. I think the main thing that
the introduction of the skip scan changes is the name that we should
show, e.g. instead of "Non-key Filter" we might want to call it "Skip
Scan Cond"

I do think though that in addition to a "Skip Scan Filtered" count for
ANALYZE, it would be very nice to also get a "Skip Scan Skipped" count
(if that's possible to measure/estimate somehow). This would allow
users to determine how effective the skip scan was, i.e. were they
able to skip over large swaths of the index? Or did they skip over
nothing because the second column of the index (on which there was no
filter) was unique within the table

Re: POC, WIP: OR-clause support for indexes

2024-06-27 Thread Alena Rybakina

Tobe honest,I've alreadystartedwritingcodetodothis,butI'm facedwitha 
misunderstandingof howto correctlycreatea 
conditionfor"OR"expressionsthatare notsubjectto transformation.


For example,the expressions b=1in the query below:

alena@postgres=# explain select * from x where ( (a =5 or a=4) and a = 
ANY(ARRAY[5,4])) or (b=1); QUERY PLAN 
-- 
Seq Scan on x (cost=0.00..123.00 rows=1 width=8) Filter: a = 5) OR 
(a = 4)) AND (a = ANY ('{5,4}'::integer[]))) OR (b = 1)) (2 rows)


I see that two expressions have remained unchanged and it only works 
for "AND" binary operations.


But I think it might be worth applying this together, where does the 
optimizer generate indexes (build_paths_for_OR function)?




Sorry, it works) I needed to create one more index for b column.

Just in case, I gave an example of a complete case, otherwise it might 
not be entirely clear:


alena@postgres=# create table x (a int, b int);
CREATE TABLE
alena@postgres=# create index a_idx on x(a);
    insert into x select id,id from 
generate_series(1, 5000) as id;

CREATE INDEX
INSERT 0 5000
alena@postgres=# analyze;
ANALYZE

alena@postgres=# explain select * from x where ( (a =5 or a=4) and a = 
ANY(ARRAY[5,4])) or (b=1); QUERY PLAN 
-- 
Seq Scan on x (cost=0.00..123.00 rows=1 width=8) Filter: a = 5) OR 
(a = 4)) AND (a = ANY ('{5,4}'::integer[]))) OR (b = 1)) (2 rows)


alena@postgres=# create index b_idx on x(b);

CREATE INDEX

alena@postgres=# explain select * from x where ( (a =5 or a=4) and a = 
ANY(ARRAY[5,4]))  or (b=1);

    QUERY PLAN
--
 Bitmap Heap Scan on x  (cost=12.87..21.68 rows=1 width=8)
   Recheck Cond: ((a = ANY ('{5,4}'::integer[])) OR (b = 1))
   ->  BitmapOr  (cost=12.87..12.87 rows=3 width=0)
 ->  Bitmap Index Scan on a_idx  (cost=0.00..8.58 rows=2 width=0)
   Index Cond: (a = ANY ('{5,4}'::integer[]))
 ->  Bitmap Index Scan on b_idx  (cost=0.00..4.29 rows=1 width=0)
   Index Cond: (b = 1)
(7 rows)

--
Regards,
Alena Rybakina
Postgres Professional:http://www.postgrespro.com
The Russian Postgres Company

Re: Improve EXPLAIN output for multicolumn B-Tree Index

2024-06-27 Thread Peter Geoghegan

On Fri, Jun 21, 2024 at 3:12 AM wrote:
> Regarding the multicolumn B-Tree Index, I'm considering
> if we can enhance the EXPLAIN output. There have been requests
> for this from our customer.

I agree that this is a real problem -- I'm not surprised to hear that
your customer asked about it.

In the past, we've heard complaints about this problem from Markus Winand, too:

https://use-the-index-luke.com/sql/explain-plan/postgresql/filter-predicates

As it happens I have been thinking about this problem a lot recently.
Specifically the user-facing aspects, what we show in EXPLAIN. It is
relevant to my ongoing work on skip scan:

https://commitfest.postgresql.org/48/5081/
https://www.postgresql.org/message-id/flat/cah2-wzmn1yslzoggjaqzdn1stsg_y8qp__vggtapayxjp+g...@mail.gmail.com

Unfortunately, my patch will make the situation more complicated for
your patch. I would like to resolve the tension between the two
patches, but I'm not sure how to do that.

If you look at the example query that I included in my introductory
email on the skip scan thread (the query against the sales_mdam_paper
table), you'll see that my patch makes it go much faster. My patch
will effectively "convert" nbtree scan keys that would traditionally
have to use non-index-bound conditions/filter predicates, into
index-bound conditions/access predicates. This all happens at runtime,
during nbtree preprocessing (not during planning).

This may mean that your patch's approach of determining which
columns/scan keys are in which category (bound vs. non-bound) cannot
rely on its current approach of placing each type of clause into one
of two categories inside btcostestimate() -- the view that we see from
btcostestimate() will be made less authoritative by skip scan. What
actually matters in what happens during nbtree preprocessing, inside
_bt_preprocess_keys().

Unfortunately, this is even more complicated than it sounds. It would
be a good idea if we moved _bt_preprocess_keys() to plan time, so that
btcostestimate() operated off of authoritative information, rather
than independently figuring out the same details for the purposes of
costing. We've talked about this before, even [1]. That way your patch
could just work off of this authoritative information. But even that
doesn't necessarily fix the problem.

Note that the skip scan patch makes _bt_preprocess_keys()
*indiscriminately* "convert" *all* scan keys to index bound conditions
-- at least where that's possible at all. There are minor
implementation restrictions that mean that we can't always do that.
But overall, the patch more or less eliminates non-bound index
conditions. That is, it'll be rare to non-existent for nbtree to fail
to mark *any* scan key as SK_BT_REQFWD/SK_BT_REQBKWD. Technically
speaking, non-bound conditions mostly won't exist anymore.

Of course, this doesn't mean that the problem that your patch is
solving will actually go away. I fully expect that the skip scan patch
will merely make some scan keys "required-by-scan/index bound
condition scan keys in name only". Technically they won't be the
problematic kind of index condition, but that won't actually be true
in any practical sense. Because users (like your customer) will still
get full index scans, and be surprised, just like today.

As I explain in my email on the skip scan thread, I believe that the
patch's aggressive approach to "converting" scan keys is an advantage.
The amount of skipping that actually takes place should be decided
dynamically, at runtime. It is a decision that should be made at the
level of individual leaf pages (or small groups of leaf pages), not at
the level of the whole scan. The distinction between index bound
conditions and non-bound conditions becomes much more "squishy", which
is mostly (though not entirely) a good thing.

I really don't know what to do about this. As I said, I agree with the
general idea of this patch -- this is definitely a real problem. And,
I don't pretend that my skip scan patch will actually define the
problem out of existence (except perhaps in those cases that it
actually makes it much faster). Maybe we could make a guess (based on
statistics) whether or not any skip attributes will leave the
lower-order clauses as useful index bound conditions at runtime. But I
don't know...that condition is actually a "continuous" condition now
-- it is not a strict dichotomy (it is not either/or, but rather a
question of degree, perhaps on a scale of 0.0 - 1.0).

It's also possible that we should just do something simple, like your
patch, even though technically it won't really be accurate in cases
where skip scan is used to good effect. Maybe showing the "default
working assumption" about how the scan keys/clauses will behave at
runtime is actually the right thing to do. Maybe I am just
overthinking it.

[1] https://www.postgresql.org/message-id/2587523.1647982...@sss.pgh.pa.us
-- the final full paragraph mentions moving _bt_preprocess_keys() into
the planner
--
Peter G

Re: Is missing LOGIN Event on Trigger Firing Matrix ?

2024-06-27 Thread David G. Johnston

On Thu, Jun 27, 2024 at 12:39 PM Andrew Dunstan  wrote:

> On 2024-06-27 Th 2:40 PM, Marcos Pegoraro wrote:
>
> create event trigger ... on login is now available but it is not shown on
> DOCs or is it in another page ?
>
> https://www.postgresql.org/docs/devel/event-trigger-matrix.html
> doesn't have it, should be there ?
>
>
> It's not triggered by a statement. But see here:
>
>
>
> https://www.postgresql.org/docs/devel/event-trigger-database-login-example.html
>
>
>
I suggest adding a sentence and link from the main description [1] to that
page.

We already do that for the ones that have commands:
"For a complete list of commands supported by the event trigger mechanism,
see Section 38.2."

and login deserves something similar on that page.

David J.

[1] https://www.postgresql.org/docs/devel/event-trigger-definition.html

Re: POC, WIP: OR-clause support for indexes

2024-06-27 Thread Alena Rybakina

On 26.06.2024 23:19, Robert Haas wrote:

I think maybe replying to multiple emails with a single email is
something you'd be better off doing less often.
Ok, I won't do this in the future. After thinkingit over,I
realizedthatit turnedout to be somekindof messinthe end.

On Tue, Jun 25, 2024 at 7:14 PM Alena Rybakina
wrote:

Sorry, you are right and I'll try to explain more precisely. The first approach is the
first part of the patch, where we made "Or" expressions into an SAOP at an
early stage of plan generation [0], the second one was the one proposed by A.Korotkov [1].

[0] isn't doing anything "at an early stage of plan generation". It's
changing something in *the parser*. The parser and planner are VERY
different stages of query parsing, and it's really important to keep
them separate mentally and in discussions.

Thanks for the detailed explanation, I'm always glad to learn new things
for myself)

To be honest, I had an intuitive feeling that the transformation was
called in the analyzer stage, but I wasn't sure about it, so I tried to
summarize it.

As for the fact that in general all this can be divided into two large
stages, parsing and planner is a little new to me. I have reread the
documentation [0] andI foundinformationaboutitthere.

Beforethat, Iwas guidedbyinformationfromthe
CarnegieMellonUniversitylecture andthe BruceMamjian report[1],whichwas
wrongonmypart.

By the way,it turnsout that the queryrewritingstagereferstoan
independentstage,whichis locatedbetweenthe
parserstageandtheplanner/optimizer. I found it from the documentation [2].

[0] https://www.postgresql.org/docs/current/planner-optimizer.html

[1] https://momjian.us/main/writings/pgsql/optimizer.pdf

[2] https://www.postgresql.org/docs/16/rule-system.html

We should not be changing
anything about the query in the parser, because that will, as
Alexander also pointed out, change what gets stored if the user does
something like CREATE VIEW whatever AS SELECT ...; and we should, for
the most part, be storing the query as the user entered it, not some
transformed version of it. Further, at the parser stage, we do not
know the cost of anything, so we can only transform things when the
transformed version is always - or practically always - going to be
cheaper than the untransformed version.

Thank you, now it has become clear to me why it is so important to leave
the transformation at the planner stage.

On 24.06.2024 18:28, Robert Haas wrote:
Andrei mentioned the problem, which might be caused by the transformation on
the later stage of optimization is updating references to expressions in
RestrictInfo [3] lists, because they can be used in different parts during the
formation of the query plan. As the practice of Self-join removal [4] has
shown, this can be expensive, but feasible. By applying the transformation at
the analysis stage [0], because no links were created, so we did not encounter
such problems, so this approach was more suitable than the others.

The link you provided for [3] doesn't show me exactly what code you're
talking about, but I can see why mutating a RestrictInfo after
creating it could be problematic. However, I'm not proposing that, and
I don't think it's a good idea. Instead of mutating an existing data
structure after it's been created, we want to get each data structure
correct at the moment that it is created. What that means is that at
each stage of processing, whenever we create a new in-memory data
structure, we have an opportunity to make changes along the way.

For instance, let's say we have a RestrictInfo and we are creating a
Path, perhaps via create_index_path(). One argument to that function
is a list of indexclauses. The indexclauses are derived from the
RestrictInfo list associated with the RelOptInfo. We take some subset
of those quals that are deemed to be indexable and we reorder them and
maybe change a few things and we build this new list of indexclauses
that is then passed to create_index_path(). The RelOptInfo's list of
RestrictInfos is not changed -- only the new list of clauses derived
from it is being built up here, without any mutation of the original
structure.

This is the kind of thing that this patch can and probably should do.
Join removal is quite awkward, as you rightly point out, because we
end up modifying existing data structures after they've been created,
and that requires us to run around and fix up a bunch of stuff, and
that can have bugs. Whenever possible, we don't want to do it that
way. Instead, we want to pick points in the processing when we're
anyway constructing some new structure and use that as an opportunity
to do transformations when building the new structure that incorporate
optimizations that make sense.

Thanks for the idea! I hadn't thought in this direction before, but it
really might just work and solve all our original problems.
By the way, I saw that the optimizer is smart enough to eliminate
duplicates. Below I ha

Re: Is missing LOGIN Event on Trigger Firing Matrix ?

2024-06-27 Thread Andrew Dunstan



On 2024-06-27 Th 2:40 PM, Marcos Pegoraro wrote:
create event trigger ... on login is now available but it is not shown 
on DOCs or is it in another page ?


https://www.postgresql.org/docs/devel/event-trigger-matrix.html
doesn't have it, should be there ?



It's not triggered by a statement. But see here:


https://www.postgresql.org/docs/devel/event-trigger-database-login-example.html


cheers


andrew


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

Re: Apparent bug in WAL summarizer process (hung state)

2024-06-27 Thread Israel Barth Rubio

> Yeah, this is a bug. It seems that the WAL summarizer process, when
> restarted, wants to restart from wherever it was previously
> summarizing WAL, which is correct if that WAL is still around, but if
> summarize_wal has been turned off in the meanwhile, it might not be
> correct. Here's a patch to fix that.

Thanks for checking this!

> Thanks. New version attached.

And besides that, thanks for the patch, of course!

I compiled Postgres locally with your patch. I attempted to break it several
times, both manually and through a shell script.

No success on that -- which in this case is actually success :)
The WAL summarizer seems able to always resume from a valid point,
so `pg_basebackup` isn't failing anymore.

Is missing LOGIN Event on Trigger Firing Matrix ?

2024-06-27 Thread Marcos Pegoraro

create event trigger ... on login is now available but it is not shown on
DOCs or is it in another page ?

https://www.postgresql.org/docs/devel/event-trigger-matrix.html
doesn't have it, should be there ?

regards
Marcos

Re: Visibility bug with prepared transaction with subtransactions on standby

2024-06-27 Thread Heikki Linnakangas


On 20/06/2024 17:10, Heikki Linnakangas wrote:

On 20/06/2024 16:41, Heikki Linnakangas wrote:

Attached is a patch to fix this, with a test case.


The patch did not compile, thanks to a last-minute change in a field
name. Here's a fixed version.


All I heard is crickets, so committed and backported to all supported 
versions.


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

Re: Custom type's modifiers

Marthin Laubscher  writes:
> I confess to some uncertainty whether the PostgreSQL specific x::y notation 
> and the standards based CAST(x AS y) would both be addressed by creating a 
> cast.

Those notations are precisely equivalent, modulo concerns about
operator precedence (ie, if x isn't a single token you might have
to parenthesize x to get (x)::y to mean the same as CAST(x AS y)).

regards, tom lane

Re: Custom type's modifiers

2024-06-27 Thread Marthin Laubscher

On 2024/06/27, 18:13, "David G. Johnston"  
wrote:
> A cast between two types is going to accept a concrete instance of the input 
> type, in memory, as its argument and then produces a concrete instance of the 
> output type, in memory, as output.  If the input data is serialized the 
> constructor for the input type will handle deserialization.

I confess to some uncertainty whether the PostgreSQL specific x::y notation and 
the standards based CAST(x AS y) would both be addressed by creating a cast. 
What you’re saying means both forms engage the same code and defining a cast 
would cover the :: syntax as well. Thanks for that.

If my understanding is accurate, it means that even when both values are of 
MyType the CAST function would still be invoked so the type logic can determine 
how to handle (or reject) the cast. Cast would (obviously) require the target 
type modifiers as well, and the good news is that it’s already there as the 
second parameter of the function. So that’s the other function that receives 
the type modifier that I was missing. It’s starting to make plenty sense. 

To summarise:
- The type modifiers, encoded by the TYPMOD_IN function are passed directly as 
parameters to:- 
  -  the type's input function (parameter 3), and
  - any "with function" cast where the target type has type modifiers.
- Regardless of the syntax used to invoke the cast, the same function will be 
called.
- Depending on what casts are defined, converting from the external string 
format to a value of MyType will be handled either by the input function or a 
cast function. By default (without any casts) only values recognised by input 
can be converted to MyType values.

 -- Thanks for your time – Marthin Laubscher

Re: JIT causes core dump during error recovery

Ranier Vilela  writes:
> Em qui., 27 de jun. de 2024 às 13:18, Tom Lane  escreveu:
>> In any case, I found that adding some copying logic to CopyErrorData()
>> is enough to solve this problem, since the SPI infrastructure applies
>> that before executing xact cleanup.

> In this case, I think that these fields, in struct definition struct
> ErrorData (src/include/utils/elog.h)
> should be changed too?
> from const char * to char*

No, that would imply casting away const in errstart() etc.  We're
still mostly expecting those things to be pointers to constant
strings.

I'm about half tempted to file this as an LLVM bug.  When it inlines
a function, it should still reference the same string constants that
the original code did, otherwise it's failing to be a transparent
conversion.  But they'll probably cite some standards-ese that claims
this is undefined behavior:

const char * foo(void) { return "foo"; }

void bar(void) { Assert( foo() == foo() ); }

on which I call BS, but it's probably in there somewhere.

regards, tom lane

Re: Backporting BackgroundPsql

2024-06-27 Thread Heikki Linnakangas


On 26/06/2024 14:54, Robert Haas wrote:

On Wed, Jun 26, 2024 at 3:34 AM Heikki Linnakangas  wrote:

I haven't looked closely at the new PgFFI stuff but +1 on that in
general, and it makes sense to backport that once it lands on master. In
the meanwhile, I think we should backport BackgroundPsql as it is, to
make it possible to backport tests using it right now, even if it is
short-lived.


+1. The fact that PgFFI may be coming isn't a reason to not back-patch
this. The risk of back-patching testing infrastructure is also very
low as compared with code; in fact, there's a lot of risk from NOT
back-patching popular testing infrastructure.


Ok, I pushed commits to backport BackgroundPsql down to v12. I used 
"option 2", i.e. I changed background_psql() to return the new 
BackgroundPsql object.


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

Re: JIT causes core dump during error recovery

2024-06-27 Thread Ranier Vilela

Em qui., 27 de jun. de 2024 às 13:18, Tom Lane  escreveu:

> I wrote:
> > So delaying removal of the jit-created code segment until transaction
> > cleanup wouldn't be enough to prevent this crash, if I'm reading
> > things right.  The extra-pstrdup solution may be the only viable one.
>
> > I could use confirmation from someone who knows the JIT code about
> > when jit-created code is unloaded.  It also remains very unclear
> > why there is no crash if we don't force both jit_optimize_above_cost
> > and jit_inline_above_cost to small values.
>
> I found where the unload happens: ResOwnerReleaseJitContext, which
> is executed during the resource owner BEFORE_LOCKS phase.  (Which
> seems like a pretty dubious choice from here; why can't we leave
> it till the less time-critical phase after we've let go of locks?)
> But anyway, we definitely do drop this stuff during xact cleanup.
>
> Also, it seems that the reason that both jit_optimize_above_cost
> and jit_inline_above_cost must be small is that otherwise int4div
> is simply called from the JIT-generated code, not inlined into it.
> This gives me very considerable fear about how well that behavior
> has been tested: if throwing an error from inlined code doesn't
> work, and we hadn't noticed that, how much can it really have been
> exercised?  I have also got an itchy feeling that we have code that
> will be broken by this behavior of "if it happens to get inlined
> then string constants aren't so constant".
>
> In any case, I found that adding some copying logic to CopyErrorData()
> is enough to solve this problem, since the SPI infrastructure applies
> that before executing xact cleanup.

In this case, I think that these fields, in struct definition struct
ErrorData (src/include/utils/elog.h)
should be changed too?
from const char * to char*

best regards,
Ranier Vilela

Re: Custom type's modifiers

Marthin Laubscher  writes:
> I don't see another function getting passed the value so I'd assume that 
> (unless I return a MyType value from one of my own functions which would 
> follow its internal logic to determine which type modifiers to use) the only 
> way a MyType can get an initial value is via the input function. If the type 
> is in a table column the input function would be called with the default 
> value specified in external format if a value isn't specified during insert, 
> but either way it would always originate from the eternal format. I suppose 
> when a cast is involved it goes via the external format as well, right?

The only code anywhere in the system that can produce a MyType value
is code you've written.  So that has to come originally from your
input function, or cast functions or constructor functions you write.

You'd be well advised to read the CREATE CAST doco about how to
support notations like 'something'::MyType(x,y,z).  Although the
input function is expected to be able to apply a typemod if it's
passed one, in most situations coercion to a specific typemod is
handled by invoking a multi-parameter cast function.

regards, tom lane

Re: JIT causes core dump during error recovery

I wrote:
> So delaying removal of the jit-created code segment until transaction
> cleanup wouldn't be enough to prevent this crash, if I'm reading
> things right.  The extra-pstrdup solution may be the only viable one.

> I could use confirmation from someone who knows the JIT code about
> when jit-created code is unloaded.  It also remains very unclear
> why there is no crash if we don't force both jit_optimize_above_cost
> and jit_inline_above_cost to small values.

I found where the unload happens: ResOwnerReleaseJitContext, which
is executed during the resource owner BEFORE_LOCKS phase.  (Which
seems like a pretty dubious choice from here; why can't we leave
it till the less time-critical phase after we've let go of locks?)
But anyway, we definitely do drop this stuff during xact cleanup.

Also, it seems that the reason that both jit_optimize_above_cost
and jit_inline_above_cost must be small is that otherwise int4div
is simply called from the JIT-generated code, not inlined into it.
This gives me very considerable fear about how well that behavior
has been tested: if throwing an error from inlined code doesn't
work, and we hadn't noticed that, how much can it really have been
exercised?  I have also got an itchy feeling that we have code that
will be broken by this behavior of "if it happens to get inlined
then string constants aren't so constant".

In any case, I found that adding some copying logic to CopyErrorData()
is enough to solve this problem, since the SPI infrastructure applies
that before executing xact cleanup.  I had feared that we'd have to
add copying to every single elog/ereport sequence, which would have
been an annoying amount of overhead; but the attached seems
acceptable.  We do get through check-world with this patch and the
JIT parameters all set to small values.

regards, tom lane

diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index d91a85cb2d..d1d1632bdd 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -1743,7 +1743,21 @@ CopyErrorData(void)
 	newedata = (ErrorData *) palloc(sizeof(ErrorData));
 	memcpy(newedata, edata, sizeof(ErrorData));

-	/* Make copies of separately-allocated fields */
+	/*
+	 * Make copies of separately-allocated strings.  Note that we copy even
+	 * theoretically-constant strings such as filename.  This is because those
+	 * could point into JIT-created code segments that might get unloaded at
+	 * transaction cleanup.  In some cases we need the copied ErrorData to
+	 * survive transaction boundaries, so we'd better copy those strings too.
+	 */
+	if (newedata->filename)
+		newedata->filename = pstrdup(newedata->filename);
+	if (newedata->funcname)
+		newedata->funcname = pstrdup(newedata->funcname);
+	if (newedata->domain)
+		newedata->domain = pstrdup(newedata->domain);
+	if (newedata->context_domain)
+		newedata->context_domain = pstrdup(newedata->context_domain);
 	if (newedata->message)
 		newedata->message = pstrdup(newedata->message);
 	if (newedata->detail)
@@ -1756,6 +1770,8 @@ CopyErrorData(void)
 		newedata->context = pstrdup(newedata->context);
 	if (newedata->backtrace)
 		newedata->backtrace = pstrdup(newedata->backtrace);
+	if (newedata->message_id)
+		newedata->message_id = pstrdup(newedata->message_id);
 	if (newedata->schema_name)
 		newedata->schema_name = pstrdup(newedata->schema_name);
 	if (newedata->table_name)

Re: Custom type's modifiers

2024-06-27 Thread David G. Johnston

On Thu, Jun 27, 2024 at 8:49 AM Marthin Laubscher 
wrote:

>
> I suppose when a cast is involved it goes via the external format as well,
> right?
>

A cast between two types is going to accept a concrete instance of the
input type, in memory, as its argument and then produces a concrete
instance of the output type, in memory, as output.  If the input data is
serialized the constructor for the input type will handle deserialization.

See: create cast

https://www.postgresql.org/docs/current/sql-createcast.html

In particular the phrasing: identical to or binary-coercible to

David J.

Re: Custom type's modifiers

2024-06-27 Thread Marthin Laubscher

On 2024/06/27, 17:06, "Tom Lane" mailto:t...@sss.pgh.pa.us>> wrote:
> You can't. Whatever info is needed by operations on the type had better be 
> embedded in the value.

OK, thanks, that's clear and easy enough. I'll ensure the the third parameter 
to the input function is embedded in my opaque value. 

I don't see another function getting passed the value so I'd assume that 
(unless I return a MyType value from one of my own functions which would follow 
its internal logic to determine which type modifiers to use) the only way a 
MyType can get an initial value is via the input function. If the type is in a 
table column the input function would be called with the default value 
specified in external format if a value isn't specified during insert, but 
either way it would always originate from the eternal format. I suppose when a 
cast is involved it goes via the external format as well, right?

Are those sound assumptions to make or am I still way off base here?

  --- Thanks for your time - Marthin Laubscher

Re: ClientRead on ROLLABACK

2024-06-27 Thread Simone G.

Oh, I see. So the ROLLBACK command was executed! So I suppose the client was 
waiting just for the ACK and the connection has been left open.

> I think this is a super common confusion among users. Maybe we should
> consider making it clearer that no query is currently being executed.
> Something like
> 
> IDLE: last query: SELECT * FROM mytable;

I think the clearest option would be leave the query column empty and add a new 
column last_query. But this suggestion may still do its job in clarifying that 
the query is not running.

Re: ClientRead on ROLLABACK

On Thu, 27 Jun 2024 at 17:21, Tom Lane  wrote:
> (We used to show "" in the query column in this state, but that
> was deemed less helpful than the current behavior.)

I think this is a super common confusion among users. Maybe we should
consider making it clearer that no query is currently being executed.
Something like

IDLE: last query: SELECT * FROM mytable;

Re: ClientRead on ROLLABACK

Simone Giusso  writes:
> I have a question regarding postgresql waiting for the client. I queried
> the pg_stat_activity because I noticed a connection that had not been
> released for days!!! I saw that the wait_event was ClientRead and the query
> was ROLLBACK. What the server is waiting for from the client?

You are misunderstanding that display.  If the wait state is ClientRead
then the server has nothing to do and is awaiting a fresh SQL command
from the client.  The query that's shown is the last-executed query.
(We used to show "" in the query column in this state, but that
was deemed less helpful than the current behavior.)

> So I ended up in a situation where both client and server were reading from
> the socket :(. I'm not sure why. Something went wrong between client and
> server, network problems?

Yeah, a dropped packet could explain this perhaps.

regards, tom lane

Re: Custom type's modifiers

Marthin Laubscher  writes:
> But now I need to (re)define MyType to support type modifiers (e.g. 
> MyType(1,14,18)) and I got that done using CREATE TYPE’s TYPMOD_IN and 
> TYPMOD_OUT parameters resulting in the correct packed value getting stored in 
> pg_attribute when I define a column of that type. 

OK ...

> But when I pass a MyType value to a function defined in my C extension how 
> would I access the type modifier value for the argument which could have been 
> drawn from the catalog or the result of a cast. 

You can't.  Whatever info is needed by operations on the type had
better be embedded in the value.

regards, tom lane

Re: Patch bug: Fix jsonpath .* on Arrays

2024-06-27 Thread David E. Wheeler

On Jun 27, 2024, at 04:17, Michael Paquier  wrote:

> The tests of jsonb_jsonpath.sql include a lot of patterns for @? and
> jsonb_path_query with the lax and strict modes, so shouldn't these
> additions be grouped closer to the existing tests rather than added at 
> the end of the file?

I think you could argue that they should go with other tests for array 
unwrapping, though it’s kind of mixed throughout. But that’s more the bit I was 
testing; almost all the tests are lax, and it’s less the strict behavior to 
test here than the explicit behavior of unwrapping in lax mode.

But ultimately I don’t care where they go, just that we have them.

D

Custom type's modifiers

2024-06-27 Thread Marthin Laubscher

Hi,

I’m defining a custom type “MyType” with additional functions and an custom 
aggregate in a C-coded extension. From a PostgreSQL perspective it is a base 
type that piggybacks on the bytea type, i.e. LIKE = BYTEA.
 
But now I need to (re)define MyType to support type modifiers (e.g. 
MyType(1,14,18)) and I got that done using CREATE TYPE’s TYPMOD_IN and 
TYPMOD_OUT parameters resulting in the correct packed value getting stored in 
pg_attribute when I define a column of that type. 

But when I pass a MyType value to a function defined in my C extension how 
would I access the type modifier value for the argument which could have been 
drawn from the catalog or the result of a cast. 

E.g. if I: 
SELECT MyFunc(‘xDEADBEEF’::MyType(1,14,18));
In the C function MyFunc calls I get a pointer to the data using 
PG_GETARG_BYTEA_P(0) macro and its length using the VARSIZE macro but I also 
need the given type modifiers (1, 14 and 18 in the example) before I can 
process the data correctly. Clearly I'd have to unpack the component values 
myself from the 16-bit atttypemod value into which the TYPMOD_OUT function has 
packed it, but where would I get access to that value?  My type is written in C 
to be as fast as possible having to go do some SPI-level lookup or involved 
logic would slow it right down again.

My searches to date only yielded results referring to the value stored for a 
table in pg_attribute with the possibility of there being a value in 
HeapTupleHeader obtained by using the PG_GETARG_HEAPTUPLEHEADER(0) macro but 
that assumes the parameter is a tuple, not the individual value it actually is. 
I struggle to imagine that the type modifier value isn't being maintained by 
whatever casts are applied and not getting passed through to the extension 
already, but where to find it? 

Can someone please point me in the right direction here, the name of the 
structure containing the raw type modifier value, the name of the value in that 
structure, the name of a macro that accesses it, even if it’s just what 
keywords to search for in the documentation and/or archives? Even if it’s just 
a pointer to the code where e.g. the numeric type (which has type modifiers) is 
implemented so I can see how that code does it. Anything, I’m getting 
desperate. Perhaps not many before me needed to do this so it's not often 
mentioned, but sure it is in there somewhere, how else would type like numeric 
and even varchar actually work (given that the VARSIZE of a varlena gives its 
actual size, not the maximum as given when the column or value was created)?

Thank you in advance,

Marthin Laubscher

Re: SQL/JSON query functions context_item doc entry and type requirement

Hi,

On Sat, Jun 22, 2024 at 6:39 PM jian he  wrote:
> On Fri, Jun 21, 2024 at 8:18 PM Amit Langote  wrote:
> >
> > >> JSON_VALUE on error, on empty semantics should be the same as json_query.
> > >> like:
> > >> [ { ERROR | NULL | EMPTY { [ ARRAY ] | OBJECT } | DEFAULT expression }
> > >> ON EMPTY ]
> > >> [ { ERROR | NULL | EMPTY { [ ARRAY ] | OBJECT } | DEFAULT expression }
> > >> ON ERROR ])
> > >>
> > >> examples:
> > >> select JSON_value(jsonb '[]' , '$'  empty array on error);
> > >> select JSON_value(jsonb '[]' , '$'  empty object on error);
> > >
> > > Again the documented behavior seems to make sense though and the ability 
> > > to specify empty in the value function seems like a bug.  If you really 
> > > want an empty array or object you do have access to default.  The reason 
> > > json_query provides for an empty array/object is that it is already 
> > > expecting to produce an array (object seems a little odd).
> > >
> > > I agree our docs and code do not match which needs to be fixed, ideally 
> > > in the direction of the standard which I'm guessing our documentation is 
> > > based off of.  But let's not go off of my guess.
> >
> > Oops, that is indeed not great and, yes, the problem is code not
> > matching the documentation, the latter of which is "correct".
> >
> > Basically, the grammar allows specifying any of the all possible ON
> > ERROR/EMPTY behavior values irrespective of the function, so parse
> > analysis should be catching and flagging an attempt to specify
> > incompatible value for a given function, which it does not.
> >
> > I've attached a patch to fix that, which I'd like to push before
> > anything else we've been discussing.
> >
>
> + errcode(ERRCODE_SYNTAX_ERROR),
> + errmsg("invalid ON ERROR behavior"),
> + errdetail("Only ERROR, NULL, EMPTY [ ARRAY | OBJECT }, or DEFAULT
>  is allowed in ON ERROR for JSON_QUERY()."),
> + parser_errposition(pstate, func->on_error->location));
>
> `EMPTY [ ARRAY | OBJECT }` seems not correct,
> maybe just EMPTY, EMPTY ARRAY, EMPTY OBJECT.
> (apply to other places)

Or EMPTY [ ARRAY ], EMPTY OBJECT

> `DEFAULT `
> `DEFAULT ` or just `DEFAULT expression` would be more correct?
> (apply to other places)

"DEFAULT expression" sounds good.

> I think we should make json_query, json_value on empty, on error
> behave the same way.
> otherwise, it will have consistency issues for scalar jsonb.
> for example, we should expect the following two queries to  return the
> same result?
> SELECT * FROM JSON_query(jsonb '1', '$.a' returning jsonb empty on empty);
> SELECT * FROM JSON_value(jsonb '1', '$.a' returning jsonb empty on empty);
>
> Also the json_table function will call json_value or json_query,
> make these two functions on error, on empty behavior the same can
> reduce unintended complexity.
>
> So based on your
> patch(v1-0001-SQL-JSON-Disallow-incompatible-values-in-ON-ERROR.patch)
> and the above points, I have made some changes, attached.
> it will make json_value, json_query not allow {true | false | unknown
> } on error, {true | false | unknown } on empty.
> json_table error message deal the same way as
> b4fad46b6bc8a9bf46ff689bcb1bd4edf8f267af

Here is an updated patch that I think takes care of these points.

> BTW,
> i found one JSON_TABLE document deficiency
> [ { ERROR | NULL | EMPTY { ARRAY | OBJECT } | DEFAULT
> expression } ON EMPTY ]
> [ { ERROR | NULL | EMPTY { ARRAY | OBJECT } | DEFAULT
> expression } ON ERROR ]
>
> it should be
>
> [ { ERROR | NULL | EMPTY { [ARRAY] | OBJECT } | DEFAULT
> expression } ON EMPTY ]
> [ { ERROR | NULL | EMPTY { [ARRAY] | OBJECT } | DEFAULT
> expression } ON ERROR ]

You're right.  Fixed.

Also, I noticed that the grammar allows ON EMPTY in JSON_TABLE EXISTS
columns which is meaningless because JSON_EXISTS() doesn't have a
corresponding ON EMPTY clause.  Fixed grammar to prevent that in the
attached 0002.

--
Thanks, Amit Langote


v2-0002-SQL-JSON-Prevent-ON-EMPTY-for-EXISTS-columns-in-J.patch
Description: Binary data


v2-0001-SQL-JSON-Disallow-incompatible-values-in-ON-ERROR.patch
Description: Binary data

Re: Doc: fix track_io_timing description to mention pg_stat_io

2024-06-27 Thread Nazir Bilal Yavuz

Hi,

On Thu, 27 Jun 2024 at 14:30, Melanie Plageman
 wrote:
>
> On Thu, Jun 27, 2024 at 5:06 AM  wrote:
> >
> > Hi,
> >
> > pg_stat_io has I/O statistics that are collected when track_io_timing is
> > enabled, but it is not mentioned in the description of track_io_timing.
> > I think it's better to add a description of pg_stat_io for easy reference.
> > What do you think?
>
> Seems quite reasonable to me given that track_wal_io_timing mentions
> pg_stat_wal. I noticed that the sentence about track_io_timing in the
> statistics collection configuration section [1] only mentions reads
> and writes -- perhaps it should also mention extends and fsyncs?

Both suggestions look good to me. If what you said will be
implemented, maybe track_wal_io_timing too should mention fsyncs?

-- 
Regards,
Nazir Bilal Yavuz
Microsoft

Re: Should we document how column DEFAULT expressions work?

2024-06-27 Thread Peter Eisentraut


On 27.06.24 02:34, David Rowley wrote:

For the special timestamp stuff, that place is probably the special
timestamp table in [1].  It looks like the large caution you added in
540849814 might not be enough or perhaps wasn't done soon enough to
catch the people who read that part of the manual before the caution
was added. Hard to fix if it's the latter without a time machine. :-(


Maybe we should really be thinking about deprecating these special 
values and steering users more urgently toward more robust alternatives.


Imagine if 'random' were a valid input value for numeric types.

Re: Avoid incomplete copy string (src/backend/access/transam/xlog.c)

2024-06-27 Thread Ranier Vilela

Em qui., 27 de jun. de 2024 às 08:48, Ranier Vilela 
escreveu:

> Em qui., 27 de jun. de 2024 às 01:01, Yugo NAGATA 
> escreveu:
>
>> On Mon, 24 Jun 2024 08:25:36 -0300
>> Ranier Vilela  wrote:
>>
>> > Em dom., 23 de jun. de 2024 às 23:56, Richard Guo <
>> guofengli...@gmail.com>
>> > escreveu:
>> >
>> > > On Mon, Jun 24, 2024 at 7:51 AM Ranier Vilela 
>> wrote:
>> > > > In src/include/access/xlogbackup.h, the field *name*
>> > > > has one byte extra to store null-termination.
>> > > >
>> > > > But, in the function *do_pg_backup_start*,
>> > > > I think that is a mistake in the line (8736):
>> > > >
>> > > > memcpy(state->name, backupidstr, strlen(backupidstr));
>> > > >
>> > > > memcpy with strlen does not copy the whole string.
>> > > > strlen returns the exact length of the string, without
>> > > > the null-termination.
>> > >
>> > > I noticed that the two callers of do_pg_backup_start both allocate
>> > > BackupState with palloc0.  Can we rely on this to ensure that the
>> > > BackupState.name is initialized with null-termination?
>> > >
>> > I do not think so.
>> > It seems to me the best solution is to use Michael's suggestion,
>> strlcpy +
>> > sizeof.
>> >
>> > Currently we have this:
>> > memcpy(state->name, "longlongpathexample1",
>> > strlen("longlongpathexample1"));
>> > printf("%s\n", state->name);
>> > longlongpathexample1
>> >
>> > Next random call:
>> > memcpy(state->name, "longpathexample2", strlen("longpathexample2"));
>> > printf("%s\n", state->name);
>> > longpathexample2ple1
>>
>> In the current uses, BackupState is freed (by pfree or
>> MemoryContextDelete)
>> after each use of BackupState, so the memory space is not reused as your
>> scenario above, and there would not harms even if the null-termination is
>> omitted.
>>
>> However, I wonder it is better to use strlcpy without assuming such the
>> good
>> manner of callers.
>>
> Thanks for your inputs.
>
> strlcpy is used across all the sources, so this style is better and safe.
>
> Here v4, attached, with MAXPGPATH -1, according to your suggestion.
>
Now with file patch really attached.

best regards,
Ranier Vilela


v4-avoid-incomplete-copy-string-do_pg_backup_start.patch
Description: Binary data

Re: pgsql: Add more SQL/JSON constructor functions

On Thu, Jun 27, 2024 at 6:57 PM Amit Langote  wrote:
> On Wed, Jun 26, 2024 at 11:46 PM jian he  wrote:
> > On Wed, Jun 26, 2024 at 8:39 PM Amit Langote  
> > wrote:
> > >
> > > >
> > > > The RETURNING variant giving an error is what the standard asks us to
> > > > do apparently.  I read Tom's last message on this thread as agreeing
> > > > to that, even though hesitantly.  He can correct me if I got that
> > > > wrong.
> > > >
> > > > > your patch will make domain and char(n) behavior inconsistent.
> > > > > create domain char2 as char(2);
> > > > > SELECT JSON_VALUE(jsonb '"aaa"', '$' RETURNING char2 ERROR ON ERROR);
> > > > > SELECT JSON_VALUE(jsonb '"aaa"', '$' RETURNING char(2) ERROR ON 
> > > > > ERROR);
> > > > >
> > > > >
> > > > > another example:
> > > > > SELECT JSON_query(jsonb '"aaa"', '$' RETURNING char(2) keep quotes
> > > > > default '"aaa"'::jsonb ON ERROR);
> > > > > same value (jsonb "aaa") error on error will yield error,
> > > > > but `default expression on error` can coerce the value to char(2),
> > > > > which looks a little bit inconsistent, I think.
> > > >
> > > > Interesting examples, thanks for sharing.
> > > >
> > > > Attached updated version should take into account that typmod may be
> > > > hiding under domains.  Please test.
> > >
> >
> > SELECT JSON_VALUE(jsonb '111', '$' RETURNING queryfuncs_char2 default
> > '13' on error);
> > return
> > ERROR:  value too long for type character(2)
> > should return 13
> >
> > I found out the source of the problem is in coerceJsonExprOutput
> > /*
> > * Use cast expression for domain types; we need CoerceToDomain here.
> > */
> > if (get_typtype(returning->typid) != TYPTYPE_DOMAIN)
> > {
> > jsexpr->use_io_coercion = true;
> > return;
> > }
>
> Thanks for this test case and the analysis.  Yes, using a cast
> expression for coercion to the RETURNING type generally seems to be a
> source of many problems that could've been solved by fixing things so
> that only use_io_coercion and use_json_coercion are enough to handle
> all the cases.
>
> I've attempted that in the attached 0001, which removes
> JsonExpr.coercion_expr and a bunch of code around it.
>
> 0002 is now the original patch minus the changes to make
> JSON_EXISTS(), JSON_QUERY(), and JSON_VALUE() behave as we would like,
> because the changes in 0001 covers them. The changes for JsonBehavior
> expression coercion as they were in the last version of the patch are
> still needed, but I decided to move those into 0001 so that the
> changes for query functions are all in 0001 and those for constructors
> in 0002.  It would be nice to get rid of that coerce_to_target_type()
> call to coerce the "behavior expression" to RETURNING type, but I'm
> leaving that as a task for another day.

Updated 0001 to remove outdated references, remove some more unnecessary code.

-- 
Thanks, Amit Langote


v4-0002-SQL-JSON-Use-implicit-casts-for-RETURNING-type-wi.patch
Description: Binary data


v4-0001-SQL-JSON-Always-coerce-JsonExpr-result-at-runtime.patch
Description: Binary data

Re: Avoid incomplete copy string (src/backend/access/transam/xlog.c)

2024-06-27 Thread Ranier Vilela

Em qui., 27 de jun. de 2024 às 01:01, Yugo NAGATA 
escreveu:

> On Mon, 24 Jun 2024 08:25:36 -0300
> Ranier Vilela  wrote:
>
> > Em dom., 23 de jun. de 2024 às 23:56, Richard Guo <
> guofengli...@gmail.com>
> > escreveu:
> >
> > > On Mon, Jun 24, 2024 at 7:51 AM Ranier Vilela 
> wrote:
> > > > In src/include/access/xlogbackup.h, the field *name*
> > > > has one byte extra to store null-termination.
> > > >
> > > > But, in the function *do_pg_backup_start*,
> > > > I think that is a mistake in the line (8736):
> > > >
> > > > memcpy(state->name, backupidstr, strlen(backupidstr));
> > > >
> > > > memcpy with strlen does not copy the whole string.
> > > > strlen returns the exact length of the string, without
> > > > the null-termination.
> > >
> > > I noticed that the two callers of do_pg_backup_start both allocate
> > > BackupState with palloc0.  Can we rely on this to ensure that the
> > > BackupState.name is initialized with null-termination?
> > >
> > I do not think so.
> > It seems to me the best solution is to use Michael's suggestion, strlcpy
> +
> > sizeof.
> >
> > Currently we have this:
> > memcpy(state->name, "longlongpathexample1",
> > strlen("longlongpathexample1"));
> > printf("%s\n", state->name);
> > longlongpathexample1
> >
> > Next random call:
> > memcpy(state->name, "longpathexample2", strlen("longpathexample2"));
> > printf("%s\n", state->name);
> > longpathexample2ple1
>
> In the current uses, BackupState is freed (by pfree or MemoryContextDelete)
> after each use of BackupState, so the memory space is not reused as your
> scenario above, and there would not harms even if the null-termination is
> omitted.
>
> However, I wonder it is better to use strlcpy without assuming such the
> good
> manner of callers.
>
Thanks for your inputs.

strlcpy is used across all the sources, so this style is better and safe.

Here v4, attached, with MAXPGPATH -1, according to your suggestion.

>From the linux man page:
https://linux.die.net/man/3/strlcpy

" The *strlcpy*() function copies up to *size* - 1 characters from the
NUL-terminated string *src* to *dst*, NUL-terminating the result. "

best regards,
Ranier Vilela

Re: Doc: fix track_io_timing description to mention pg_stat_io

2024-06-27 Thread Melanie Plageman

On Thu, Jun 27, 2024 at 5:06 AM  wrote:
>
> Hi,
>
> pg_stat_io has I/O statistics that are collected when track_io_timing is
> enabled, but it is not mentioned in the description of track_io_timing.
> I think it's better to add a description of pg_stat_io for easy reference.
> What do you think?

Seems quite reasonable to me given that track_wal_io_timing mentions
pg_stat_wal. I noticed that the sentence about track_io_timing in the
statistics collection configuration section [1] only mentions reads
and writes -- perhaps it should also mention extends and fsyncs?

- Melanie

[1] 
https://www.postgresql.org/docs/devel/monitoring-stats.html#MONITORING-STATS-SETUP

Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into PostgreSQL 15)

2024-06-27 Thread Aleksander Alekseev

Hi,

> > I prepared the patch for clog.c. The rest of the warnings don't strike
> > me as something we should immediately act on unless we have a bug
> > report. Or perhaps there is a particular warning that worries you?
>
> Is "int" acceptable or unacceptable in the following grep match?
>
> src/backend/commands/async.c:1274:  int headPage = 
> QUEUE_POS_PAGE(QUEUE_HEAD);

Good catch. We better use int64s here.

Here is the corrected patchset.

-- 
Best regards,
Aleksander Alekseev


v2-0001-Fix-the-comment-for-SlruCtlData.long_segment_name.patch
Description: Binary data


v2-0002-Use-int64-for-page-numbers-in-clog.c-async.c.patch
Description: Binary data

Re: Support a wildcard in backtrace_functions

On Wed, 15 May 2024 at 20:31, Robert Haas  wrote:
> That's fine, except that I think that what the previous discussion
> revealed is that we don't have consensus on how backtrace behavior
> ought to be controlled: backtrace_on_internal_error was one proposal,
> and this was a competing proposal, and neither one of them seemed to
> be completely satisfactory.

Attached is a rebased patchset of my previous proposal, including a
few changes that Michael preferred:
1. Renames log_backtrace to log_backtrace_mode
2. Rename internal to internal_error

I reread the thread since I previously posted the patch and apart from
Michaels feedback I don't think there was any more feedback on the
current proposal.

Rethinking about it myself though, I think the main downside of this
proposal is that if you want the previous behaviour of
backtrace_functions (add backtraces to all elog/ereports in the given
functions) you now need to set three GUCs:
log_backtrace_mode='all'
backtrace_functions='some_func'
backtrace_min_level=DEBUG5

The third one is not needed in the common case where someone only
cares about errors, but still needing to set log_backtrace_mode='all'
might seem a bit annoying. One way around that would be to make
log_backtrace_mode and backtrace_functions be additive instead of
subtractive.

Personally I think the proposed subtractive nature would be exactly
what I want for backtraces I'm interested in. Because I would want to
use backtrace_functions in this way:

1. I see an error I want a backtrace of: et log_backtrace_mode='all'
and try to trigger again.
2. Oops, there's many backtraces now let's filter by function: set
backtrace_functions=some_func

So if it's additive, I'd have to also undo log_backtrace_mode='all'
again at step 2. So changing two GUCs instead of one to do what I
want.
From 85a894cc32381a4ff2a1c7aab31476b2bbf6bdf3 Mon Sep 17 00:00:00 2001
From: Jelte Fennema-Nio 
Date: Thu, 21 Mar 2024 13:05:35 +0100
Subject: [PATCH 1/2] Add PGErrorVerbosity to typedefs.list

This one was missing, resulting in some strange alignment.
---
 src/include/utils/elog.h | 2 +-
 src/tools/pgindent/typedefs.list | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h
index 054dd2bf62f..da1a7469fa5 100644
--- a/src/include/utils/elog.h
+++ b/src/include/utils/elog.h
@@ -493,7 +493,7 @@ typedef enum
 	PGERROR_TERSE,/* single-line error messages */
 	PGERROR_DEFAULT,			/* recommended style */
 	PGERROR_VERBOSE,			/* all the facts, ma'am */
-}			PGErrorVerbosity;
+} PGErrorVerbosity;

 extern PGDLLIMPORT int Log_error_verbosity;
 extern PGDLLIMPORT char *Log_line_prefix;
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 61ad417cde6..67ec5408a98 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -1778,6 +1778,7 @@ PGAsyncStatusType
 PGCALL2
 PGChecksummablePage
 PGContextVisibility
+PGErrorVerbosity
 PGEvent
 PGEventConnDestroy
 PGEventConnReset

base-commit: 3e53492aa7084bceaa92757c90e067d79768797e
-- 
2.34.1

From 9a1256aa3e4f585e3f588122662050f2302585dc Mon Sep 17 00:00:00 2001
From: Jelte Fennema-Nio 
Date: Thu, 27 Jun 2024 10:56:10 +0200
Subject: [PATCH 2/2] Allow logging backtraces in more cases

We previously only had the backtrace_functions GUC to control when
backtraces were logged. Based on mailinglist discussion there were two
common cases that people wanted backtraces that were not covered by this
GUC though:

1. Logging backtraces for all internal errors
2. Logging backtraces for all errors

To support those two usecases, as well as to allow users to continue to
log backtraces for specific warnings using `backtrace_functions`, this
modifies the GUCs in the following way:

1. Adds a `log_backtrace` GUC, which can be set to `none` (default),
   `internal_error` and `all` to log different types of backtraces.
2. Change `backtrace_functions` to behave as an additional filter on top
   of `log_backtrace`. The empty string (the default) is now interpreted
   as doing no filtering based on function name.
3. Add a `backtrace_min_level` GUC, which limits for which log entries
   backtraces are written, based on their log level. This defaults to
   ERROR.

This does mean that setting `backtrace_functions=some_func` alone is not
enough to get backtraces for some_func. You now need to combine that
with `log_backtrace_mode=all` and if you want to get backtraces for
non-errors (which you previously got), you also need to change
backtrace_min_level to whichever level you want backtraces for.
---
 doc/src/sgml/config.sgml  | 82 +--
 src/backend/utils/error/elog.c| 30 ++-
 src/backend/utils/misc/guc_tables.c   | 50 +++
 src/backend/utils/misc/postgresql.conf.sample |  1 +
 src/include/utils/elog.h  |  8 ++
 src/include/utils/guc.h   |  1 +
 src/tools/pgindent/typ

Re: doc: modify the comment in function libpqrcv_check_conninfo()

2024-06-27 Thread ikedarintarof

Thanks for your suggestion. I used ChatGPT to choose the wording, but 
it's still difficult for me.


The new patch includes your suggestion.

On 2024-06-27 17:18, Jelte Fennema-Nio wrote:

On Thu, 27 Jun 2024 at 09:09, ikedarintarof
 wrote:


Thank you for your comment!

I've added the must_use_password argument. A new patch is attached.


s/whther/whether

nit: "it will do nothing" sounds a bit strange to me (but I'm not
native english). Something like this reads more natural to me:

an error. If must_use_password is true, the function raises an error
if no password is provided in the connection string. In any other case
it successfully completes.
From c01cadcb955d79ff525992985bd59736c6731abe Mon Sep 17 00:00:00 2001
From: Rintaro Ikeda <51394766+rinik...@users.noreply.github.com>
Date: Thu, 27 Jun 2024 16:04:08 +0900
Subject: [PATCH] modify the commnet in libpqrcv_check_conninfo()

---
 src/backend/replication/libpqwalreceiver/libpqwalreceiver.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
index 02f12f2921..4f7b69a73d 100644
--- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
+++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
@@ -309,8 +309,9 @@ bad_connection:
  * local filesystem access to be attempted.
  *
  * If the connection string can't be parsed, this function will raise
- * an error and will not return. If it can, it will return true if this
- * connection string specifies a password and false otherwise.
+ * an error. If must_use_password is true, the function raises an error
+ * if no password is provided in the connection string. In any other case
+ * it successfully completes.
  */
 static void
 libpqrcv_check_conninfo(const char *conninfo, bool must_use_password)
-- 
2.39.3 (Apple Git-146)

Re: pgsql: Add more SQL/JSON constructor functions

Hi,

On Wed, Jun 26, 2024 at 11:46 PM jian he  wrote:
> On Wed, Jun 26, 2024 at 8:39 PM Amit Langote  wrote:
> >
> > >
> > > The RETURNING variant giving an error is what the standard asks us to
> > > do apparently.  I read Tom's last message on this thread as agreeing
> > > to that, even though hesitantly.  He can correct me if I got that
> > > wrong.
> > >
> > > > your patch will make domain and char(n) behavior inconsistent.
> > > > create domain char2 as char(2);
> > > > SELECT JSON_VALUE(jsonb '"aaa"', '$' RETURNING char2 ERROR ON ERROR);
> > > > SELECT JSON_VALUE(jsonb '"aaa"', '$' RETURNING char(2) ERROR ON ERROR);
> > > >
> > > >
> > > > another example:
> > > > SELECT JSON_query(jsonb '"aaa"', '$' RETURNING char(2) keep quotes
> > > > default '"aaa"'::jsonb ON ERROR);
> > > > same value (jsonb "aaa") error on error will yield error,
> > > > but `default expression on error` can coerce the value to char(2),
> > > > which looks a little bit inconsistent, I think.
> > >
> > > Interesting examples, thanks for sharing.
> > >
> > > Attached updated version should take into account that typmod may be
> > > hiding under domains.  Please test.
> >
>
> SELECT JSON_VALUE(jsonb '111', '$' RETURNING queryfuncs_char2 default
> '13' on error);
> return
> ERROR:  value too long for type character(2)
> should return 13
>
> I found out the source of the problem is in coerceJsonExprOutput
> /*
> * Use cast expression for domain types; we need CoerceToDomain here.
> */
> if (get_typtype(returning->typid) != TYPTYPE_DOMAIN)
> {
> jsexpr->use_io_coercion = true;
> return;
> }

Thanks for this test case and the analysis.  Yes, using a cast
expression for coercion to the RETURNING type generally seems to be a
source of many problems that could've been solved by fixing things so
that only use_io_coercion and use_json_coercion are enough to handle
all the cases.

I've attempted that in the attached 0001, which removes
JsonExpr.coercion_expr and a bunch of code around it.

0002 is now the original patch minus the changes to make
JSON_EXISTS(), JSON_QUERY(), and JSON_VALUE() behave as we would like,
because the changes in 0001 covers them. The changes for JsonBehavior
expression coercion as they were in the last version of the patch are
still needed, but I decided to move those into 0001 so that the
changes for query functions are all in 0001 and those for constructors
in 0002.  It would be nice to get rid of that coerce_to_target_type()
call to coerce the "behavior expression" to RETURNING type, but I'm
leaving that as a task for another day.

-- 
Thanks, Amit Langote

v3-0001-SQL-JSON-Always-coerce-JsonExpr-result-at-runtime.patch
Description: Binary data

v3-0002-SQL-JSON-Use-implicit-casts-for-RETURNING-type-wi.patch
Description: Binary data

Re: Doc: fix track_io_timing description to mention pg_stat_io

2024-06-27 Thread Hajime.Matsunaga

Hi,

pg_stat_io has I/O statistics that are collected when track_io_timing is
enabled, but it is not mentioned in the description of track_io_timing.
I think it's better to add a description of pg_stat_io for easy reference.
What do you think?
Its always good to add new things.
Thanks for your positive comments.

Regards,
--
Hajime Matsunaga
NTT DATA Group Corporation

[PATCH] Fix docs to use canonical links

2024-06-27 Thread Joel Jacobson

Hello hackers,

During work in the separate thread [1], I discovered more cases
where the link in docs wasn't the canonical link [2].

[1] 
https://postgr.es/m/cakfquwyex9pj9g0zhjewsmsbnquygh+fycw-66ezjfvg4ko...@mail.gmail.com
[2] https://en.wikipedia.org/wiki/Canonical_link_element

The. below script e.g. doesn't parse SGML, and is broken in some other ways
also, but probably good enough to suggest changes that can then be manually
carefully verified.

```
#!/bin/bash
output_file="changes.log"
> $output_file
extract_canonical() {
  local url=$1
  canonical=$(curl -s "$url" | sed -n 's/.*> $output_file
echo "+$canonical" >> $output_file
echo $canonical
  else
echo $url
  fi
}
find . -type f -name '*.sgml' | while read -r file; do
  urls=$(sed -n 's/.*\(https:\/\/[^"]*\).*/\1/p' "$file")
  for url in $urls; do
canonical_url=$(extract_canonical "$url")
if [[ "$canonical_url" != "$url" ]]; then
  # Replace the original URL with the canonical URL in the file
  sed -i '' "s|$url|$canonical_url|g" "$file"
fi
  done
done
```

Most of what it found was indeed correct, but I had to undo some mistakes it 
did.

All the changes in the attached patch have been manually verified, by clicking
the original link, and observing the URL seen in the browser.

/Joel

0001-Fix-docs-to-use-canonical-links.patch
Description: Binary data

Re: Pgoutput not capturing the generated columns

2024-06-27 Thread Peter Smith

Hi, here are some patch v11-0001 comments.

(BTW, I had difficulty reviewing this because something seemed strange
with the changes this patch made to the test_decoding tests).

==
General

1. Patch name

Patch name does not need to quote 'logical replication'

~

2. test_decoding tests

Multiple test_decoding tests were failing for me. There is something
very suspicious about the unexplained changes the patch made to the
expected "binary.out" and "decoding_into_rel.out" etc. I REVERTED all
those changes in my nitpicks top-up to get everything working. Please
re-confirm that all the test_decoding tests are OK!

==
Commit Message

3.
Since you are including the example usage for test_decoding, I think
it's better to include the example usage of CREATE SUBSCRIPTION also.

==
contrib/test_decoding/expected/binary.out

4.
 SELECT 'init' FROM
pg_create_logical_replication_slot('regression_slot',
'test_decoding');
- ?column?
---
- init
-(1 row)
-
+ERROR:  replication slot "regression_slot" already exists

Huh? Why is this unrelated expected output changed by this patch?

The test_decoding test fails for me unless I REVERT this change!! See
my nitpicks diff.

==
.../expected/decoding_into_rel.out

5.
-SELECT 'stop' FROM pg_drop_replication_slot('regression_slot');
- ?column?
---
- stop
-(1 row)
-

Huh? Why is this unrelated expected output changed by this patch?

The test_decoding test fails for me unless I REVERT this change!! See
my nitpicks diff.

==
.../test_decoding/sql/decoding_into_rel.sql

6.
-SELECT 'stop' FROM pg_drop_replication_slot('regression_slot');
+SELECT 'stop' FROM pg_drop_replication_slot('regression_slot');

Huh, Why does this patch change this code at all? I REVERTED this
change. See my nitpicks diff.

==
.../test_decoding/sql/generated_columns.sql

(see my nitpicks replacement file for this test)

7.
+-- test that we can insert the result of a 'include_generated_columns'
+-- into the tables created. That's really not a good idea in practical terms,
+-- but provides a nice test.

NITPICK - I didn't understand the point of this comment.  I updated
the comment according to my understanding.

~

NITPICK - The comment "when 'include-generated-columns' is not set
then columns will not be replicated" is the opposite of what the
result is. I changed this comment.

NITPICK - modified and unified wording of some of the other comments

NITPICK - changed some blank lines

==
contrib/test_decoding/test_decoding.c

8.
+ else if (strcmp(elem->defname, "include-generated-columns") == 0)
+ {
+ if (elem->arg == NULL)
+ data->include_generated_columns = true;

Is there any way to test that "elem->arg == NULL" in the
generated.sql? OTOH, if it is not possible to get here then is the
code even needed?

==
doc/src/sgml/ddl.sgml

9.
  
-  Generated columns are skipped for logical replication and cannot be
-  specified in a CREATE PUBLICATION column list.
+  'include_generated_columns' option controls whether generated columns
+  should be included in the string representation of tuples during
+  logical decoding in PostgreSQL. The default is true.
  

NITPICK - Use proper markdown instead of single quotes for the parameter.

NITPICK - I think this can be reworded slightly to provide a
cross-reference to the CREATE SUBSCRIPTION parameter for more details
(which means then we can avoid repeating details like the default
value here). PSA my nitpicks diff for an example of how I thought docs
should look.

==
doc/src/sgml/protocol.sgml

10.
+The default is true.

No, it isn't. AFAIK you made the default behaviour true only for
'test_decoding', but the default for CREATE SUBSCRIPTION remains
*false* because that is the existing PG17 behaviour. And the default
for the 'include_generated_columns' in the protocol is *also* false to
match the CREATE SUBSCRIPTION default.

e.g. libpqwalreceiver.c only sets ", include_generated_columns 'true'"
when options->proto.logical.include_generated_columns
e.g. worker.c says: options->proto.logical.include_generated_columns =
MySubscription->includegencols;
e.g. subscriptioncmds.c sets default: opts->include_generated_columns = false;

(This confirmed my previous review expectation that using different
default behaviours for test_decoding and pgoutput would surely lead to
confusion)

~~~

11.
- 
-  Next, the following message part appears for each column included in
-  the publication (except generated columns):
- 
-

AFAIK you cannot just remove this entire paragraph because I thought
it was still relevant to talking about "... the following message
part". But, if you don't want to explain and cross-reference about
'include_generated_columns' then maybe it is OK just to remove the
"(except generated columns)" part?

==
src/test/subscription/t/011_generated.pl

NITPICK - comment typo /tab2/tab3/
NITPICK - remove some blank lines

~~~

12.
# the column was NOT replicated (the res

Re: Doc: fix track_io_timing description to mention pg_stat_io

2024-06-27 Thread Kashif Zeeshan

On Thu, Jun 27, 2024 at 2:06 PM  wrote:

> Hi,
>
> pg_stat_io has I/O statistics that are collected when track_io_timing is
> enabled, but it is not mentioned in the description of track_io_timing.
> I think it's better to add a description of pg_stat_io for easy reference.
> What do you think?
>
Its always good to add new things.

>
> Regards,
> --
> Hajime Matsunaga
> NTT DATA Group Corporation
>

Doc: fix track_io_timing description to mention pg_stat_io

2024-06-27 Thread Hajime.Matsunaga

Hi,

pg_stat_io has I/O statistics that are collected when track_io_timing is
enabled, but it is not mentioned in the description of track_io_timing.
I think it's better to add a description of pg_stat_io for easy reference.
What do you think?

Regards,
--
Hajime Matsunaga
NTT DATA Group Corporation


guc-track-io-timing.patch
Description: guc-track-io-timing.patch

Re: Vacuum statistics

2024-06-27 Thread Andrei Zubkov

Hello!

On Thu, 27/06/2024 at 10:39 +0900, Masahiko Sawada:
> On Fri, May 31, 2024 at 4:19 AM Andrei Zubkov 
> wrote:
> > As the vacuum process is a backend it has a workload
> > instrumentation.
> > We have all the basic counters available such as a number of blocks
> > read, hit and written, time spent on I/O, WAL stats and so on..
> > Also,
> > we can easily get some statistics specific to vacuum activity i.e.
> > number of tuples removed, number of blocks removed, number of VM
> > marks
> > set and, of course the most important metric - time spent on vacuum
> > operation.
> 
> I've not reviewed the patch closely but it sounds helpful for users.
> I
> would like to add a statistic, the high-water mark of memory usage of
> dead tuple TIDs. Since the amount of memory used by TidStore is hard
> to predict, I think showing the high-water mark would help users to
> predict how much memory they set to maintenance_work_mem.
> 
Thank you for your interest on this patch. I've understand your idea.
The obvious goal of it is to avoid expensive index multi processing
during vacuum of the heap. Provided statistics in the patch contain the
index_vacuum_count counter for each table which can be compared to the
pg_stat_all_tables.vacuum_count to detect specific relation index
multi-passes. Previous setting of maintenance_work_mem is known. Usage
of TidStore should be proportional to the amount of dead-tuples vacuum
workload on the table, so as the first evaluation we can take the
number of index passes per one heap pass as a maintenance_work_mem
multiplier.

But there is a better way. Once we detected the index multiprocessing
we can lower the vacuum workload for the heap pass making vacuum a
little bit more aggressive for this particular relation. I mean, in
such case increasing maintenance_work_mem is not only decision.

Suggested high-water mark statistic can't be used as cumulative
statistic - any high-water mark statistic as maximim-like statistic is
valid for certain time period thus should be reset on some kind of
schedule. Without resets it should reach 100% once under the heavy load
and stay there forever.

Said that such high-water mark seems a little bit unclear and
complicated for the DBA. It seems redundant to me right now. I can see
the main value of such statistic is to avoid too large
maintenance_work_mem setting. But I can't see really dramatic
consequences of that. Maybe I've miss something..

-- 
Andrei Zubkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [PATCH] Add ACL (Access Control List) acronym

2024-06-27 Thread Joel Jacobson

On Wed, Jun 26, 2024, at 18:54, David G. Johnston wrote:
> On Wed, Jun 26, 2024 at 8:47 AM Nathan Bossart  
> wrote:
>> On Wed, Jun 26, 2024 at 07:58:55AM -0700, David G. Johnston wrote:
>> > On Wed, Jun 26, 2024 at 7:52 AM Joel Jacobson  wrote:
>> >> Want me to fix that or will the committer handle that?
>> >>
>> >> I found some more similar cases in acronyms.sgml.
>> >
>> > Given this I'd be OK with committing as-is in the name of matching existing
>> > project style.  Then bringing up this inconsistency as a separate concern
>> > to be bulk fixed as part of implementing a new policy on what to check for
>> > and conform to when establishing acronyms in our documentation.
>> > 
>> > Otherwise the author (you) should make the change here - the committer
>> > wouldn't be expected to know to do that from the discussion.

OK, I've made the change, new patch attached.

>> If I was writing these patches, I'd create a separate 0001 patch to fix the
>> existing problems, then 0002 would be just the new stuff (without the
>> inconsistency).  But that's just what I'd do; there's no problem with doing
>> it the other way around.
>> 
>
> Agreed, if Joel wants to write both.  But as the broader fix shouldn't 
> block adding a new acronym, it doesn't make sense to insist on this 
> approach.  Consistency makes sense though doing it the expected way 
> would be OK as well.  Either way, assuming the future patch 
> materializes and gets committed the end state is the same, and the path 
> to it doesn't really matter.

I'll start a new separate thread about fixing the other non-canonical URLs.

/Joel

v6-0001-Add-ACL-Access-Control-List-acronym.patch
Description: Binary data

Re: Adminpack removal

2024-06-27 Thread Matthias van de Meent

On Thu, 27 Jun 2024, 07:34 Philippe BEAUDOIN,  wrote:
>
> Hi,
>
> I have just tested PG17 beta1 with the E-Maj solution I maintain. The
> only issue I found is the removal of the adminpack contrib.
>
> In the emaj extension, which is the heart of the solution, and which is
> written in plpgsql, the code just uses the pg_file_unlink() function to
> automatically remove files produced by COPY TO statements when no rows
> have been written. In some specific use cases, it avoids the user to get
> a few interesting files among numerous empty files in a directory. I
> have found a workaround. That's a little bit ugly, but it works. So this
> is not blocking for me.
>
> FYI, the project's repo is on github (https://github.com/dalibo/emaj),
> which was supposed to be scanned to detect potential adminpack usages.

The extension at first glance doesn't currently seem to depend on
adminpack: it is not included in the control file as dependency, and
has not been included as a dependency since the creation of that file.

Where else would you expect us to search for dependencies?


Kind regards,

Matthias van de Meent

Re: doc: modify the comment in function libpqrcv_check_conninfo()

On Thu, 27 Jun 2024 at 09:09, ikedarintarof
 wrote:
>
> Thank you for your comment!
>
> I've added the must_use_password argument. A new patch is attached.

s/whther/whether

nit: "it will do nothing" sounds a bit strange to me (but I'm not
native english). Something like this reads more natural to me:

an error. If must_use_password is true, the function raises an error
if no password is provided in the connection string. In any other case
it successfully completes.

Re: Patch bug: Fix jsonpath .* on Arrays

2024-06-27 Thread Michael Paquier

On Thu, Jun 27, 2024 at 11:53:14AM +0700, Stepan Neretin wrote:
> HI! Now it looks good for me.

The tests of jsonb_jsonpath.sql include a lot of patterns for @? and
jsonb_path_query with the lax and strict modes, so shouldn't these
additions be grouped closer to the existing tests rather than added at 
the end of the file?
--
Michael


signature.asc
Description: PGP signature

Re: libpq: Fix lots of discrepancies in PQtrace