Re: cost delay brainstorming

[Andy Fan]

Hi, 
> Hi,
>
> On 2024-06-17 15:39:27 -0400, Robert Haas wrote:
>> As I mentioned in my talk at 2024.pgconf.dev, I think that the biggest
>> problem with autovacuum as it exists today is that the cost delay is
>> sometimes too low to keep up with the amount of vacuuming that needs
>> to be done.
>
> I agree it's a big problem, not sure it's *the* problem. But I'm happy to see
> it improved anyway, so it doesn't really matter.

In my past knowldege,  another big problem is the way we triggers an
autovacuum on a relation. With the current stategy, if we have lots of
writes between 9:00 AM ~ 5:00 PM,  it is more likely to triggers an
autovauum at that time which is the peak time of application as well.

If we can trigger vacuum at off-peak time, like 00:00 am ~ 05:00 am,
even we use lots of resource, it is unlikly cause any issue.

> One issue around all of this is that we pretty much don't have the tools to
> analyze autovacuum behaviour across a larger number of systems in a realistic
> way :/.  I find my own view of what precisely the problem is being heavily
> swayed by the last few problematic cases I've looked t.
>
>
>> I think we might able to get fairly far by observing that if the
>> number of running autovacuum workers is equal to the maximum allowable
>> number of running autovacuum workers, that may be a sign of trouble,
>> and the longer that situation persists, the more likely it is that
>> we're in trouble. So, a very simple algorithm would be: If the maximum
>> number of workers have been running continuously for more than, say,
>> 10 minutes, assume we're falling behind and exempt all workers from
>> the cost limit for as long as the situation persists. One could
>> criticize this approach on the grounds that it causes a very sudden
>> behavior change instead of, say, allowing the rate of vacuuming to
>> gradually increase. I'm curious to know whether other people think
>> that would be a problem.
>
> Another issue is that it's easy to fall behind due to cost limits on systems
> where autovacuum_max_workers is smaller than the number of busy tables.
>
> IME one common situation is to have a single table that's being vacuumed too
> slowly due to cost limits, with everything else keeping up easily.
>
>
>> I think it might be OK, for a couple of reasons:
>>
>> 1. I'm unconvinced that the vacuum_cost_delay system actually prevents
>> very many problems. I've fixed a lot of problems by telling users to
>> raise the cost limit, but virtually never by lowering it. When we
>> lowered the delay by an order of magnitude a few releases ago -
>> equivalent to increasing the cost limit by an order of magnitude - I
>> didn't personally hear any complaints about that causing problems. So
>> disabling the delay completely some of the time might just be fine.
>
> I have seen disabling cost limits cause replication setups to fall over
> because the amount of WAL increases beyond what can be
> replicated/archived/replayed.  It's very easy to reach the issue when syncrep
> is enabled.

Usually applications have off-peak time, if we can use such character, we
might have some good result. But I know it is hard to do in PostgreSQL
core, I ever tried it in an external system (external minotor +
crontab-like). I can see the CPU / Memory ussage of autovacuum reduced a
lot at the daytime (application peak time).


>> 1a. Incidentally, when I have seen problems because of vacuum running
>> "too fast", it's not been because it was using up too much I/O
>> bandwidth, but because it's pushed too much data out of cache too
>> quickly. A long overnight vacuum can evict a lot of pages from the
>> system page cache by morning - the ring buffer only protects our
>> shared_buffers, not the OS cache. I don't think this can be fixed by
>> rate-limiting vacuum, though: to keep the cache eviction at a level
>> low enough that you could be certain of not causing trouble, you'd
>> have to limit it to an extremely low rate which would just cause
>> vacuuming not to keep up. The cure would be worse than the disease at
>> that point.
>
> I've seen versions of this too. Ironically it's often made way worse by
> ringbuffers, because even if there is space is shared buffers, we'll not move
> buffers there, instead putting a lot of pressure on the OS page cache.

I can understand the pressure on the OS page cache, but I thought the
OS page cache can be reused easily for any other purposes. Not sure what
outstanding issue it can cause.  

> - Longrunning transaction prevents increasing relfrozenxid, we run autovacuum
>   over and over on the same relation, using up the whole cost budget. This is
>   particularly bad because often we'll not autovacuum anything else, building
>   up a larger and larger backlog of actual work.

Could we maintain a pg_class.last_autovacuum_min_xid during vacuum? So
if we compare the OldestXminXid with pg_class.last_autovacuum_min_xid
before doing the real work. I think we can use a in-place update on it
to avoi

Re: Shared detoast Datum proposal

[Andy Fan]

Hi,

Let's me clarify the current state of this patch and what kind of help
is needed.  You can check [1] for what kind of problem it try to solve
and what is the current approach.

The current blocker is if the approach is safe (potential to memory leak
crash etc.).  And I want to have some agreement on this step at least.

"""
When we access [some desired] toast column in EEOP_{SCAN|INNER_OUTER}_VAR
steps, we can detoast it immediately and save it back to
slot->tts_values. so the later ExprState can use the same detoast version
all the time.  this should be more the most effective and simplest way.
"""

IMO, it doesn't break any design of TupleTableSlot and should be
safe. Here is my understanding of TupleTableSlot, please correct me if
anything I missed.

(a). TupleTableSlot define tts_values[*] / tts_isnulls[*] at the high level.
  When the upper layer want a attnum, it just access tts_values/nulls[n].

(b). TupleTableSlot has many different variants, depends on how to get
  tts_values[*] from them, like VirtualTupleTableSlot, HeapTupleTableSlot,
  how to manage its resource (mainly memory) when the life cycle is
  done, for example BufferHeapTupleTableSlot. 
  
(c). TupleTableSlot defines a lot operation against on that, like
getsomeattrs, copy, clear and so on, for the different variants.  
  
the impacts of my proposal on the above factor:

(a). it doesn't break it clearly,
(b). it adds a 'detoast' step for step (b) when fill the tts_values[*],
(this is done conditionally, see [1] for the overall design, we
currently focus the safety). What's more, I only added the step in 
EEOP_{SCAN|INNER_OUTER}_VAR_TOAST step, it reduce the impact in another
step.
(c). a special MemoryContext in TupleTableSlot is introduced for the
detoast datum, it is reset whenever the TupleTableSlot.tts_nvalid
= 0. and I also double check every operation defined in
TupleTableSlotOps, looks nothing should be specially handled.

Robert has expressed his concern as below and wanted Andres to have a
look at, but unluckly Andres doesn't have such time so far:( 

> Robert Haas  writes:
>
>> On Wed, May 22, 2024 at 9:46 PM Andy Fan  wrote:
>>> Please give me one more chance to explain this. What I mean is:
>>>
>>> Take SELECT f(a) FROM t1 join t2...; for example,
>>>
>>> When we read the Datum of a Var, we read it from tts->tts_values[*], no
>>> matter what kind of TupleTableSlot is.  So if we can put the "detoast
>>> datum" back to the "original " tts_values, then nothing need to be
>>> changed.
>>
>> Yeah, I don't think you can do that.
>>
>>> - Saving the "detoast datum" version into tts_values[*] doesn't break
>>>   the above semantic and the ExprEval engine just get a detoast version
>>>   so it doesn't need to detoast it again.
>>
>> I don't think this is true. If it is true, it needs to be extremely
>> well-justified. Even if this seems to work in simple cases, I suspect
>> there will be cases where it breaks badly, resulting in memory leaks
>> or server crashes or some other kind of horrible problem that I can't
>> quite imagine. Unfortunately, my knowledge of this code isn't
>> fantastic, so I can't say exactly what bad thing will happen, and I
>> can't even say with 100% certainty that anything bad will happen, but
>> I bet it will. It seems like it goes against everything I understand
>> about how TupleTableSlots are supposed to be used. (Andres would be
>> the best person to give a definitive answer here.)

I think having a discussion of the design of TupleTableSlots would be
helpful for the progress since in my knowldege it doesn't against
anything of the TupleTaleSlots :(, as I stated above. I'm sure I'm
pretty open on this. 

>>
>>> - The keypoint is the memory management and effeiciency. for now I think
>>>   all the places where "slot->tts_nvalid" is set to 0 means the
>>>   tts_values[*] is no longer validate, then this is the place we should
>>>   release the memory for the "detoast datum".  All the other places like
>>>   ExecMaterializeSlot or ExecCopySlot also need to think about the
>>>   "detoast datum" as well.
>>
>> This might be a way of addressing some of the issues with this idea,
>> but I doubt it will be acceptable. I don't think we want to complicate
>> the slot API for this feature. There's too much downside to doing
>> that, in terms of performance and understandability.

Complexity: I double check the code, it has very limitted changes on the
TupleTupleSlot APIs (less than 10 rows).  see tuptable.h.

Performance: by design, it just move the chance of detoast action
*conditionly* and put it back to tts_values[*], there is no extra
overhead to find out the detoast version for sharing.

Understandability: based on how do we understand TupleTableSlot:) 

[1] https://www.postgresql.org/message-id/87il4jrk1l.fsf%40163.com

-- 
Best Regards
Andy Fan

Re: Add pg_get_acl() function get the ACL for a database object

[Joel Jacobson]

On Fri, Jun 21, 2024, at 05:25, Michael Paquier wrote:
> Interesting idea.
>
> I am not really convinced that the regproc and regclass overloads are
> really necessary, considering the fact that one of the goals
> mentioned, as far as I understand, is to be able to get an idea of the
> ACLs associated to an object with its dependencies in pg_depend and/or 
> pg_shdepend.  Another one is to reduce the JOIN burden when querying
> a set of them, like attribute ACLs.

Overloads moved to a second patch, which can be applied
on top of the first one. I think they would be quite nice, but I could
also live without them.

> Perhaps the documentation should add one or two examples to show this
> point?

Good point, added.

>
> +tup = get_catalog_object_by_oid(rel, Anum_oid, objectId);
> +if (!HeapTupleIsValid(tup))
> +elog(ERROR, "cache lookup failed for object %u of catalog 
> \"%s\"",
> +objectId, RelationGetRelationName(rel));
>
> get_catalog_object_by_oid() is handled differently here than in
> functions line pg_identify_object().  Shouldn't we return NULL for
> this case?  That would be more useful when using this function with
> one or more large scans.

Right, I've changed the patch accordingly.

On Fri, Jun 21, 2024, at 05:48, Isaac Morland wrote:
> Those were just examples. I think for completeness there should be 5 
> overloads:
>
> [input type] → [relation.aclattribute]
> regproc/regprocedure → pg_proc.proacl
> regtype → pg_type.typacl
> regclass → pg_class.relacl
> regnamespace → pg_namespace.nspacl
>
> I believe the remaining reg* types don't correspond to objects with 
> ACLs, and the remaining ACL fields are for objects which don't have a 
> corresponding reg* type.
>
> In general I believe the reg* types are underutilized. All over the 
> place I see examples where people write code to generate SQL statements 
> and they take schema and object name and then format with %I.%I when 
> all that is needed is a reg* value and then format it with a simple %s 
> (of course, need to make sure the SQL will execute with the same 
> search_path as when the SQL was generated, or generate with an empty 
> search_path).

I've added regtype and regnamespace overloads to the second patch.

On Fri, Jun 21, 2024, at 05:58, Isaac Morland wrote:
> On Thu, 20 Jun 2024 at 23:44, Tom Lane  wrote:
>> Doesn't that result in "cannot resolve ambiguous function call"
>> failures?
>
> If you try to pass an oid directly, as a value of type oid, you should 
> get "function is not unique". But if you cast a string or numeric value 
> to the appropriate reg* type for the object you are using, it should 
> work fine.

Yes, I can confirm that's the case, it works fine when casting a string
to reg* type.

/Joel

v4-0001-Add-pg_get_acl.patch
Description: Binary data


0002-Add-pg_get_acl-overloads.patch
Description: Binary data

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

[Alena Rybakina]

On 21.06.2024 23:35, Robert Haas wrote:

On Fri, Jun 21, 2024 at 1:05 PM Alena Rybakina
  wrote:

I'm confused, I have seen that we have two threads [1] and [2] about this 
thread and I haven't found any explanation for how they differ.

And I don't understand, why am I not listed as the author of the patch? I was 
developing the first part of the patch before Andrey came to review it [3] and 
his first part hasn't changed much since then.

v25 still lists you as an author (in fact, the first author) but I
can't say why we have two CommitFest entries. Surely that's a mistake.


Sorry, maybe I was overreacting.

Thank you very much for taking the time to do a detailed review!

On 22.06.2024 00:07, Alexander Korotkov wrote:

Sorry, I didn't notice that the [1] commitfest entry exists and
created the [2] commitfest entry.  I'm removed [2].

Thank you!


On the patch itself, I'm really glad we got to a design where this is
part of planning, not parsing. I'm not sure yet whether we're doing it
at the right time within the planner, but I think this *might* be
right, whereas the old way was definitely wrong.


It's hard to tell, but I think it might be one of the good places to 
apply transformation. Let me describe a brief conclusion on the two 
approaches.


In the first approach, we definitely did not process the extra "OR" 
expressions in the first approach, since they were packaged as an Array. 
It could lead to the fact that less planning time would be spent on the 
optimizer.


Also the selectivity for Array expressions is estimated better, which 
could lead to the generation of a more optimal plan, but, to be honest, 
this is just an observation from changes in regression tests and, in 
general, how the process of calculating the selectivity of a complex 
expression works.


 SELECT * FROM check_estimated_rows('SELECT * FROM 
functional_dependencies WHERE (a = 1 OR a = 51) AND b = ''1''');


  estimated | actual

 ---+

-    99 |    100

+   100 |    100

 (1 row)

 SELECT * FROM check_estimated_rows('SELECT * FROM 
functional_dependencies WHERE (a = 1 OR a = 51) AND (b = ''1'' OR b = 
''2'')');


  estimated | actual

 ---+

-    99 |    100

+   100 |    100

 (1 row)

 SELECT * FROM check_estimated_rows('SELECT * FROM 
functional_dependencies WHERE (a = 1 OR a = 2 OR a = 51 OR a = 52) AND 
(b = ''1'' OR b = ''2'')');


  estimated | actual

 ---+

-   197 |    200

+   200 |    200


In addition, we do not have new equivalence classes, since some “Or” 
expressions are not available for their formation. This can result in 
reduced memory and time spent generating the query plan, especially in 
partitions.


Speaking of the main disadvantages, we do not give the optimizer the 
opportunity to generate a plan using BitmapScan, which can lead to the 
generation of a suboptimal plan, but in the current approach the same 
thing happens [0].


And the second one might be related the lack of generation Equivalence 
Classes and generation of useful pathkeysas a result, so we could miss 
an optimal plan again. But I haven't caught something like this on 
practice. I see we won't have such problems if we apply the 
transformation later.


Overall, I have not yet noticed any very different parts from what was 
in the first approach: I didn’t see any significant degradation or 
improvement, which is good, but so far the main problem with the 
degradation of the plan has not yet been solved, that is, we have not 
escaped from the main problems.


Andrei mentioned the problem in the second approach about updating 
references to expressions in RestrictInfo [1] lists, because the can be 
used in different variables during the formation of the query plan. As 
the practice of Self-join removal [2] has shown, this can be expensive, 
but feasible.


By applying the transformation at the analysis stage in the first 
approach, because no links were created, so we did not encounter such 
problems, so this approach was more suitable than the others.


[0] 
https://www.postgresql.org/message-id/7d5aed92-d4cc-4b76-8ae0-051d182c9eec%40postgrespro.ru


[1] 
https://www.postgresql.org/message-id/6850c306-4e9d-40b7-8096-1f3c7d29cd9e%40postgrespro.ru


[2] https://commitfest.postgresql.org/48/5043/


What exactly is the strategy around OR-clauses with type differences?
If I'm reading the code correctly, the first loop requires an exact
opno match, which presumably implies that the constant-type elements
are of the same type. But then why does the second loop need to use
coerce_to_common_type?


It needs to transform all similar constants to one type, because some 
constants of "OR" expressions can belong others, like the numeric and 
int types. Due to the fact that array structure demands that all types 
must be belonged to one type, so for this reason we applied this procedure.


You can find the similar strategy in transformAExprIn function, when we 
transform 

Re: call for applications: mentoring program for code contributors

[Andreas 'ads' Scherbaum]

On 20/06/2024 19:12, Robert Haas wrote:



I'm working to start a mentoring program where code contributors can
be mentored by current committers. Applications are now open:
https://forms.gle/dgjmdxtHYXCSg6aB7



Periodically, someone -- most likely not me, since a few people have
been kind enough to offer help -- will contact mentors and mentees to
get feedback on how things are going. We'll use this feedback to
improve the program, which might involve adjusting mentoring
assignments, or might involve taking such other actions as the
situation may suggest.


I'm offering to help with this part.

--
Andreas 'ads' Scherbaum
German PostgreSQL User Group
European PostgreSQL User Group - Board of Directors
Volunteer Regional Contact, Germany - PostgreSQL Project

Re: race condition in pg_class

[Noah Misch]

On Thu, Jun 13, 2024 at 05:35:49PM -0700, Noah Misch wrote:
> On Mon, Jun 10, 2024 at 07:45:25PM -0700, Noah Misch wrote:
> > On Thu, Jun 06, 2024 at 09:48:51AM -0400, Robert Haas wrote:
> > > It's not this patch set's fault, but I'm not very pleased to see that
> > > the injection point wait events have been shoehorned into the
> > > "Extension" category
> > 
> > I've replied on that branch of the thread.
> 
> 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?

Two more test-related changes compared to v3:

- In inplace010-tests, add to 027_stream_regress.pl a test that catalog
  contents match between primary and standby.  If one of these patches broke
  replay of inplace updates, this would help catch it.

- In inplace031-inj-wait-event, make sysviews.sql indifferent to whether
  InjectionPoint wait events exist.  installcheck need this if other activity
  created such an event since the last postmaster restart.
Author: Noah Misch 
Commit: Noah Misch 

Improve test coverage for changes to inplace-updated catalogs.

This covers both regular and inplace changes, since bugs arise at their
intersection.  Where marked, these witness extant bugs.  Back-patch to
v12 (all supported versions).

Reviewed by FIXME.

Discussion: https://postgr.es/m/20240512232923.aa.nmi...@google.com

diff --git a/src/bin/pgbench/t/001_pgbench_with_server.pl 
b/src/bin/pgbench/t/001_pgbench_with_server.pl
index 59ea538..956e290 100644
--- a/src/bin/pgbench/t/001_pgbench_with_server.pl
+++ b/src/bin/pgbench/t/001_pgbench_with_server.pl
@@ -68,6 +68,34 @@ $node->pgbench(
  "CREATE TYPE pg_temp.e AS ENUM ($labels); DROP TYPE 
pg_temp.e;"
});
 
+# Test inplace updates from VACUUM concurrent with heap_update from GRANT.
+# The PROC_IN_VACUUM environment can't finish MVCC table scans consistently,
+# so this fails rarely.  To reproduce consistently, add a sleep after
+# GetCatalogSnapshot(non-catalog-rel).
+Test::More->builder->todo_start('PROC_IN_VACUUM scan breakage');
+$node->safe_psql('postgres', 'CREATE TABLE ddl_target ()');
+$node->pgbench(
+   '--no-vacuum --client=5 --protocol=prepared --transactions=50',
+   0,
+   [qr{processed: 250/250}],
+   [qr{^$}],
+   'concurrent GRANT/VACUUM',
+   {
+   '001_pgbench_grant@9' => q(
+   DO $$
+   BEGIN
+   PERFORM pg_advisory_xact_lock(42);
+   FOR i IN 1 .. 10 LOOP
+   GRANT SELECT ON ddl_target TO PUBLIC;
+   REVOKE SELECT ON ddl_target FROM PUBLIC;
+   END LOOP;
+   END
+   $$;
+),
+   '001_pgbench_vacuum_ddl_target@1' => "VACUUM ddl_target;",
+   });
+Test::More->builder->todo_end;
+
 # Trigger various connection errors
 $node->pgbench(
'no-such-database',
diff --git a/src/test/isolation/expected/eval-plan-qual.out 
b/src/test/isolation/expected/eval-plan-qual.out
index 0237271..032d420 100644
--- a/src/test/isolation/expected/eval-plan-qual.out
+++ b/src/test/isolation/expected/eval-plan-qual.out
@@ -1337,3 +1337,29 @@ a|b|c|   d
 2|2|2|1004
 (2 rows)
 
+
+starting permutation: sys1 sysupd2 c1 c2
+step sys1: 
+   UPDATE pg_class SET reltuples = 123 WHERE oid = 'accounts'::regclass;
+
+step sysupd2: 
+   UPDATE pg_class SET reltuples = reltuples * 2
+   WHERE oid = 'accounts'::regclass;
+ 
+step c1: COMMIT;
+step sysupd2: <... completed>
+step c2: COMMIT;
+
+starting permutation: sys1 sysmerge2 c1 c2
+step sys1: 
+   UPDATE pg_class SET reltuples = 123 WHERE oid = 'accounts'::regclass;
+
+step sysmerge2: 
+   MERGE INTO pg_class
+   USING (SELECT 'accounts'::regclass AS o) j
+   ON o = oid
+   WHEN MATCHED THEN UPDATE SET reltuples = reltuples * 2;
+ 
+step c1: COMMIT;
+step sysmerge2: <... completed>
+step c2: COMMIT;
diff --git a/src/test/isolation/expected/inplace-inval.out 
b/src/test/isolation/expected/inplace-inval.out
new file mode 100644
index 000..67b34ad
--- /dev/null
+++ b/src/test/isolation/expected/inplace-inval.out
@@ -0,0 +1,32 @@
+Parsed test spec with 3 sessions
+
+starting permutation: cachefill3 cir1 cic2 ddl3 read1
+step cachefill3: TABLE newly_indexed;
+c
+-
+(0 row

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

[Alexander Korotkov]

Hi, Alena.

On Fri, Jun 21, 2024 at 8:05 PM Alena Rybakina
 wrote:
>
> I'm confused, I have seen that we have two threads [1] and [2] about this 
> thread and I haven't found any explanation for how they differ.
>
> And I don't understand, why am I not listed as the author of the patch? I was 
> developing the first part of the patch before Andrey came to review it [3] 
> and his first part hasn't changed much since then.
>
> If I wrote to the wrong person about it, then please tell me where.
>
> [1] https://commitfest.postgresql.org/48/4450/
>
> [2] https://commitfest.postgresql.org/48/5037/
>
> [3] 
> https://www.postgresql.org/message-id/b301dce1-09fd-72b1-834a-527ca428db5e%40yandex.ru

Sorry, I didn't notice that the [1] commitfest entry exists and
created the [2] commitfest entry.  I'm removed [2].

--
Regards,
Alexander Korotkov
Supabase

Re: libpq: Fix lots of discrepancies in PQtrace

[Nathan Bossart]

On Fri, Jun 21, 2024 at 11:22:05AM +0200, Jelte Fennema-Nio wrote:
> 0001 is some cleanup after f4b54e1ed9

Oops.  I'll plan on committing this after the 17beta2 release freeze is
lifted.

-- 
nathan

Re: What is a typical precision of gettimeofday()?

[Tom Lane]

Hannu Krosing  writes:
> This is my current patch which also adds running % and optionally uses
> faster way to count leading zeros, though I did not  see a change from
> that.

I've not read the patch yet, but I did create a CF entry [1]
to get some CI cycles on this.  The cfbot complains [2] about

[19:24:31.951] pg_test_timing.c: In function ‘output’:
[19:24:31.951] pg_test_timing.c:229:11: error: format ‘%ld’ expects argument of 
type ‘long int’, but argument 3 has type ‘int64’ {aka ‘long long int’} 
[-Werror=format=]
[19:24:31.951]   229 |printf("%*ld%*.4f  %*.4f %*lld\n",
[19:24:31.951]   |   ^~
[19:24:31.951]   230 |   Max(8, len1), i,
[19:24:31.951]   | ~
[19:24:31.951]   | |
[19:24:31.951]   | int64 {aka long long int}

which seems a bit confused, but anyway you cannot assume that int64 is
a match for "%ld", or "%lld" either.  What we generally do for this
elsewhere is to explicitly cast printf arguments to long long int.

Also there's this on Windows:

[19:23:48.231] ../src/bin/pg_test_timing/pg_test_timing.c(162): warning C4067: 
unexpected tokens following preprocessor directive - expected a newline

regards, tom lane

[1] https://commitfest.postgresql.org/48/5066/
[2] http://cfbot.cputube.org/highlights/all.html#5066

Re: allow changing autovacuum_max_workers without restarting

[Nathan Bossart]

On Tue, Jun 18, 2024 at 07:43:36PM -0500, Nathan Bossart wrote:
> On Tue, Jun 18, 2024 at 02:33:31PM -0700, Andres Freund wrote:
>> Another one:
>> 
>> Have a general cap of 64, but additionally limit it to something like
>>  max(1, min(WORKER_CAP, max_connections / 4))
>> 
>> so that cases like tap tests don't end up allocating vastly more worker slots
>> than actual connection slots.
> 
> That's a clever idea.  My only concern would be that we are tethering two
> parameters that aren't super closely related, but I'm unsure whether it
> would cause any problems in practice.

Here is an attempt at doing this.  I've added 0001 [0] and 0002 [1] as
prerequisite patches, which helps simplify 0003 a bit.  It probably doesn't
work correctly for EXEC_BACKEND builds yet.

I'm still not sure about this approach.  At the moment, I'm leaning towards
something more like v2 [2] where the upper limit is a PGC_POSTMASTER GUC
(that we would set very low for TAP tests).

[0] https://commitfest.postgresql.org/48/4998/
[1] https://commitfest.postgresql.org/48/5059/
[2] https://postgr.es/m/20240419154322.GA3988554%40nathanxps13

-- 
nathan
>From 69245426585052af1317c1bf3e564cae0c019f52 Mon Sep 17 00:00:00 2001
From: Nathan Bossart 
Date: Tue, 21 May 2024 14:02:22 -0500
Subject: [PATCH v5 1/3] add num_os_semaphores GUC

---
 doc/src/sgml/config.sgml| 14 +++
 doc/src/sgml/runtime.sgml   | 39 -
 src/backend/storage/ipc/ipci.c  |  6 -
 src/backend/utils/misc/guc_tables.c | 12 +
 4 files changed, 53 insertions(+), 18 deletions(-)

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 0c7a9082c5..087385cb4e 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -11234,6 +11234,20 @@ dynamic_library_path = 
'C:\tools\postgresql;H:\my_project\lib;$libdir'
   
  
 
+ 
+  num_os_semaphores (integer)
+  
+   num_os_semaphores configuration 
parameter
+  
+  
+  
+   
+Reports the number of semaphores that are needed for the server based
+on the number of allowed connections, worker processes, etc.
+   
+  
+ 
+
  
   ssl_library (string)
   
diff --git a/doc/src/sgml/runtime.sgml b/doc/src/sgml/runtime.sgml
index 2f7c618886..26f3cbe555 100644
--- a/doc/src/sgml/runtime.sgml
+++ b/doc/src/sgml/runtime.sgml
@@ -781,13 +781,13 @@ psql: error: connection to server on socket 
"/tmp/.s.PGSQL.5432" failed: No such

 SEMMNI
 Maximum number of semaphore identifiers (i.e., sets)
-at least ceil((max_connections + 
autovacuum_max_workers + max_wal_senders + max_worker_processes + 7) / 
16) plus room for other applications
+at least ceil(num_os_semaphores / 16) plus 
room for other applications

 

 SEMMNS
 Maximum number of semaphores system-wide
-ceil((max_connections + autovacuum_max_workers + 
max_wal_senders + max_worker_processes + 7) / 16) * 17 plus room for 
other applications
+ceil(num_os_semaphores / 16) * 17 plus room 
for other applications

 

@@ -836,30 +836,38 @@ psql: error: connection to server on socket 
"/tmp/.s.PGSQL.5432" failed: No such
 

 When using System V semaphores,
-PostgreSQL uses one semaphore per allowed 
connection
-(), allowed autovacuum worker process
-(), allowed WAL sender process
-(), and allowed background
-process (), in sets of 16.
+PostgreSQL uses one semaphore per allowed 
connection,
+worker process, etc., in sets of 16.
 Each such set will
 also contain a 17th semaphore which contains a magic
 number, to detect collision with semaphore sets used by
 other applications. The maximum number of semaphores in the system
 is set by SEMMNS, which consequently must be at least
-as high as max_connections plus
-autovacuum_max_workers plus 
max_wal_senders,
-plus max_worker_processes, plus one extra for each 16
-allowed connections plus workers (see the formula in  plus one extra for
+each set of 16 required semaphores (see the formula in ).  The parameter SEMMNI
 determines the limit on the number of semaphore sets that can
 exist on the system at one time.  Hence this parameter must be at
-least ceil((max_connections + autovacuum_max_workers + 
max_wal_senders + max_worker_processes + 7) / 16).
+least ceil(num_os_semaphores / 16).
 Lowering the number
 of allowed connections is a temporary workaround for failures,
 which are usually confusingly worded No space
 left on device, from the function semget.

 
+   
+The number of semaphores required by PostgreSQL
+is provided by the runtime-computed parameter
+num_os_semaphores, which can be determined before
+starting the server with a postgres command like:
+
+$ postgres -D $PGDATA -C num_os_semaphores
+
+The value of num_os_semaphores should be in

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

[Robert Haas]

On Fri, Jun 21, 2024 at 1:05 PM Alena Rybakina
 wrote:
> I'm confused, I have seen that we have two threads [1] and [2] about this 
> thread and I haven't found any explanation for how they differ.
>
> And I don't understand, why am I not listed as the author of the patch? I was 
> developing the first part of the patch before Andrey came to review it [3] 
> and his first part hasn't changed much since then.

v25 still lists you as an author (in fact, the first author) but I
can't say why we have two CommitFest entries. Surely that's a mistake.

On the patch itself, I'm really glad we got to a design where this is
part of planning, not parsing. I'm not sure yet whether we're doing it
at the right time within the planner, but I think this *might* be
right, whereas the old way was definitely wrong.

What exactly is the strategy around OR-clauses with type differences?
If I'm reading the code correctly, the first loop requires an exact
opno match, which presumably implies that the constant-type elements
are of the same type. But then why does the second loop need to use
coerce_to_common_type?

Also, why is the array built with eval_const_expressions instead of
something like makeArrayResult? There should be no need for general
expression evaluation here if we are just dealing with constants.

+ foreach(lc2, entry->exprs)
+ {
+ RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc2);
+
+ is_pushed_down = is_pushed_down || rinfo->is_pushed_down;
+ has_clone = has_clone || rinfo->is_pushed_down;
+ security_level = Max(security_level, rinfo->security_level);
+ required_relids = bms_union(required_relids, rinfo->required_relids);
+ incompatible_relids = bms_union(incompatible_relids,
rinfo->incompatible_relids);
+ outer_relids = bms_union(outer_relids, rinfo->outer_relids);
+ }

This seems like an extremely bad idea. Smushing together things with
different security levels (or a bunch of these other properties) seems
like it will break things. Presumably we wouldn't track these
properties on a per-RelOptInfo basis unless we needed an accurate idea
of the property value for each RelOptInfo. If the values are
guaranteed to match, then it's fine, but then we don't need this code
to merge possibly-different values. If they're not guaranteed to
match, then presumably we shouldn't merge into a single OR clause
unless they do.

On a related note, it looks to me like the tests focus too much on
simple cases. It seems like it's mostly testing cases where there are
no security quals, no weird operator classes, no type mismatches, and
few joins. In the cases where there are joins, it's an inner join and
there's no distinction between an ON-qual and a WHERE-qual. I strongly
suggest adding some test cases for weirder scenarios.

+ if (!OperatorIsVisible(entry->opno))
+ namelist = lappend(namelist,
makeString(get_namespace_name(operform->oprnamespace)));
+
+ namelist = lappend(namelist, makeString(pstrdup(NameStr(operform->oprname;
+ ReleaseSysCache(opertup);
+
+ saopexpr =
+ (ScalarArrayOpExpr *)
+ make_scalar_array_op(NULL,
+ namelist,
+ true,
+ (Node *) entry->expr,
+ (Node *) newa,
+ -1);

I do not think this is acceptable. We should find a way to get the
right operator into the ScalarArrayOpExpr without translating the OID
back into a name and then back into an OID.

+ /* One more trick: assemble correct clause */

This comment doesn't seem to make much sense. Some other comments
contain spelling mistakes. The patch should have comments in more
places explaining key design decisions.

+extern JumbleState *JumbleExpr(Expr *expr, uint64 *exprId);

This is no longer needed.

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

FreezeLimit underflows in pg14 and 15 causing incorrect behavior in heap_prepare_freeze_tuple

[Melanie Plageman]

While investigating a bug over on [1], I found that
vacuum_set_xid_limits() is calculating freezeLimit in an unsafe way on
at least Postgres 14 and 15.

limit = *oldestXmin - freezemin;
safeLimit = ReadNextTransactionId() - autovacuum_freeze_max_age;
if (TransactionIdPrecedes(limit, safeLimit))
limit = *oldestXmin;
*freezeLimit = limit;

All of these are unsigned, so it doesn't work very nicely when
freezemin (based on autovacuum_freeze_min_age) is larger than
oldestXmin and autovacuum_freeze_max_age is bigger than the next
transaction ID -- which is pretty common right after initdb, for
example.

I noticed the effect of this because FreezeLimit is saved in the
LVRelState and passed to heap_prepare_freeze_tuple() as cutoff_xid,
which is used to guard against freezing tuples that shouldn't be
frozen.

I didn't propose a fix because I just want to make sure I'm not
missing something first.

- Melanie

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

Re: New standby_slot_names GUC in PG 17

[Nathan Bossart]

On Fri, Jun 21, 2024 at 03:50:00PM -0400, Tom Lane wrote:
> Allow specification of physical standbys that must be synchronized
> before they are visible to subscribers (Hou Zhijie, Shveta Malik)
> 
> it seems like the name ought to have some connection to
> synchronization.  Perhaps something like "synchronized_standby_slots"?

IMHO that might be a bit too close to synchronous_standby_names.  But the
name might not be the only issue, as there is a separate proposal [0] to
add _another_ GUC to tie standby_slot_names to synchronous replication.  I
wonder if this could just be a Boolean parameter or if folks really have
use-cases for both a list of synchronous standbys and a separate list of
synchronous standbys for failover slots.

[0] 
https://postgr.es/m/CA%2B-JvFtq6f7%2BwAwSdud-x0yMTeMejUhpkyid1Xa_VNpRd_-oPw%40mail.gmail.com

-- 
nathan

Re: New standby_slot_names GUC in PG 17

[Muhammad Ikram]

Thanks Tom Lane. You are more insightful.

Regards,
Ikram

On Sat, Jun 22, 2024 at 12:50 AM Tom Lane  wrote:

> Muhammad Ikram  writes:
> > A humble input, as on primary we have #primary_slot_name = ''  then
> should
> > not it be okay to have standby_slot_names or standby_slot_name ? It seems
> > consistent with the Guc on primary.
> > Another suggestion is *standby_replication_slots*.
>
> IIUC, Bruce's complaint is that the name is too generic (which I agree
> with).  Given the stated functionality:
>
>  Allow specification of physical standbys that must be synchronized
>  before they are visible to subscribers (Hou Zhijie, Shveta Malik)
>
> it seems like the name ought to have some connection to
> synchronization.  Perhaps something like "synchronized_standby_slots"?
>
> I haven't read the patch, so I don't know if this name is especially
> on-point.  But "standby_slot_names" seems completely unhelpful, as
> a server could well have slots that are for standbys but are not to
> be included in this list.
>
> regards, tom lane
>


-- 
Muhammad Ikram

Re: New standby_slot_names GUC in PG 17

[Tom Lane]

Muhammad Ikram  writes:
> A humble input, as on primary we have #primary_slot_name = ''  then should
> not it be okay to have standby_slot_names or standby_slot_name ? It seems
> consistent with the Guc on primary.
> Another suggestion is *standby_replication_slots*.

IIUC, Bruce's complaint is that the name is too generic (which I agree
with).  Given the stated functionality:

 Allow specification of physical standbys that must be synchronized
 before they are visible to subscribers (Hou Zhijie, Shveta Malik)

it seems like the name ought to have some connection to
synchronization.  Perhaps something like "synchronized_standby_slots"?

I haven't read the patch, so I don't know if this name is especially
on-point.  But "standby_slot_names" seems completely unhelpful, as
a server could well have slots that are for standbys but are not to
be included in this list.

regards, tom lane

Re: Small LO_BUFSIZE slows down lo_import and lo_export in libpq

[Tom Lane]

Jelte Fennema-Nio  writes:
> On Fri, 21 Jun 2024 at 10:46, Дмитрий Питаков  wrote:
>> Why not increase the buffer size?

> I think changing the buffer size sounds like a reasonable idea, if
> that speeds stuff up. But I think it would greatly help your case if
> you showed the perf increase using a simple benchmark, especially if
> people could run this benchmark on their own machines to reproduce.

Yeah.  "Why not" is not a patch proposal, mainly because the correct
question is "what other size are you proposing?"

This is not something that we can just randomly whack around, either.
Both lo_import_internal and lo_export assume they can allocate the
buffer on the stack, which means you have to worry about available
stack space.  As a concrete example, I believe that musl still
defaults to 128kB thread stack size, which means that a threaded
client program on that platform would definitely fail with 
LO_BUFSIZE >= 128kB, and even 64kB would be not without risk.

We could dodge that objection by malloc'ing the buffer, which might
be a good thing to do anyway because it'd improve the odds of getting
a nicely-aligned buffer.  But then you have to make the case that the
extra malloc and free isn't a net loss, which it could be for
not-very-large transfers.

So bottom line is that you absolutely need a test case whose
performance can be measured under different conditions.

regards, tom lane

Re: New standby_slot_names GUC in PG 17

[Muhammad Ikram]

Hi,

A humble input, as on primary we have #primary_slot_name = ''  then should
not it be okay to have standby_slot_names or standby_slot_name ? It seems
consistent with the Guc on primary.

Another suggestion is *standby_replication_slots*.

Regards,
Muhammad Ikram
Bitnine Global.

On Fri, Jun 21, 2024 at 8:47 PM Nathan Bossart 
wrote:

> On Fri, Jun 21, 2024 at 11:37:54AM -0400, Bruce Momjian wrote:
> > The release notes have this item:
> >
> >   Allow specification of physical standbys that must be synchronized
> >   before they are visible to subscribers (Hou Zhijie, Shveta Malik)
> >
> >   The new server variable is standby_slot_names.
> >
> > Is standby_slot_names an accurate name for this GUC?  It seems too
> > generic.
>
> +1, I was considering bringing this up, too.  I'm still thinking of
> alternate names to propose, though.
>
> --
> nathan
>
>
>

-- 
Muhammad Ikram

Re: suspicious valgrind reports about radixtree/tidstore on arm64

[Tom Lane]

John Naylor  writes:
> Pushed. I'll clear the open item once all buildfarm members have reported in.

Just to confirm, my raspberry pi 4 got through "make
installcheck-parallel" under valgrind after this commit.

regards, tom lane

Re: Meson far from ready on Windows

[Tristan Partin]

On Fri Jun 21, 2024 at 10:46 AM CDT, Dave Page wrote:

On Fri, 21 Jun 2024 at 16:15, Robert Haas  wrote:
> As a practical matter, I don't think MSVC is coming back. The
> buildfarm was already changed over to use meson, and it would be
> pretty disruptive to try to re-add buildfarm coverage for a
> resurrected MSVC on the eve of beta2. I think we should focus on
> improving whatever isn't quite right in meson -- plenty of other
> people have also complained about various things there, me included --
> rather than trying to undo over a year's worth of work by lots of
> people to get things on Windows switched over to MSVC.
>

The buildfarm hasn't switched over - it had support added for Meson. If it
had been switched, then the older back branches would have gone red.

Anyway, that's immaterial - I know the old code isn't coming back now. My
motivation for this thread is to get Meson to a usable state on Windows,
that doesn't require hacking stuff around for the casual builder moving
forwards - and at present, it requires *significantly* more hacking around
than it has in many years.

The design goals Andres spoke about would clearly be a technical
improvement to PostgreSQL, however as we're finding, they rely on the
upstream dependencies being built with pkgconfig or cmake files which
either doesn't happen at present, or only happens if you happen to build in
a certain way, or download from some source that has added them. I'm not
sure how to fix that without re-introducing the old hacks in the build
system, or extending my side project to add .pc files to all the
dependencies it builds. I will almost certainly do that, as it'll give
folks a single place where they can download everything they need, and
provide a reference on how everything can be built if they want to do it
themselves, but on the other hand, it's far from an ideal solution and I'd
much prefer if I didn't need to do that at all.


Hey Dave,

I'm a maintainer for Meson, and am happy to help you in any way that 
I reasonably can.


Let's start with the state of Windows support in Meson. If I were to 
rank Meson support for platforms, I would do something like this:


- Linux
- BSDs
- Solaris/IllumOS
- ...
- Apple
- Windows

As you can see Windows is the bottom of the totem pole. We don't have 
Windows people coming along to contribute very often for whatever 
reason. Thus admittedly, Windows support can be very lackluster at 
times.


Meson is not a project which sees a lot of funding. (Do any build 
tools?) The projects that support Meson in any way are Mesa and 
GStreamer, which don't have a lot of incentive to do anything with 
Windows, generally.


I'm not even sure any of the regular contributors to Meson have 
Windows development machines. I surely don't have access to a Windows 
machine.


All that being said, I would like to help you solve your Windows 
dependencies issue, or at least mitigate them. I think it is probably 
best to just look at one dependency at a time. Here is how lz4 is picked 
up in the Postgres Meson build:



lz4opt = get_option('lz4')
if not lz4opt.disabled()
  lz4 = dependency('liblz4', required: lz4opt)

  if lz4.found()
cdata.set('USE_LZ4', 1)
cdata.set('HAVE_LIBLZ4', 1)
  endif

else
  lz4 = not_found_dep
endif


As you are well aware, dependency() looks largely at pkgconfig and cmake 
to find the dependencies. In your case, that is obviously not working. 

I think there are two ways to solve your problem. A first solution would 
look like this:



lz4opt = get_option('lz4')
if not lz4opt.disabled()
  lz4 = dependency('liblz4', required: false)
  if not lz4.found()
lz4 = cc.find_library('lz4', required: lz4opt, dirs: extra_lib_dirs)
  endif

  if lz4.found()
cdata.set('USE_LZ4', 1)
cdata.set('HAVE_LIBLZ4', 1)
  end
else
  lz4 = not_found_dep
endif


Another solution that could work alongside the previous suggestion is to 
use Meson subprojects[0] for managing Postgres dependencies. I don't 
know if we would want this in the Postgres repo or a patch that 
downstream packagers would need to apply, but essentially, add the wrap 
file:



[wrap-file]
directory = lz4-1.9.4
source_url = https://github.com/lz4/lz4/archive/v1.9.4.tar.gz
source_filename = lz4-1.9.4.tgz
source_hash = 0b0e3aa07c8c063ddf40b082bdf7e37a1562bda40a0ff5272957f3e987e0e54b
patch_filename = lz4_1.9.4-2_patch.zip
patch_url = https://wrapdb.mesonbuild.com/v2/lz4_1.9.4-2/get_patch
patch_hash = 4f33456cce986167d23faf5d28a128e773746c10789950475d2155a7914630fb
wrapdb_version = 1.9.4-2

[provide]
liblz4 = liblz4_dep


into subprojects/lz4.wrap, and Meson should be able to automagically 
pick up the dependency. Do this for all the projects that Postgres 
depends on, and you'll have an entire build managed by Meson. Note that 
Meson subprojects don't have to use Meson themselves. They can also use 
CMake[1] or Autotools[2], but your results may vary.


Happy to hear your thoughts. I think if our goal is to enable more 
people to work

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

[Alena Rybakina]

I'm confused, I have seen that we have two threads [1] and [2] about 
this thread andIhaven'tfoundanyexplanationfor howtheydiffer.


And I don't understand, whyam Inotlistedas the authorof the patch? 
Iwasdevelopingthe firstpartof the patchbeforeAndreycameto review it[3] 
andhisfirstparthasn'tchangedmuchsincethen.


IfIwroteto the wrongpersonaboutit,thenpleasetellme where.

[1] https://commitfest.postgresql.org/48/4450/

[2] https://commitfest.postgresql.org/48/5037/

[3] 
https://www.postgresql.org/message-id/b301dce1-09fd-72b1-834a-527ca428db5e%40yandex.ru 



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

RE: AIX support

[Srirama Kucherlapati]

We are continuing to work on the changes…

  > Do you still care about 32-bit binaries on AIX? If not, let's make that
  > the default in configure or a check for it, and remove the instructions
  > on building 32-bit binaries from the docs.

As most of the products are moving towards 64bit, we will try to remove
this 32bit support.


Warm regards,
Sriram.

Re: Table AM Interface Enhancements

[Matthias van de Meent]

Hi,

On Tue, 16 Apr 2024 at 12:34, Alexander Korotkov  wrote:
>
> You're right.  No sense trying to fix this.  Reverted.

I just noticed that this revert (commit 6377e12a) seems to have
introduced two comment blocks atop TableAmRoutine's
scan_analyze_next_block, and I can't find a clear reason why these are
two separate comment blocks.
Furthermore, both comment blocks seemingly talk about different
implementations of a block-based analyze functionality, and I don't
have the time to analyze which of these comments is authorative and
which are misplaced or obsolete.


Kind regards,

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

Re: pg_combinebackup --clone doesn't work

[Tomas Vondra]

On 6/21/24 00:07, Tomas Vondra wrote:
> Here's a fix adding the missing headers to pg_combinebackup, and fixing
> some compile-time issues in the ifdef-ed block.
> 
> I've done some basic manual testing today - I plan to test this a bit
> more tomorrow, and I'll also look at integrating this into the existing
> tests.
> 

Here's a bit more complete / cleaned patch, adding the testing changes
in separate parts.

0001 adds the missing headers / fixes the now-accessible code a bit

0002 adds the --copy option for consistency with pg_upgrade

0003 adds the PG_TEST_PG_COMBINEBACKUP_MODE, so that we can override the
copy method for tests

0004 tweaks two of the Cirrus CI tasks to use --clone/--copy-file-range


I believe 0001-0003 are likely non-controversial, although if someone
could take a look at the Perl in 0003 that'd be nice. Also, 0002 seems
nice not only because of consistency with pg_upgrade, but it also makes
0003 easier as we don't need to special-case the default mode etc.

I'm not sure about 0004 - I initially did this mostly to check we have
the right headers on other platforms, but not sure we want to actually
do this. Or maybe we want to test a different combination (e.g. also
test the --clone on Linux)?


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL CompanyFrom 94082624db5d7608f3fb7a5b9322ee90dc5cfcb5 Mon Sep 17 00:00:00 2001
From: Tomas Vondra 
Date: Thu, 20 Jun 2024 23:50:22 +0200
Subject: [PATCH 1/4] Add headers needed by pg_combinebackup --clone

The code for cloning files existed, but was inaccessible as it relied on
constants from missing headers. The consequence is that on Linux --clone
always failed with

error: file cloning not supported on this platform

Fixed by including the missing headers to relevant places. This revealed
a couple compile errors in copy_file_clone(), so fix those too.

Reported-by: Peter Eisentraut
Discussion: https://postgr.es/m/48da4a1f-ccd9-4988-9622-24f37b1de2b4%40eisentraut.org
---
 src/bin/pg_combinebackup/copy_file.c| 12 +++-
 src/bin/pg_combinebackup/pg_combinebackup.c |  8 
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/src/bin/pg_combinebackup/copy_file.c b/src/bin/pg_combinebackup/copy_file.c
index 08c3b875420..8b50e937346 100644
--- a/src/bin/pg_combinebackup/copy_file.c
+++ b/src/bin/pg_combinebackup/copy_file.c
@@ -13,6 +13,10 @@
 #ifdef HAVE_COPYFILE_H
 #include 
 #endif
+#ifdef __linux__
+#include 
+#include 
+#endif
 #include 
 #include 
 #include 
@@ -214,6 +218,9 @@ copy_file_clone(const char *src, const char *dest,
 		pg_fatal("error while cloning file \"%s\" to \"%s\": %m", src, dest);
 #elif defined(__linux__) && defined(FICLONE)
 	{
+		int			src_fd;
+		int			dest_fd;
+
 		if ((src_fd = open(src, O_RDONLY | PG_BINARY, 0)) < 0)
 			pg_fatal("could not open file \"%s\": %m", src);
 
@@ -228,8 +235,11 @@ copy_file_clone(const char *src, const char *dest,
 			unlink(dest);
 
 			pg_fatal("error while cloning file \"%s\" to \"%s\": %s",
-	 src, dest);
+	 src, dest, strerror(save_errno));
 		}
+
+		close(src_fd);
+		close(dest_fd);
 	}
 #else
 	pg_fatal("file cloning not supported on this platform");
diff --git a/src/bin/pg_combinebackup/pg_combinebackup.c b/src/bin/pg_combinebackup/pg_combinebackup.c
index 01d7fb98e79..f67ccf76ea2 100644
--- a/src/bin/pg_combinebackup/pg_combinebackup.c
+++ b/src/bin/pg_combinebackup/pg_combinebackup.c
@@ -16,6 +16,14 @@
 #include 
 #include 
 
+#ifdef HAVE_COPYFILE_H
+#include 
+#endif
+#ifdef __linux__
+#include 
+#include 
+#endif
+
 #include "backup_label.h"
 #include "common/blkreftable.h"
 #include "common/checksum_helper.h"
-- 
2.45.2

From 93ecbe611936279127cebc5ddfefb6ebf1f4730f Mon Sep 17 00:00:00 2001
From: Tomas Vondra 
Date: Fri, 21 Jun 2024 14:36:04 +0200
Subject: [PATCH 2/4] Introduce pg_combinebackup --copy option

The --copy option simply allows picking the default mode to copy data,
and does not change the behavior. This makes pg_combinebackup options
more consistent with pg_upgrade.

Reported-by: Peter Eisentraut
Discussion: https://postgr.es/m/48da4a1f-ccd9-4988-9622-24f37b1de2b4%40eisentraut.org
---
 doc/src/sgml/ref/pg_combinebackup.sgml  | 10 ++
 src/bin/pg_combinebackup/pg_combinebackup.c |  7 ++-
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/doc/src/sgml/ref/pg_combinebackup.sgml b/doc/src/sgml/ref/pg_combinebackup.sgml
index 375307d57bd..091982f62ad 100644
--- a/doc/src/sgml/ref/pg_combinebackup.sgml
+++ b/doc/src/sgml/ref/pg_combinebackup.sgml
@@ -162,6 +162,16 @@ PostgreSQL documentation
   
  
 
+ 
+  --copy
+  
+   
+Perform regular file copy.  This is the default.  (See also
+--copy-file-range and --clone.)
+   
+  
+ 
+
  
   --copy-file-range
   
diff --git a/src/bin/pg_combinebackup/pg_combinebackup.c b/src/bin/pg_combinebackup/pg_combinebackup.c
index f67ccf76ea2..6ddf5b534b1 100644

Re: cfbot update: Using GitHub for patch review

[Pavel Stehule]

pá 21. 6. 2024 v 17:55 odesílatel Nathan Bossart 
napsal:

> On Fri, Jun 21, 2024 at 04:36:13PM +0200, Jelte Fennema-Nio wrote:
> > I recently got write access to the cfbot repo[1] and machine from
> > Thomas. And I deployed a few improvements this week. The most
> > significant one is that it is now much easier to use GitHub as part of
> > your patch review workflow.
>
> Nice!  Thank you.


+1

good work

Pavel

>
>
> --
> nathan
>
>
>

Re: cfbot update: Using GitHub for patch review

[Josef Šimánek]

pá 21. 6. 2024 v 16:36 odesílatel Jelte Fennema-Nio  napsal:
>
> I recently got write access to the cfbot repo[1] and machine from
> Thomas. And I deployed a few improvements this week. The most
> significant one is that it is now much easier to use GitHub as part of
> your patch review workflow.
>
> On the cfbot website[2] there's now a "D" (diff) link next to each
> commit fest entry. A good example of such a link would be the one for
> my most recent commitfest entry[3]. There is a separate commit for
> each patch file and those commits contain the "git format-patch"
> metadata. (this is not done using git am, but using git mailinfo +
> patch + sed, because git am is horrible at resolving conflicts)

This is brilliant!

> The killer feature (imho) of GitHub diffs over looking at patch files:
> You can press the "Expand up"/"Expand down" buttons on the left of the
> diff to see some extra context that the patch file doesn't contain.
>
> You can also add the cfbot repo as a remote to your local git
> repository. That way you don't have to manually download patches and
> apply them to your local checkout anymore:
>
> # Add the remote
> git remote add -f cfbot https://github.com/postgresql-cfbot/postgresql.git
> # make future git pulls much quicker (optional)
> git maintenance start
> # check out a commitfest entry
> git checkout cf/5065
>
> P.S. Suggestions for further improvements are definitely appreciated.
> We're currently already working on better integration between the
> commitfest app website and the cfbot website.
>
> P.P.S The "D" links don't work for patches that need to be rebased
> since before I deployed this change, but that problem should fix
> itself with time.
>
> [1]: https://github.com/macdice/cfbot
> [2]: http://cfbot.cputube.org/
> [3]: 
> https://github.com/postgresql-cfbot/postgresql/compare/cf/5065~1...cf/5065
>
>

Re: cfbot update: Using GitHub for patch review

[Nathan Bossart]

On Fri, Jun 21, 2024 at 04:36:13PM +0200, Jelte Fennema-Nio wrote:
> I recently got write access to the cfbot repo[1] and machine from
> Thomas. And I deployed a few improvements this week. The most
> significant one is that it is now much easier to use GitHub as part of
> your patch review workflow.

Nice!  Thank you.

-- 
nathan

Re: New standby_slot_names GUC in PG 17

[Nathan Bossart]

On Fri, Jun 21, 2024 at 11:37:54AM -0400, Bruce Momjian wrote:
> The release notes have this item:
> 
>   Allow specification of physical standbys that must be synchronized
>   before they are visible to subscribers (Hou Zhijie, Shveta Malik)
> 
>   The new server variable is standby_slot_names. 
> 
> Is standby_slot_names an accurate name for this GUC?  It seems too
> generic.

+1, I was considering bringing this up, too.  I'm still thinking of
alternate names to propose, though.

-- 
nathan

Re: Meson far from ready on Windows

[Dave Page]

On Fri, 21 Jun 2024 at 16:15, Robert Haas  wrote:

> On Fri, Jun 21, 2024 at 7:21 AM Dave Page  wrote:
> > My assumption all along was that Meson would replace autoconf etc.
> before anything happened with MSVC, precisely because that's the type of
> environment all the Postgres devs work in primarily. Instead we seem to
> have taken what I think is a flawed approach of entirely replacing the
> build system on the platform none of the devs use, whilst leaving the new
> system as an experimental option on the platforms it will have had orders
> of magnitude more testing.
> >
> > What makes it worse, is that I don't believe anyone was warned about
> such a drastic change. Packagers were told about the git archive changes to
> the tarball generation, but that's it (and we've said before, we can't
> expect packagers to follow all the activity on -hackers).
>
> I agree that we should have given a heads up to pgsql-packagers. The
> fact that that wasn't done is a mistake, and inconsiderate. At the
> same time, I don't quite know who should have done that exactly when.
> Note that, while I believe Andres is on pgsql-packagers, many
> committers are not, and we have no written guidelines anywhere for
> what kinds of changes require notifying pgsql-packagers.
>
> Previous threads on this issue:
>
> https://postgr.es/m/zqzp_vmjcerm1...@paquier.xyz
> http://postgr.es/m/20230408191007.7lysd42euafwl...@awork3.anarazel.de
>
> Note that in the second of these threads, which contemplated removing
> MSVC for v16, I actually pointed out that if we went that way, we
> needed to notify pgsql-packagers ASAP. But, since we didn't do that,
> no email was ever sent to pgsql-packagers about this, or at least not
> that I can find.


That's what I was saying should have been done. I don't think there was a
requirement on Andres to tell them that they could use Meson instead.


> Still, MSVC support was removed more than six months
> ago, so even if somebody didn't see any of the pgsql-hackers
> discussion about this, there's been a fair amount of time (and a beta)
> for someone to notice that their build process isn't working any more.
> It seems a bit weird to me to start complaining about this now.
>

People noticed when they started prepping for beta1. Then there was a mad
rush to get things working under Meson in any way possible.


> As a practical matter, I don't think MSVC is coming back. The
> buildfarm was already changed over to use meson, and it would be
> pretty disruptive to try to re-add buildfarm coverage for a
> resurrected MSVC on the eve of beta2. I think we should focus on
> improving whatever isn't quite right in meson -- plenty of other
> people have also complained about various things there, me included --
> rather than trying to undo over a year's worth of work by lots of
> people to get things on Windows switched over to MSVC.
>

The buildfarm hasn't switched over - it had support added for Meson. If it
had been switched, then the older back branches would have gone red.

Anyway, that's immaterial - I know the old code isn't coming back now. My
motivation for this thread is to get Meson to a usable state on Windows,
that doesn't require hacking stuff around for the casual builder moving
forwards - and at present, it requires *significantly* more hacking around
than it has in many years.

The design goals Andres spoke about would clearly be a technical
improvement to PostgreSQL, however as we're finding, they rely on the
upstream dependencies being built with pkgconfig or cmake files which
either doesn't happen at present, or only happens if you happen to build in
a certain way, or download from some source that has added them. I'm not
sure how to fix that without re-introducing the old hacks in the build
system, or extending my side project to add .pc files to all the
dependencies it builds. I will almost certainly do that, as it'll give
folks a single place where they can download everything they need, and
provide a reference on how everything can be built if they want to do it
themselves, but on the other hand, it's far from an ideal solution and I'd
much prefer if I didn't need to do that at all.

-- 
Dave Page
pgAdmin: https://www.pgadmin.org
PostgreSQL: https://www.postgresql.org
EDB: https://www.enterprisedb.com

Re: improve predefined roles documentation

[Nathan Bossart]

On Thu, Jun 20, 2024 at 07:57:16PM -0700, David G. Johnston wrote:
> I like this.  Losing the table turned out to be ok.  Thank you.

Awesome.

> I would probably put pg_monitor first in the list.

Done.

> + A user granted this role cannot however send signals to a backend owned
> by a superuser.
> 
> Remove "however", or put commas around it.  I prefer the first option.

This sentence caught my eye earlier, too, because it seems to imply that a
superuser granted this role cannot signal superuser-owned backends.  I
changed it to the following:

Note that this role does not permit signaling backends owned by a
superuser.

How does that sound?

> Do we really need to repeat "even without having it explicitly" everywhere?

Removed.

> + This role does not have the role attribute BYPASSRLS set.
> 
> Even if it did, that attribute isn't inherited anyway...
> 
> "This role is still governed by any row level security policies that may be
> in force.  Consider setting the BYPASSRLS attribute on member roles."
> 
> (assuming they intend it to be ALL data then doing the bypassrls even if
> they are not today using it doesn't hurt)

How does something like the following sound?

This role does not bypass row-level security (RLS) policies.  If RLS is
being used, an administrator may wish to set BYPASSRLS on roles which
this role is granted to.

> pg_stat_scan_tables - This explanation leaves me wanting more.  Maybe give
> an example of such a function?  I think the bar is set a bit too high just
> talking about a specific lock level.

I was surprised to learn that this role only provides privileges for
functions in contrib/ modules.  Anyway, added an example.

> "As these roles are able to access any file on the server file system,"
> 
> We forbid running under root so this isn't really true.  They do have
> operating system level access logged in as the database process owner.
> They are able to access all PostgreSQL files on the server file system and
> usually can run a wide-variety of commands on the server.

I just deleted this clause.

> "access, therefore great care should be taken"
> 
> I would go with:
> 
> "access.  Great care should be taken"
> 
> Seems more impactful as its own sentence then at the end of a long
> multi-part sentence.

Done.

> "server with COPY any other file-access functions." - s/with/using/

Done.

-- 
nathan
>From 70ae4e75e59fb369bb46fccb579b9dedc6c24b11 Mon Sep 17 00:00:00 2001
From: Nathan Bossart 
Date: Tue, 18 Jun 2024 11:38:40 -0500
Subject: [PATCH v2 1/1] revamp predefined roles documentation

---
 doc/src/sgml/config.sgml |   2 +-
 doc/src/sgml/monitoring.sgml |   4 +-
 doc/src/sgml/ref/checkpoint.sgml |   2 +-
 doc/src/sgml/ref/reindex.sgml|   2 +-
 doc/src/sgml/user-manag.sgml | 337 ---
 5 files changed, 183 insertions(+), 164 deletions(-)

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 0c7a9082c5..03e37209e6 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -731,7 +731,7 @@ include_dir 'conf.d'

 Determines the number of connection slots that are
 reserved for connections by roles with privileges of the
-pg_use_reserved_connections
+
 role.  Whenever the number of free connection slots is greater than
  but less than or
 equal to the sum of superuser_reserved_connections
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index b2ad9b446f..f30c1e53fa 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -286,8 +286,8 @@ postgres   27093  0.0  0.0  30096  2752 ?Ss   11:34 
  0:00 postgres: ser
other sessions, many columns will be null.  Note, however, that the
existence of a session and its general properties such as its sessions user
and database are visible to all users.  Superusers and roles with 
privileges of
-   built-in role pg_read_all_stats (see also ) can see all the information about all 
sessions.
+   built-in role pg_read_all_stats
+   can see all the information about all sessions.
   
 
   
diff --git a/doc/src/sgml/ref/checkpoint.sgml b/doc/src/sgml/ref/checkpoint.sgml
index 28a1d717b8..db011a47d0 100644
--- a/doc/src/sgml/ref/checkpoint.sgml
+++ b/doc/src/sgml/ref/checkpoint.sgml
@@ -53,7 +53,7 @@ CHECKPOINT
 
   
Only superusers or users with the privileges of
-   the pg_checkpoint
+   the 
role can call CHECKPOINT.
   
  
diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml
index 2942dccf1e..dcf70d14bc 100644
--- a/doc/src/sgml/ref/reindex.sgml
+++ b/doc/src/sgml/ref/reindex.sgml
@@ -305,7 +305,7 @@ REINDEX [ ( option [, ...] ) ] { DA
partitioned table, such commands skip the privilege checks when processing
the individual partitions.  Reindexing a schema or database requires being 
the
owner of that schema or database or having privileges of the
-   pg_ma

New standby_slot_names GUC in PG 17

[Bruce Momjian]

The release notes have this item:

Allow specification of physical standbys that must be synchronized
before they are visible to subscribers (Hou Zhijie, Shveta Malik)

The new server variable is standby_slot_names. 

Is standby_slot_names an accurate name for this GUC?  It seems too
generic.

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

  Only you can decide what is important to you.

Re: Improve EXPLAIN output for multicolumn B-Tree Index

[Yugo NAGATA]

On Fri, 21 Jun 2024 07:12:25 +
 wrote:

> * Is this feature useful? Is there a possibility it will be accepted?

I think adding such information to EXPLAIN outputs is useful because it
will help users confirm the effect of a multicolumn index on a certain query
and decide to whether leave, drop, or recreate the index, and so on.
 
> * Are there any other ideas for determining if multicolumn indexes are
> 
> being used efficiently? Although I considered calculating the efficiency using
> 
> pg_statio_all_indexes.idx_blks_read and pg_stat_all_indexes.idx_tup_read,
> 
>  I believe improving the EXPLAIN output is better because it can be output
> 
> per query and it's more user-friendly.

It seems for me improving EXPLAIN is a natural way to show information
on query optimization like index scans.
 
> * Is "Index Bound Cond" the proper term?I also considered changing
> 
> "Index Cond" to only show quals for the boundary condition and adding
> 
> a new term "Index Filter".

"Index Bound Cond" seems not intuitive for me because I could not find
description explaining what this means from the documentation. I like
"Index Filter" that implies the index has to be scanned.
 
> * Would it be better to add new interfaces to Index AM? Is there any case
> 
>   to output the EXPLAIN for each index context? At least, I think it's worth
> 
>   considering whether it's good for amcostestimate() to modify the
> 
>   IndexPath directly as the PoC patch does.

I am not sure it is the best way to modify IndexPath in amcostestimate(), but
I don't have better ideas for now.

Regards,
Yugo Nagata

> 
> 
> 
> 
> Regards,
> 
> --
> 
> Masahiro Ikeda
> 
> NTT DATA CORPORATION
> 
> 


-- 
Yugo NAGATA 

Re: Meson far from ready on Windows

[Robert Haas]

On Fri, Jun 21, 2024 at 7:21 AM Dave Page  wrote:
> My assumption all along was that Meson would replace autoconf etc. before 
> anything happened with MSVC, precisely because that's the type of environment 
> all the Postgres devs work in primarily. Instead we seem to have taken what I 
> think is a flawed approach of entirely replacing the build system on the 
> platform none of the devs use, whilst leaving the new system as an 
> experimental option on the platforms it will have had orders of magnitude 
> more testing.
>
> What makes it worse, is that I don't believe anyone was warned about such a 
> drastic change. Packagers were told about the git archive changes to the 
> tarball generation, but that's it (and we've said before, we can't expect 
> packagers to follow all the activity on -hackers).

I agree that we should have given a heads up to pgsql-packagers. The
fact that that wasn't done is a mistake, and inconsiderate. At the
same time, I don't quite know who should have done that exactly when.
Note that, while I believe Andres is on pgsql-packagers, many
committers are not, and we have no written guidelines anywhere for
what kinds of changes require notifying pgsql-packagers.

Previous threads on this issue:

https://postgr.es/m/zqzp_vmjcerm1...@paquier.xyz
http://postgr.es/m/20230408191007.7lysd42euafwl...@awork3.anarazel.de

Note that in the second of these threads, which contemplated removing
MSVC for v16, I actually pointed out that if we went that way, we
needed to notify pgsql-packagers ASAP. But, since we didn't do that,
no email was ever sent to pgsql-packagers about this, or at least not
that I can find. Still, MSVC support was removed more than six months
ago, so even if somebody didn't see any of the pgsql-hackers
discussion about this, there's been a fair amount of time (and a beta)
for someone to notice that their build process isn't working any more.
It seems a bit weird to me to start complaining about this now.

As a practical matter, I don't think MSVC is coming back. The
buildfarm was already changed over to use meson, and it would be
pretty disruptive to try to re-add buildfarm coverage for a
resurrected MSVC on the eve of beta2. I think we should focus on
improving whatever isn't quite right in meson -- plenty of other
people have also complained about various things there, me included --
rather than trying to undo over a year's worth of work by lots of
people to get things on Windows switched over to MSVC.

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

Re: improve ssl error code, 2147483650

[Tom Lane]

Peter Eisentraut  writes:
> -   strlcpy(errbuf, strerror(ERR_GET_REASON(ecode)), SSL_ERR_LEN);
> +   strerror_r(ERR_GET_REASON(ecode), errbuf, SSL_ERR_LEN);

Most of libpq gets at strerror_r via SOCK_STRERROR for Windows
portability.  Is that relevant here?

regards, tom lane

Re: Meson far from ready on Windows

[Dave Page]

Hi

On Fri, 21 Jun 2024 at 12:20, Dave Page  wrote:

>
> We/I *could* add cmake/pc file generation to that tool, which would make
> things work nicely with PostgreSQL 17 of course.
>

For giggles, I took a crack at doing that, manually creating .pc files for
everything I've been working with so far. It seems to work as expected,
except that unlike everything else libintl is detected entirely based on
whether the header and library can be found. I had to pass extra lib and
include dirs:

meson setup --wipe --pkg-config-path=C:\build64\lib\pkgconfig
-Dextra_include_dirs=C:\build64\include -Dextra_lib_dirs=C:\build64\lib
-Duuid=ossp build-auto

I'm assuming that's an oversight, given your previous comments?

-- 
Dave Page
pgAdmin: https://www.pgadmin.org
PostgreSQL: https://www.postgresql.org
EDB: https://www.enterprisedb.com

cfbot update: Using GitHub for patch review

[Jelte Fennema-Nio]

I recently got write access to the cfbot repo[1] and machine from
Thomas. And I deployed a few improvements this week. The most
significant one is that it is now much easier to use GitHub as part of
your patch review workflow.

On the cfbot website[2] there's now a "D" (diff) link next to each
commit fest entry.  A good example of such a link would be the one for
my most recent commitfest entry[3]. There is a separate commit for
each patch file and those commits contain the "git format-patch"
metadata. (this is not done using git am, but using git mailinfo +
patch + sed, because git am is horrible at resolving conflicts)

The killer feature (imho) of GitHub diffs over looking at patch files:
You can press the "Expand up"/"Expand down" buttons on the left of the
diff to see some extra context that the patch file doesn't contain.

You can also add the cfbot repo as a remote to your local git
repository. That way you don't have to manually download patches and
apply them to your local checkout anymore:

# Add the remote
git remote add -f cfbot https://github.com/postgresql-cfbot/postgresql.git
# make future git pulls much quicker (optional)
git maintenance start
# check out a commitfest entry
git checkout cf/5065

P.S. Suggestions for further improvements are definitely appreciated.
We're currently already working on better integration between the
commitfest app website and the cfbot website.

P.P.S The "D" links don't work for patches that need to be rebased
since before I deployed this change, but that problem should fix
itself with time.

[1]: https://github.com/macdice/cfbot
[2]: http://cfbot.cputube.org/
[3]: https://github.com/postgresql-cfbot/postgresql/compare/cf/5065~1...cf/5065

Re: Failures in constraints regression test, "read only 0 of 8192 bytes"

[Heikki Linnakangas]

On 21/06/2024 02:25, Heikki Linnakangas wrote:

Hmm, looking closer, I think this might be a more appropriate place for
the RelationCloseSmgr() call:


/*
 * If it's a mapped relation, immediately update its 
rd_locator in
 * case its relfilenumber changed.  We must do this 
during phase 1
 * in case the relation is consulted during rebuild of 
other
 * relcache entries in phase 2.  It's safe since 
consulting the
 * map doesn't involve any access to relcache entries.
 */
if (RelationIsMapped(relation))
RelationInitPhysicalAddr(relation);


That's where we change the relfilenumber, before the
RelationClearRelation() call.


Pushed a fix that way.

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

Re: pgsql: Add more SQL/JSON constructor functions

[Amit Langote]

Hi,

Thanks for taking a look.

On Fri, Jun 21, 2024 at 4:05 PM jian he  wrote:
> On Tue, Jun 18, 2024 at 5:02 PM Amit Langote  wrote:
> > On Tue, Jun 4, 2024 at 7:03 PM Amit Langote  wrote:
> > > On Tue, Jun 4, 2024 at 2:20 AM Tom Lane  wrote:
> > > > Peter Eisentraut  writes:
> > > > > On 02.06.24 21:46, Tom Lane wrote:
> > > > >> If you don't
> > > > >> like our current behavior, then either you have to say that RETURNING
> > > > >> with a length-limited target type is illegal (which is problematic
> > > > >> for the spec, since they have no such type) or that the cast behaves
> > > > >> like an implicit cast, with errors for overlength input (which I find
> > > > >> to be an unintuitive definition for a construct that names the target
> > > > >> type explicitly).
> > > >
> > > > > It asks for the latter behavior, essentially (but it's not defined in
> > > > > terms of casts).  It says:
> > > >
> > > > Meh.  Who needs consistency?  But I guess the answer is to do what was
> > > > suggested earlier and change the code to use COERCE_IMPLICIT_CAST.
> > >
> > > OK, will post a patch to do so in a new thread on -hackers.
> >
> > Oops, didn't realize that this is already on -hackers.
> >
> > Attached is a patch to use COERCE_IMPLICIT_CAST when the RETURNING
> > type specifies a length limit.
>
> hi.
> i am a little confused.
>
> here[1] tom says:
> > Yeah, I too think this is a cast, and truncation is the spec-defined
> > behavior for casting to varchar with a specific length limit.  I see
> > little reason that this should work differently from
> >
> > select json_serialize('{"a":1, "a":2}' returning text)::varchar(5);
> >  json_serialize
> > 
> >  {"a":
> > (1 row)
>
> if i understand it correctly, and my english interpretation is fine.
> i think tom means something like:
>
> select json_serialize('{"a":1, "a":2}' returning text)::varchar(5) =
> json_serialize('{"a":1, "a":2}' returning varchar(5));
>
> should return true.
> the master will return true, but apply your patch, the above query
> will yield an error.

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.

> --
> current in ExecInitJsonExpr we have
>
> if (jsexpr->coercion_expr)
> ...
> else if (jsexpr->use_json_coercion)
> ...
> else if (jsexpr->use_io_coercion)
> ...
>
> do you think it is necessary to add following asserts:
> Assert (!(jsexpr->coercion_expr &&  jsexpr->use_json_coercion))
> Assert (!(jsexpr->coercion_expr &&  jsexpr->use_io_coercion))

Yeah, perhaps, but let's consider this independently please.

--
Thanks, Amit Langote


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

Re: improve ssl error code, 2147483650

[Daniel Gustafsson]

> On 21 Jun 2024, at 13:15, Peter Eisentraut  wrote:

> I noticed that this change uses not-thread-safe strerror() in libpq code.
> 
> Perhaps something like this would be better (and simpler):

Nice catch, LGTM.

--
Daniel Gustafsson

Re: Avoid orphaned objects dependencies, take 3

[Bertrand Drouvot]

Hi,

On Wed, Jun 19, 2024 at 02:11:50PM +, Bertrand Drouvot wrote:
> To sum up, I did not see any cases that did not lead to 1. or 2., so I think
> it's safe to not add an extra lock for the RelationRelationId case. If, for 
> any
> reason, there is still cases that are outside 1. or 2. then they may lead to
> orphaned dependencies linked to the RelationRelationId class. I think that's
> fine to take that "risk" given that a. that would not be worst than currently
> and b. I did not see any of those in our fleet currently (while I have seen a 
> non
> negligible amount outside of the RelationRelationId case).

Another thought for the RelationRelationId class case: we could check if there
is a lock first and if there is no lock then acquire one. That way that would
ensure the relation is always locked (so no "risk" anymore), but OTOH it may
add "unecessary" locking (see 2. mentioned previously).

I think I do prefer this approach to be on the safe side of thing, what do
you think?

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Re: Improve EXPLAIN output for multicolumn B-Tree Index

[Ashutosh Bapat]

On Fri, Jun 21, 2024 at 12:42 PM  wrote:

> Hi,
>
>
>
> 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.
>
>
>
> As the document says, we need to use it carefully.
>
> > The exact rule is that equality constraints on leading columns,
>
> > plus any inequality constraints on the first column that does
>
> > not have an equality constraint, will be used to limit the portion
>
> > of the index that is scanned.
>
> *https://www.postgresql.org/docs/17/indexes-multicolumn.html
> *
>
>
>
> However, it's not easy to confirm whether multi-column indexes are
>
> being used efficiently because we need to compare the index
>
> definitions and query conditions individually.
>
>
>
> For instance, just by looking at the following EXPLAIN result, we
>
> can't determine whether the index is being used efficiently or not
>
> at a glance. Indeed, the current index definition is not suitable
>
> for the query, so the cost is significantly high.
>
>
>
>   =# EXPLAIN (VERBOSE, ANALYZE) SELECT * FROM test WHERE id1 = 1 AND id3 =
> 101;
>
>QUERY
> PLAN
>
>
> 
>
>Index Scan using test_idx on public.test  (cost=0.42..12754.76 rows=1
> width=18) (actual time=0.033..54.115 rows=1 loops=1)
>
>  Output: id1, id2, id3, value
>
>  Index Cond: ((test.id1 = 1) AND (test.id3 = 101))-- Is it
> efficient or not?
>
>Planning Time: 0.145 ms
>
>Execution Time: 54.150 ms
>
>   (6 rows)
>
>
>
> So, I'd like to improve the output to be more user-friendly.
>
>
>
>
>
> # Idea
>
>
>
> I'm considering adding new information, "Index Bound Cond", which specifies
>
> what quals will be used for the boundary condition of the B-Tree index.
>
> (Since this is just my current idea, I'm open to changing the output.)
>
>
>
> Here is an example output.
>
>
>
> -- prepare for the test
>
> CREATE TABLE test (id1 int, id2 int, id3 int, value varchar(32));
>
> CREATE INDEX test_idx ON test(id1, id2, id3);--
> multicolumn B-Tree index
>
> INSERT INTO test (SELECT i % 2, i, i, 'hello' FROM
> generate_series(1,100) s(i));
>
>  ANALYZE;
>
>
>
> -- explain
>
>
>
> =# EXPLAIN (VERBOSE, ANALYZE) SELECT * FROM test WHERE id1 = 1 AND id2 =
> 101;
>
>QUERY
> PLAN
>
>
>  
> ---
>
>   Index Scan using test_idx on public.test  (cost=0.42..8.45 rows=1
> width=18) (actual time=0.046..0.047 rows=1 loops=1)
>
> Output: id1, id2, id3, value
>
> Index Cond: ((test.id1 = 1) AND (test.id2 = 101))
>
> Index Bound Cond: ((test.id1 = 1) AND (test.id2 = 101))  -- The B-Tree
> index is used efficiently.
>
>   Planning Time: 0.124 ms
>
>   Execution Time: 0.076 ms
>
> (6 rows)
>
>  =# EXPLAIN (VERBOSE, ANALYZE) SELECT * FROM test WHERE id1 = 1 AND id3 =
> 101;
>
>   QUERY
> PLAN
>
>
>  
> 
>
>   Index Scan using test_idx on public.test  (cost=0.42..12754.76 rows=1
> width=18) (actual time=0.033..54.115 rows=1 loops=1)
>
> Output: id1, id2, id3, value
>
> Index Cond: ((test.id1 = 1) AND (test.id3 = 101))
>
> Index Bound Cond: (test.id1 = 1)-- The B-tree
> index is *not* used efficiently
>
>   -- compared to
> the previous execution conditions,
>
>   -- because it
> differs from "Index Cond".
>
>   Planning Time: 0.145 ms
>
>   Execution Time: 54.150 ms
>
> (6 rows)
>
>
>
>
>
> # PoC patch
>
>
>
> The PoC patch makes the following changes:
>
>
>
> * Adds a new variable related to bound conditions
>
>   to IndexPath, IndexScan, IndexOnlyScan, and BitmapIndexScan
>
> * Adds quals for bound conditions to IndexPath when estimating cost, since
>
>   the B-Tree index considers the boundary condition in btcostestimate()
>
> * Adds quals for bound conditions to the output of EXPLAIN
>
>
>
>
>
>
>
> Thank you for reading my suggestion. Please feel free to comment.
>
>
>
> * Is this feature useful? Is there a possibility it will be accepted?
>
> * Are there any other ideas for determining if multicolumn indexes are
>
> being used efficiently? Although I considered calculating the efficiency
> using
>
> pg_statio_all_indexes.idx_blks_read and pg_stat_all_indexes.idx_tup_read,
>
>  I believe improving the EXPLAIN output is better because it can be output
>
> per query and it's more user-friendly.
>
> * Is "Index Boun

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

[Amit Langote]

On Fri, Jun 21, 2024 at 9:47 AM David G. Johnston
 wrote:
> On Thu, Jun 20, 2024 at 9:01 AM jian he  wrote:
>>
>> On Thu, Jun 20, 2024 at 5:46 PM Amit Langote  wrote:
>> >
>> > On Thu, Jun 20, 2024 at 1:03 AM David G. Johnston
>> >  wrote:
>> > > On Wed, Jun 19, 2024 at 8:29 AM jian he  
>> > > wrote:
>> > >>
>> > >> On Mon, Jun 17, 2024 at 9:05 PM Chapman Flack  wrote:
>> > >> >
>> > >> > Hi,
>> > >> >
>> > >> > On 06/17/24 02:43, Amit Langote wrote:
>> > >> > > context_item expression can be a value of
>> > >> > > any type that can be cast to jsonb. This includes types
>> > >> > > such as char,  text, bpchar,
>> > >> > > character varying, and bytea (with
>> > >> > > ENCODING UTF8), as well as any domains over these 
>> > >> > > types.
>> > >> >
>> > >> > Reading this message in conjunction with [0] makes me think that we 
>> > >> > are
>> > >> > really talking about a function that takes a first parameter of type 
>> > >> > jsonb,
>> > >> > and behaves exactly that way (so any cast required is applied by the 
>> > >> > system
>> > >> > ahead of the call). Under those conditions, this seems like an unusual
>> > >> > sentence to add in the docs, at least until we have also documented 
>> > >> > that
>> > >> > tan's argument can be of any type that can be cast to double 
>> > >> > precision.
>> > >> >
>> > >>
>> > >> I guess it would be fine to add an unusual sentence to the docs.
>> > >>
>> > >> imagine a function: array_avg(anyarray) returns anyelement.
>> > >> array_avg calculate an array's elements's avg. like
>> > >> array('{1,2,3}'::int[]) returns 2.
>> > >> but array_avg won't make sense if the input argument is a date array.
>> > >> so mentioning in the doc: array_avg can accept anyarray, but anyarray
>> > >> cannot date array.
>> > >> seems ok.
>> > >
>> > >
>> > > There is existing wording for this:
>> > >
>> > > "The expression can be of any JSON type, any character string type, or 
>> > > bytea in UTF8 encoding."
>> > >
>> > > If you add this sentence to the paragraph the link that already exists, 
>> > > which simply points the reader to this sentence, becomes redundant and 
>> > > should be removed.
>> >
>> > I've just posted a patch in the other thread [1] to restrict
>> > context_item to be of jsonb type, which users would need to ensure by
>> > adding an explicit cast if needed.  I think that makes this
>> > clarification unnecessary.
>> >
>> > > 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)
> 
> 
>  Returns true if the SQL/JSON 
> path_expression
> @@ -18732,12 +18734,14 @@ ERROR:  jsonpath array subscript is out of bounds
>
>  json_query
>  json_query (
> -context_item, 
> path_expression  
> PASSING { value 
> AS varname } , 
> ...
> - RETURNING 
> data_type  FORMAT 
> JSON  ENCODING UTF8  
>  
> - { WITHOUT | WITH { 
> CONDITIONAL | 
> UNCONDITIONAL } }  
> ARRAY  WRAPPER 
> - { KEEP | OMIT } 
> QUOTES  ON SCALAR STRING 
>  
> - { ERROR | NULL | 
> EMPTY {  ARRAY  | 
> OBJECT } | DEFAULT 
> expression } ON EMPTY 
> 
> - { ERROR | NULL | 
> EMPTY {  ARRAY  | 
> OBJECT } | DEFAULT 
> expression } ON ERROR 
> )
> +context_item,
> +path_expression
> +variable_definitions
> +return_clause
> +wrapping_clause
> +quoting_clause
> +on_empty_set
> +on_error_set)
>
> 
>  Returns the result of applying the SQL/JSON
> @@ -18809,11 +18813,12 @@ DETAIL:  Missing "]" after array dimensions.
>
>  json_value
>  json_value (
> -context_item, 
> path_expression
> - PASSING { 
> value AS 
> varname } , ...
> - RETURNING 
> data_type 
> - { ERROR | NULL | 
> DEFAULT expression } 
> ON EMPTY 
> - { ERR

Re: Small LO_BUFSIZE slows down lo_import and lo_export in libpq

[Jelte Fennema-Nio]

On Fri, 21 Jun 2024 at 10:46, Дмитрий Питаков  wrote:
> Why not increase the buffer size?

I think changing the buffer size sounds like a reasonable idea, if
that speeds stuff up. But I think it would greatly help your case if
you showed the perf increase using a simple benchmark, especially if
people could run this benchmark on their own machines to reproduce.

Re: Custom TupleTableSlotOps while Initializing Custom Scan

[Ashutosh Bapat]

Hi,

On Thu, Jun 20, 2024 at 5:58 PM V N G Samba Siva Reddy Chinta <
sambasivareddychi...@gmail.com> wrote:

> Hello Team,
> Good Day,
>
> I have been working on adding a CustomScanState object in the executor
> state in my project. As part of CustomScanState, I execute queries and
> store their results in the Tuplestorestate object. After storing all
> tuples in the Tuplestorestate, I retrieve each tuple and place it in the
> TupleTableSlot using the tuplestore_gettupleslot() function.
>
> However, I encounter an error: *"trying to store a minimal tuple into the
> wrong type of slot."* Upon debugging, I discovered that the TupleTableSlot
> only holds virtual tuples (tupleTableSlot->tts_ops is set to TTSOpsVirtual).
> In contrast, tuplestore_gettupleslot() calls ExecStoreMinimalTuple(),
> which expects TupleTableSlotOps of type TTSOpsMinimalTuple.
>
> Further investigation revealed that in the ExecInitCustomScan() function
> within the nodeCustom.c source file, where ScanTupleSlot and
> ResultTupleSlots are initialized, users can choose custom slots by
> setting slotOps in CustomScanState. We initialize the ScanTupleSlot based
> on user-specified slotOps, but for ResultTupleSlot, we proceed with
> TTSOpsVirtual instead of the custom slotOps, which is causing the issue.
>
> Is this behavior expected? Is there a way to store tuples in slots
> according to the TupleTableSlot type?
>
> I found a function ExecForceStoreMinimalTuple() which can be used in my
> case. We need to pass the MinimalTuple to this function, but I was unable
> to find a way to fetch the tuple from tuple storestate. We do have 
> tuplestore_gettuple()
> function to get the minimal tuple but it is a static function, is there any
> other function like that?
>

>From the description I assume that you are passing ResultTupleSlot to
tuplestore_gettupleslot(). Please confirm.

I think the reason for CustomScan's ResultTupleSlot to be set to virtual
tuple slot is it's the most general kind of tuple - values and isnulls.
Other kinds depend upon the heap tuple format. And the rest of the code
needs CustomScans to produce a deterministic tuple slot type.

Without knowing the details of your code, I think what you need to do is
declare a minimal tuple slot, fetch the tuple from store using this slot
and then store it into ResultTupleSlot using its ttsops. Other types of
planstate nodes some times return the slot they get from their subplans as
is without going through ResultTupleSlot. You may want to do something like
that, if the result tuple contents are same as the tuple stored in
tuplestore

-- 
Best Wishes,
Ashutosh Bapat

pg_createsubscriber: drop pre-existing subscriptions from the converted node

[Hayato Kuroda (Fujitsu)]

Dear Hackers,

This is a follow-up thread for pg_createsubscriber [1]. I started a new thread
since there is no activity around here.

## Problem

Assuming that there is a cascading replication like below:

node A --(logical replication)--> node B --(streaming replication)--> node C

In this case, subscriptions exist even on node C, but it does not try to connect
to node A because the logical replication launcher/worker won't be launched.
After the conversion, node C becomes a subscriber for node B, and the 
subscription
toward node A remains. Therefore, another worker that tries to connect to node A
will be launched, raising an ERROR [2]. This failure may occur even during the
conversion.

## Solution

The easiest solution is to drop pre-existing subscriptions from the converted 
node.
To avoid establishing connections during the conversion, slot_name is set to 
NONE
on the primary first, then drop on the standby. The setting will be restored on 
the
primary node.
Attached patch implements the idea. Test script is also included, but not sure 
it should
be on the HEAD

BTW, I found that LogicalRepInfo.oid won't be used. If needed, I can create
another patch to remove the attribute.

How do you think?

[1]: 
https://www.postgresql.org/message-id/CAA4eK1J22UEfrqx222h5j9DQ7nxGrTbAa_BC%2B%3DmQXdXs-RCsew%40mail.gmail.com
[2]: 
https://www.postgresql.org/message-id/CANhcyEWvimA1-f6hSrA%3D9qkfR5SonFb56b36M%2B%2BvT%3DLiFj%3D76g%40mail.gmail.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/ 



0001-pg_createsubscriber-Drop-pre-existing-subscriptions-.patch
Description:  0001-pg_createsubscriber-Drop-pre-existing-subscriptions-.patch

Re: Meson far from ready on Windows

[Dave Page]

Hi

On Thu, 20 Jun 2024 at 21:58, Andres Freund  wrote:

>
> > I don't think requiring or expecting vcpkg or conan is reasonable at all,
> > for a number of reasons:
> >
> > - Neither supports all the dependencies at present.
> > - There are real supply chain verification concerns for vendors.
> > - That would be a huge change from what we've required in the last 19
> > years, with no deprecation notices or warnings for packagers etc.
>
> I don't think we should hard-require one specifically. I do think it'd be
> good
> if we provided an easy recipe for dependencies to be installed though.
>

That is precisely what https://github.com/dpage/winpgbuild/ was intended
for - and it works well for PG <= 16.


> I think such flexibility acually means it becomes *more* important to
> abstract
> away some of the concrete ways of using the various dependencies. It
> doesn't
> make sense for postgres to understand the internals of each dependency on
> all
> platforms to a detail that it can cope with all the different ways of
> linking
> against them.
>
> E.g. libxml can be built with icu, lzma, zlib support. If libxml is built
> statically, postgres needs to link to all those libraries as well.  How
> can we
> know which of those dependencies are enabled?
>

I don't think it's unreasonable to not support static linking, but I take
your point.


> Even if we can make that somehow work, it's not reasonable for postgres
> developers adding a dependency to have to figure out how to probe all of
> this,
> when literally no other platform works that way anymore.
>
> If you look around at recipes for building postgres on windows, they all
> have
> to patch src/tools/msvc (see links at the bottom), because the builtin
> paths
> and flags just don't work outside of a single way of acquiring the
> dependency.
>

I've been responsible for the Windows installers at EDB since we started
work on them, and prior to that built the original ones with Magnus. Since
v8.0, I've implemented multiple frameworks for building those packages, and
for building PostgreSQL as a dependency of other things (e.g. pgAdmin).
I've done that using builds of dependencies found at random places on the
internet, building them all myself, and a mixture.

I have never once had to patch the MSVC build system. The most I've ever
had to do is copy/rename a .lib file - zlib, which for some reason uses
different naming depending on how you build it. I vaguely recall that
OpenSSL had a similar issue in the distant past.

My point is that we seem to be heading from minor hacks to get things
working in some corner cases, towards requiring packagers and other people
building PostgreSQL on Windows having to do significant work to make the
dependencies look as we now expect. I suspect even more people will end up
patching the Meson build system as it might actually be easier to get
things to work.

The fact that this thread started only now is actually a good example for
> how
> broken the approach to internalize all knowledge about building against
> libraries into postgres is. This could all have been figured out 1+ years
> ago
> - but wasn't.
>
> Unless you want to require postgres devs to get constantly in the muck on
> windows, we'll never get that right until just before the release.
>


Right now I'd be happy to just have the old MSVC build system back until we
can figure out a less complicated way to get to Meson (which I fully
support).

My assumption all along was that Meson would replace autoconf etc. before
anything happened with MSVC, precisely because that's the type of
environment all the Postgres devs work in primarily. Instead we seem to
have taken what I think is a flawed approach of entirely replacing the
build system on the platform none of the devs use, whilst leaving the new
system as an experimental option on the platforms it will have had orders
of magnitude more testing.

What makes it worse, is that I don't believe anyone was warned about such a
drastic change. Packagers were told about the git archive changes to the
tarball generation, but that's it (and we've said before, we can't expect
packagers to follow all the activity on -hackers).



> I don't particularly care how we abstract away the low level linking
> details
> on windows. We can use pkgconf, we can use cmake, we can invent our own
> thing.
> But it has to be something other than hardcoding windows library paths and
> compiler flags into our buildsystem.
>
>
> And yes, that might make it a bit harder for a packager on windows, but
> windows is already a *massive* drag on PG developers, it has to be somewhat
> manageable.
>
> I do think we can make the effort of windows dependency management a lot
> more
> reasonable than it is now though, by providing a recipe for acquiring the
> dependency in some form. It's a lot easier to for packagers and developers
> to
> customize ontop of something like that.
>

Well as I noted, that is the point of my Github repo above. You can just go

Re: improve ssl error code, 2147483650

[Peter Eisentraut]

On 08.03.24 01:46, Tom Lane wrote:

Daniel Gustafsson  writes:

On 7 Mar 2024, at 20:58, Tom Lane  wrote:

This could probably do with a comment, and we need to propagate
the fix into libpq's copy of the function too.  Barring objections,
I'll take care of that and push it later today.



LGTM.


Done so far as be-secure-openssl.c and fe-secure-openssl.c are
concerned.


I noticed that this change uses not-thread-safe strerror() in libpq code.

Perhaps something like this would be better (and simpler):

diff --git a/src/interfaces/libpq/fe-secure-openssl.c 
b/src/interfaces/libpq/fe-secure-openssl.c

index 5c867106fb0..14cd9ce404d 100644
--- a/src/interfaces/libpq/fe-secure-openssl.c
+++ b/src/interfaces/libpq/fe-secure-openssl.c
@@ -1767,7 +1767,7 @@ SSLerrmessage(unsigned long ecode)
 #ifdef ERR_SYSTEM_ERROR
if (ERR_SYSTEM_ERROR(ecode))
{
-   strlcpy(errbuf, strerror(ERR_GET_REASON(ecode)), SSL_ERR_LEN);
+   strerror_r(ERR_GET_REASON(ecode), errbuf, SSL_ERR_LEN);
return errbuf;
}
 #endif

Re: suspicious valgrind reports about radixtree/tidstore on arm64

[John Naylor]

On Thu, Jun 20, 2024 at 2:58 PM John Naylor  wrote:
> the struct. If that's agreeable I'll commit it that way tomorrow
> unless someone beats me to it.

Pushed. I'll clear the open item once all buildfarm members have reported in.

Re: ON ERROR in json_query and the like

[Markus Winand]

> On 21.06.2024, at 07:38, David G. Johnston  wrote:
> 
> On Thursday, June 20, 2024, Markus Winand  wrote:
> 
> 
> > On 21.06.2024, at 06:46, David G. Johnston  
> > wrote:
> >> 
> 
> > 
> > 2 also has the benefit of being standard conforming while 1 does not.
> 
> Why do you think so? Do you have any references or is this just based on 
> previous statements in this discussion?
> 
> 
> Hearsay.
> 
>  
> https://www.postgresql.org/message-id/CAFj8pRCnzO2cnHi5ebXciV%3DtuGVvAQOW9uPU%2BDQV1GkL31R%3D-g%40mail.gmail.com
> 
> > 4) If ALREADY PARSED is False, then it is implementation-defined whether the
> > following rules are applied:
> > a) The General Rules of Subclause 9.36, "Parsing JSON text", are applied 
> > with
> > JT as JSON TEXT, an implementation-defined 
> > as UNIQUENESS CONSTRAINT, and FO as FORMAT OPTION; let ST be the STATUS and
> > let CISJI be the SQL/JSON ITEM returned from the application of those
> > General Rules.
> > b) If ST is not successful completion, then ST is returned as the STATUS of
> > this application of these General Rules, and no further General Rules of
> > this Subclause are applied.
> 
> But maybe I’m mis-interpreting that snippet and Nikita’s related commentary 
> regarding have chosen between options for this implementation-defined feature.

Ah, here we go. Nowadays this is called IA050, “Whether a JSON context item 
that is not of the JSON data type is parsed.” (Likewise IA054 “Whether a JSON 
parameter is parsed.”)

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.

-markus
ps: Does anyone know a dialect that implements GR4?

Re: Issues with ON CONFLICT UPDATE and REINDEX CONCURRENTLY

[Michail Nikolaev]

Hello, Michael!

> This is a non-fresh Friday-afternoon idea, but it would make sure that
> we don't have any transactions using the indexes switched to _ccold
> with indisvalid that are waiting for a drop in phase 5.  Your tests
> seem to pass with that, and that keeps the operation intact
> concurrent-wise (I'm really wishing for isolation tests with injection
> points just now, because I could use them here).

Yes, I also have tried that approach, but it doesn't work, unfortunately.
You may fail test increasing number of connections:

'--no-vacuum --client=10 -j 2 --transactions=1000',

The source of the issue is not the swap of the indexes (and not related to
REINDEX CONCURRENTLY only), but the fact that indexes are fetched once
during planning (to find the arbiter), but then later reread with a new
catalog snapshot for the the actual execution.

So, other possible fixes I see:
* fallback to replanning in case we see something changed during the
execution
* select arbiter indexes during actual execution

> That's a HEAD-only thing IMO,
> though.
Do you mean that it needs to be moved to a separate patch?

Best regards,
Mikhail.

libpq: Fix lots of discrepancies in PQtrace

[Jelte Fennema-Nio]

After initially trying to add trace support for
StartupMessage/SSLRequest/GSSENCRequest[1] I realized there were many
more cases where PQtrace was not correctly tracing messages, or not
even tracing them at all. These patches fix all of the issues that I
was able to find.

0001 is some cleanup after f4b54e1ed9
0002 does some preparatory changes for 0004 & 0007

All the others improve the tracing, and apart from 0004 and 0007
depending on 0002, none of these patches depend on each other.
Although you could argue that 0007 and 0008 depend on 0006, because
without 0006 the code added by 0007 and 0008 won't ever really be
executed.

To test you can add a PQreset(conn) call to the start of the
test_cancel function in:
src/test/modules/libpq_pipeline/libpq_pipeline.c.

And then run:
ninja -C build all install-quiet &&
build/src/test/modules/libpq_pipeline/libpq_pipeline cancel
'port=5432' -t test.trace

And then look at the top of test.trace

[1]: 
https://www.postgresql.org/message-id/CAGECzQTTN5aGqtDaRifJXPyd_O5qHWQcOxsHJsDSVNqMugGNEA%40mail.gmail.com
From 8567a09b4e1816ccdd58786f912ee055accfbc93 Mon Sep 17 00:00:00 2001
From: Jelte Fennema-Nio 
Date: Fri, 21 Jun 2024 01:05:24 +0200
Subject: [PATCH v1 1/8] libpq: Use PqMsg macros in fe-auth.c

In f4b54e1ed9 macros were introduced for protocol characters to improve
readability of code. But these new macros were not used everywhere in
fe-auth.c by accident. This fixes the final three places.
---
 src/interfaces/libpq/fe-auth.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c
index 256f596e6bb..b5b367f24d0 100644
--- a/src/interfaces/libpq/fe-auth.c
+++ b/src/interfaces/libpq/fe-auth.c
@@ -124,7 +124,7 @@ pg_GSS_continue(PGconn *conn, int payloadlen)
 		 * first or subsequent packet, just send the same kind of password
 		 * packet.
 		 */
-		if (pqPacketSend(conn, 'p',
+		if (pqPacketSend(conn, PqMsg_GSSResponse,
 		 goutbuf.value, goutbuf.length) != STATUS_OK)
 		{
 			gss_release_buffer(&lmin_s, &goutbuf);
@@ -324,7 +324,7 @@ pg_SSPI_continue(PGconn *conn, int payloadlen)
 		 */
 		if (outbuf.pBuffers[0].cbBuffer > 0)
 		{
-			if (pqPacketSend(conn, 'p',
+			if (pqPacketSend(conn, PqMsg_GSSResponse,
 			 outbuf.pBuffers[0].pvBuffer, outbuf.pBuffers[0].cbBuffer))
 			{
 FreeContextBuffer(outbuf.pBuffers[0].pvBuffer);
@@ -683,7 +683,7 @@ pg_SASL_continue(PGconn *conn, int payloadlen, bool final)
 		/*
 		 * Send the SASL response to the server.
 		 */
-		res = pqPacketSend(conn, 'p', output, outputlen);
+		res = pqPacketSend(conn, PqMsg_SASLResponse, output, outputlen);
 		free(output);
 
 		if (res != STATUS_OK)
@@ -754,7 +754,7 @@ pg_password_sendauth(PGconn *conn, const char *password, AuthRequest areq)
 		default:
 			return STATUS_ERROR;
 	}
-	ret = pqPacketSend(conn, 'p', pwd_to_send, strlen(pwd_to_send) + 1);
+	ret = pqPacketSend(conn, PqMsg_PasswordMessage, pwd_to_send, strlen(pwd_to_send) + 1);
 	free(crypt_pwd);
 	return ret;
 }

base-commit: c5c82123d3050c3a5eef0f51e9783f1cc5004ba0
-- 
2.34.1

From 1187d694bbff1e1c13041bb361db906a0cb329f1 Mon Sep 17 00:00:00 2001
From: Jelte Fennema-Nio 
Date: Fri, 21 Jun 2024 10:45:38 +0200
Subject: [PATCH v1 2/8] libpq: Add suppress argument to pqTraceOutputNchar

In future commits we're going to trace authentication related messages.
Some of these messages contain challenge bytes as part of a
challenge-responese flow. Since these bytes are different for every
connection we want normalized them when the PQTRACE_REGRESS_MODE trace
flag is set. This commit modifies pqTraceOutputNchar to take a suppress
argument, which makes it possible to do so.
---
 src/interfaces/libpq/fe-trace.c | 17 -
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/src/interfaces/libpq/fe-trace.c b/src/interfaces/libpq/fe-trace.c
index d7a61ec9cc1..6ea7bb821a1 100644
--- a/src/interfaces/libpq/fe-trace.c
+++ b/src/interfaces/libpq/fe-trace.c
@@ -185,12 +185,19 @@ pqTraceOutputString(FILE *pfdebug, const char *data, int *cursor, bool suppress)
  * pqTraceOutputNchar: output a string of exactly len bytes message to the log
  */
 static void
-pqTraceOutputNchar(FILE *pfdebug, int len, const char *data, int *cursor)
+pqTraceOutputNchar(FILE *pfdebug, int len, const char *data, int *cursor, bool suppress)
 {
 	int			i,
 next;			/* first char not yet printed */
 	const char *v = data + *cursor;
 
+	if (suppress)
+	{
+		fprintf(pfdebug, " ''");
+		*cursor += len;
+		return;
+	}
+
 	fprintf(pfdebug, " \'");
 
 	for (next = i = 0; i < len; ++i)
@@ -246,7 +253,7 @@ pqTraceOutput_Bind(FILE *f, const char *message, int *cursor)
 		nbytes = pqTraceOutputInt32(f, message, cursor, false);
 		if (nbytes == -1)
 			continue;
-		pqTraceOutputNchar(f, nbytes, message, cursor);
+		pqTraceOutputNchar(f, nbytes, message, cursor, false);
 	}
 
 	nparams = pqTraceOutputInt16(f, message, cursor);
@@ -283,7 +290,7 @@ pqTraceOutput_D

Re: minor doc issue in 9.16.2.1.1. Boolean Predicate Check Expressions

[jian he]

On Fri, Jun 21, 2024 at 11:11 AM David G. Johnston
 wrote:
>
> On Thu, Jun 20, 2024 at 7:30 PM jian he  wrote:
>>
>> "predicate check expressions return the single three-valued result of
>>
>> the predicate: true, false, or unknown."
>> "unknown" is wrong, because `select 'unknown'::jsonb;` will fail.
>> here "unknown" should be "null"? see jsonb_path_query doc entry also.
>>
>
> The syntax for json_exists belies this claim (assuming our docs are accurate 
> there).  Its "on error" options are true/false/unknown.  Additionally, the 
> predicate test operator is named "is unknown" not "is null".
>
> The result of the predicate test, which is never produced as a value, only a 
> concept, is indeed "unknown" - which then devolves to false when it is 
> practically applied to determining whether to output the path item being 
> tested.  As it does also when used in a parth expression.
>

in [1] says
The similar predicate check expression simply returns true, indicating
that a match exists:

=> select jsonb_path_query(:'json', '$.track.segments[*].HR > 130');
 jsonb_path_query
--
 true



but in this example
select jsonb_path_query('1', '$ == "1"');
return null.

I guess here, the match evaluation cannot be applied, thus returning null.


So summary:
if the boolean predicate check expressions are applicable, return true or false.

the boolean predicate check expressions are not applicable, return null.
example: select jsonb_path_query('1', '$ == "a"');


but I found following two examples returning different results,
i think they should return the same value.
select json_value('1', '$ == "1"' returning jsonb error on error);
select json_query('1', '$ == "1"' returning jsonb error on error);

[1] 
https://www.postgresql.org/docs/devel/functions-json.html#FUNCTIONS-SQLJSON-CHECK-EXPRESSIONS

Small LO_BUFSIZE slows down lo_import and lo_export in libpq

[Дмитрий Питаков]

In libpq, in the code of the lo_import function, for each piece of a file
of 8KB in size, lo_write is called, which greatly slows down the work of
lo_import because lo_write sends a request and waits for a response.  The
size of 8KB is specified in define LO_BUFSIZE which changed from 1KB to 8KB
24 years ago.
Why not increase the buffer size?

Re: Allow non-superuser to cancel superuser tasks.

[Andrey M. Borodin]

> On 21 Jun 2024, at 10:36, Michael Paquier  wrote:
> 
> On Fri, Jun 14, 2024 at 03:12:50PM -0500, Nathan Bossart wrote:
>> On Fri, Jun 14, 2024 at 12:06:36PM +0500, Andrey M. Borodin wrote:
>>> This patch looks good to me.
>> 
>> Thanks for looking.
> 
> While double-checking the whole, where I don't have much to say about
> 0001, I have fixed a few issues with the test presented upthread and
> stabilized it (CI and my stuff are both OK).  I'd suggest to move it
> to test_misc/, because there is no clear category where to put it, and
> we have another test with injection points there for timeouts so the
> module dependency with EXTRA_INSTALL is already cleared.
> 
> What do you think?

Thanks Michael!

All changes look good to me.

I just have one more concern: we do not wakeup() upon test end. I observed that 
there might happen more autovacuums and start sleeping in injection point. In 
every case I observed - these autovacuums quit gracefully. But is it guaranteed 
that test will shut down node even if some of backends are waiting in injection 
points?
Or, perhaps, should we always wakeup() after detaching? (in case when new point 
run might happen)


Best regards, Andrey Borodin.

Re: call for applications: mentoring program for code contributors

[Aleksander Alekseev]

Hi,

> > I'm working to start a mentoring program where code contributors can
> > be mentored by current committers. Applications are now open:
> > https://forms.gle/dgjmdxtHYXCSg6aB7
>
> This is amazing! Thank you for putting it together, Robert.

Great initiative! Thanks Rovert and to everyone involved.

-- 
Best regards,
Aleksander Alekseev

Re: Pgoutput not capturing the generated columns

[Peter Smith]

Hi Shubham/Shlok.

FYI, there is some other documentation page that mentions generated
column replication messages.

This page [1] says:
"Next, the following message part appears for each column included in
the publication (except generated columns):"

But, with the introduction of your new feature, I think that the
"except generated columns" wording is not strictly correct anymore.
IOW that docs page needs updating but AFAICT your patches have not
addressed this yet.

==
[1] https://www.postgresql.org/docs/17/protocol-logicalrep-message-formats.html

Kind Regards,
Peter Smith.
Fujitsu Australia

Re: Pgoutput not capturing the generated columns

[Peter Smith]

Hi, Here are some review comments for patch v9-0003

==
Commit Message

/fix/fixes/

==
1.
General. Is tablesync enough?

I don't understand why is the patch only concerned about tablesync?
Does it make sense to prevent VIRTUAL column replication during
tablesync, if you aren't also going to prevent VIRTUAL columns from
normal logical replication (e.g. when copy_data = false)? Or is this
already handled somewhere?

~~~

2.
General. Missing test.

Add some test cases to verify behaviour is different for STORED versus
VIRTUAL generated columns

==
src/sgml/ref/create_subscription.sgml

NITPICK - consider rearranging as shown in my nitpicks diff
NITPICK - use  sgml markup for STORED

==
src/backend/replication/logical/tablesync.c

3.
- if ((walrcv_server_version(LogRepWorkerWalRcvConn) >= 12 &&
- walrcv_server_version(LogRepWorkerWalRcvConn) <= 16) ||
- !MySubscription->includegencols)
+ if (walrcv_server_version(LogRepWorkerWalRcvConn) < 17)
+ {
+ if (walrcv_server_version(LogRepWorkerWalRcvConn) >= 12)
  appendStringInfo(&cmd, " AND a.attgenerated = ''");
+ }
+ else if (walrcv_server_version(LogRepWorkerWalRcvConn) >= 17)
+ {
+ if(MySubscription->includegencols)
+ appendStringInfo(&cmd, " AND a.attgenerated != 'v'");
+ else
+ appendStringInfo(&cmd, " AND a.attgenerated = ''");
+ }

IMO this logic is too tricky to remain uncommented -- please add some comments.
Also, it seems somewhat complex. I think you can achieve the same more simply:

SUGGESTION

if (walrcv_server_version(LogRepWorkerWalRcvConn) >= 12)
{
  bool gencols_allowed = walrcv_server_version(LogRepWorkerWalRcvConn) >= 17
&& MySubscription->includegencols;
  if (gencols_allowed)
  {
/* Replication of generated cols is supported, but not VIRTUAL cols. */
appendStringInfo(&cmd, " AND a.attgenerated != 'v'");
  }
  else
  {
/* Replication of generated cols is not supported. */
appendStringInfo(&cmd, " AND a.attgenerated = ''");
  }
}

==

99.
Please refer also to my attached nitpick diffs and apply those if you agree.

==
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/doc/src/sgml/ref/create_subscription.sgml 
b/doc/src/sgml/ref/create_subscription.sgml
index 5666931..4ce89e9 100644
--- a/doc/src/sgml/ref/create_subscription.sgml
+++ b/doc/src/sgml/ref/create_subscription.sgml
@@ -433,16 +433,15 @@ CREATE SUBSCRIPTION subscription_nameinclude_generated_columns 
(boolean)
 
  
-  Specifies whether the generated columns present in the tables
-  associated with the subscription should be replicated.
+  Specifies whether the STORED generated columns 
present in
+  the tables associated with the subscription should be replicated.
   The default is false.
  
 
  
   If the subscriber-side column is also a generated column then this 
option
   has no effect; the subscriber column will be filled as normal with 
the
-  subscriber-side computed or default data. This option allows 
replication
-  of only STORED GENERATED COLUMNS.
+  subscriber-side computed or default data.
  
 

Re: pgsql: Add more SQL/JSON constructor functions

[jian he]

On Tue, Jun 18, 2024 at 5:02 PM Amit Langote  wrote:
>
> On Tue, Jun 4, 2024 at 7:03 PM Amit Langote  wrote:
> > On Tue, Jun 4, 2024 at 2:20 AM Tom Lane  wrote:
> > > Peter Eisentraut  writes:
> > > > On 02.06.24 21:46, Tom Lane wrote:
> > > >> If you don't
> > > >> like our current behavior, then either you have to say that RETURNING
> > > >> with a length-limited target type is illegal (which is problematic
> > > >> for the spec, since they have no such type) or that the cast behaves
> > > >> like an implicit cast, with errors for overlength input (which I find
> > > >> to be an unintuitive definition for a construct that names the target
> > > >> type explicitly).
> > >
> > > > It asks for the latter behavior, essentially (but it's not defined in
> > > > terms of casts).  It says:
> > >
> > > Meh.  Who needs consistency?  But I guess the answer is to do what was
> > > suggested earlier and change the code to use COERCE_IMPLICIT_CAST.
> >
> > OK, will post a patch to do so in a new thread on -hackers.
>
> Oops, didn't realize that this is already on -hackers.
>
> Attached is a patch to use COERCE_IMPLICIT_CAST when the RETURNING
> type specifies a length limit.
>

hi.
i am a little confused.

here[1] tom says:
> Yeah, I too think this is a cast, and truncation is the spec-defined
> behavior for casting to varchar with a specific length limit.  I see
> little reason that this should work differently from
>
> select json_serialize('{"a":1, "a":2}' returning text)::varchar(5);
>  json_serialize
> 
>  {"a":
> (1 row)

if i understand it correctly, and my english interpretation is fine.
i think tom means something like:

select json_serialize('{"a":1, "a":2}' returning text)::varchar(5) =
json_serialize('{"a":1, "a":2}' returning varchar(5));

should return true.
the master will return true, but apply your patch, the above query
will yield an error.


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.


--
current in ExecInitJsonExpr we have

if (jsexpr->coercion_expr)
...
else if (jsexpr->use_json_coercion)
...
else if (jsexpr->use_io_coercion)
...

do you think it is necessary to add following asserts:
Assert (!(jsexpr->coercion_expr &&  jsexpr->use_json_coercion))
Assert (!(jsexpr->coercion_expr &&  jsexpr->use_io_coercion))



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