Re: postgres_fdw: Provide better emulation of READ COMMITTED behavior

[Andy Fan]

Robert Haas  writes:

> On Thu, Dec 5, 2024 at 4:41 AM Etsuro Fujita  wrote:
>> Comments welcome!  Maybe I am missing something, though.
>
> I have a hard time seeing how this would work if cursors are in use on
> the main server. Say I do this:
>
> DECLARE foo CURSOR FOR SELECT * FROM ft1 UNION ALL SELECT * FROM ft2;
> ...fetch some rows from cursor foo but few enough that we only scan ft1...
> ...do something that causes a snapshot refresh like issue another query...
> ...fetch more rows from cursor foo until we start scanning ft2...

Apart from the above issue, what do you think about that we are using a
'SELECT pg_catalog.pg_refresh_snapshot()' to let the remote do the
refresh_snapshot VS 'a new message type for this'?  There are lots of
things happen in the 'SELECT' way like 'a extra network communication',
'a complete parser-planner-executor workflow.' With a new message type
for this, we can send the message character with the next query
together. if so, can the two overheads removed? 


-- 
Best Regards
Andy Fan

Re: 64 bit numbers vs format strings

[Alvaro Herrera]

On 2024-Dec-06, Thomas Munro wrote:

> And no doubt .po file churn ...
> I don't know what tooling is used for that sort of stuff but I imagine
> that automated replacement might go a long way.

There's a msgfilter utility that can be used for automated updates.  I
documented one usage a few years ago for another mechanical change
nightmare
https://postgr.es/m/20190428173733.GA12483@alvherre.pgsql
So it sounds like it might not be _that_ difficult.

-- 
Álvaro Herrera PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"I can't go to a restaurant and order food because I keep looking at the
fonts on the menu.  Five minutes later I realize that it's also talking
about food" (Donald Knuth)

Re: logical replication: patch to ensure timely cleanup of aborted transactions in ReorderBuffer

[Ashutosh Bapat]

Hi Vaijayanti,


On Fri, Dec 6, 2024 at 2:50 PM Vaijayanti Bharadwaj
 wrote:
>
> Hello,
>
> This patch is a proposed fix for an issue that can occur in logical 
> replication where:
> 1. streaming of transactions is disabled.
> 2. First, say there is a transaction that's PREPAREd. This prepared 
> transaction is not resolved for some time. It holds back the oldestRunningXid.
> 3. Large transactions with xid higher than that of the prepared transaction 
> spill to disk as they are decoded in the ReorderBuffer.
> 4. There is an unclean shutdown due to which the large transactions get 
> aborted. But there is no explicit abort record.
> 5. On startup, the ReorderBuffer continues to decode and hold the aborted 
> transactions and they continue to accumulate as spill files.
> 6. Once the prepared transaction is resolved, these transactions will be 
> cleaned up, but if there is a delay, the aborted transactions could hold up 
> considerable space.
> (A customer had run into this issue at EDB)
>
> 0001: is a TAP test that reproduces this and tests for this. It fails without 
> this and passes with it.
> 0002: is the patch that fixes this issue.

A similar problem was discussed at [1]. That thread seems to have
halted without a commit. Can you please check if the solution there
works for you? If so review the patch and help move the thread ahead.

[1] 
https://www.postgresql.org/message-id/flat/CAD21AoDht9Pz_DFv_R2LqBTBbO4eGrpa9Vojmt5z5sEx3XwD7A%40mail.gmail.com

-- 
Best Wishes,
Ashutosh Bapat

Re: Proposal to add a new URL data type.

[Alexander Borisov]

05.12.2024 17:59, Peter Eisentraut пишет:

On 05.12.24 15:01, Alexander Borisov wrote:

Postgres users often store URLs in the database.  As an example, they
provide links to their pages on the web, analyze users posts and get
links for further storage and analysis.  Naturally, there is a need to
compare, sort, build indexes, get statistics on individual parts of
the URL: hosts, protocols, path and so on.

Adding a new URL type will standardize the work with URLs in Postgres
and provide simple tools for manipulating the new type.


Have you seen this: https://github.com/petere/pguri ?

The difference there is that it uses an external library for parsing the 
URLs, which makes the code much smaller.  Overall, the functionality 
looks pretty similar to yours at first glance.




Hi Peter,

I looked at your implementation of the URI extension.  You are using
a third party library uriparser for URI/URL parsing.  The uriparser
library is based on the RFC 3986 specification, which I cite in
comparison to WHATWG in my email.  The name of the functions to get the
individual parts of the URL will naturally be the same, that's how
URLs/URIs work. But you have to look at the point and perspective here.

As I've written before, there is a difference between parsing URLs
according to the RFC 3986 specification and WHATWG URLs.  This is
especially true for host.  Here are a couple more examples.

The specifications describe character encoding differently, this
applies to userinfo (username/password), path, query, fragment.
RFC 3986 standard encodes characters in the same way everywhere.
The characters to encode are: < > “ ` \r \n \t { } | \ ^ '.
The WHATWG URL standard takes a more selective and subtle approach.
For example, path may contain the character “|”, but userinfo does
not (encoded).

The WHATWG specification also requires that tabs and newlines be removed
from URLs before parsing.  The WHATWG character encoding gradation can
be found in the specification [1].

In addition to functions to retrieve individual parts, the WHATWG URL
standard describes an API for changing them: scheme, userinfo, host,
hostname, port, path, query, fragment.  There is not just one value is
replaced by another, there is a certain logic, which is not always
obvious.  For example, try to replace scheme, let's take the URL for
example: https://example.com/.
This URL contains special scheme (any others are not special), there
are only six of them: ftp, file, http, https, ws, wss.  And it is
impossible to replace scepial scheme with a non-special one.  More
precisely, the URL will be returned with special scheme, i.e. without
changes.  This is how you can check it with the patch I have given:

Example: select url_scheme_set('https://example.com/'::url, 'wss');
Result: wss://example.com/

Example: select url_scheme_set('https://example.com/'::url, 'myown');
Result: https://example.com/

Example: select url_scheme_set('best://example.com/'::url, 'myown');
Result: myown://example.com/

Example: select url_scheme_set('best://example.com/'::url, 'https');
Result: best://example.com/

In addition, WHATWG validates URLs during parsing and reports
non-critical errors [2].  If such errors occur, parsing continues.
However, in my implementation I don't output these errors, I just
haven't figured out how to do it correctly in SQL (as NOTICE?).

Without going further into the differences in specifications I could
say simply - RFC 3986 is obsolete, for example, node.js has labeled
the API with it as Legacy (they use WHATWG).

If we abstract from specifications and consider our approaches in
implementation.  You parse the URL every time for any request
(even to retrieve fragments).  In my implementation I proceed from
the fact that requests to read URLs will significantly exceed their
changes. Parsing is done once on input, the result is saved in
a special format and later the necessary parts of the URL
(or the whole URL) are retrieved. Also please note that there are
no dependencies on third-party libraries (ICU dependencies can also
be fought off).

All currently available functions and examples can be seen in
the README file, in the patch.

[1] https://url.spec.whatwg.org/#c0-control-percent-encode-set
[2] https://url.spec.whatwg.org/#writing


--
Alexander Borisov

Re: Considering fractional paths in Append node

[Andrei Lepikhov]

On 12/6/24 13:48, Andrei Lepikhov wrote:

On 11/2/24 01:18, Nikita Malakhov wrote:

I've corrected failing test and created a patch at Commitfest:
https://commitfest.postgresql.org/51/5361/ commitfest.postgresql.org/51/5361/>
I have played around with this feature, which looks promising for such a 
tiny change. It provides a 'bottom boundary' recommendation for 
appending subpaths, participating in the 'fractional branch' of paths.
As I see it works consistently with the plans, created for plain tables 
filled with similar data.
According to the proposal to change SeqScan logic, IMO, Andy is right. 
But it is a separate improvement because it wouldn't work in the case of 
LIMIT 10 or 100, as the newly added regression tests demonstrate.
I think this feature gives sensible profit for partitionwise paths. 
Pushing this knowledge into subpaths could help postgres_fdw to reduce 
network traffic.


See the attached patch: regression tests added; *_ext function removed - 
I think we wouldn't back-patch it into minor releases.


--
regards, Andrei LepikhovFrom bd2187403ce44b0bf72c9297552049abc6644469 Mon Sep 17 00:00:00 2001
From: "Andrei V. Lepikhov" 
Date: Thu, 5 Dec 2024 15:15:49 +0700
Subject: [PATCH v1] Teach Append to consider tuple_fraction when accumulating
 subpaths.

This change is dedicated to more active usage of IndexScan and parameterised
NestLoop paths in partitioned cases under an Append node, as it already works
with plain tables. As newly added regression tests demonstrate, it should
provide more smartness to the partitionwise technique.

Having an indication of how many tuples are needed, it may be more meaningful
to use the 'fractional branch' subpaths of the Append path list, which are more
optimal for this specific number of tuples. Planning on a higher level, if
the optimiser needs all the tuples, it will choose non-fractional paths.
In the case when, during execution, Append needs to return fewer tuples than
declared by tuple_fraction, it would not be harmful to use the 'intermediate'
variant of paths. However, it will earn a considerable profit if a sensible set
of tuples is selected.

The change of the existing regression test demonstrates the positive outcome of
this feature - instead of scanning the whole table, the optimiser prefers
to use a parameterised scan, being aware of the only single tuple the join has
to produce to perform the query.

Discussion: https://www.postgresql.org/message-id/flat/CAN-LCVPxnWB39CUBTgOQ9O7Dd8DrA_tpT1EY3LNVnUuvAX1NjA%40mail.gmail.com
---
 src/backend/optimizer/path/allpaths.c|  18 ++-
 src/backend/optimizer/plan/planner.c |   4 +
 src/test/regress/expected/partition_join.out | 116 +++
 src/test/regress/expected/union.out  |  15 ++-
 src/test/regress/sql/partition_join.sql  |  21 
 5 files changed, 164 insertions(+), 10 deletions(-)

diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c
index 172edb643a..4ac8d8e69b 100644
--- a/src/backend/optimizer/path/allpaths.c
+++ b/src/backend/optimizer/path/allpaths.c
@@ -1364,9 +1364,23 @@ add_paths_to_append_rel(PlannerInfo *root, RelOptInfo *rel,
 		 */
 		if (rel->consider_startup && childrel->cheapest_startup_path != NULL)
 		{
+			Path	   *cheapest_path;
+
+			/*
+			 * Having an indication on how much tuples the query should provide
+			 * the optimiser tries to choose the path, optimal for that specific
+			 * number of tuples.
+			 */
+			if (root->tuple_fraction > 0.0)
+cheapest_path =
+	get_cheapest_fractional_path(childrel,
+	 root->tuple_fraction);
+			else
+cheapest_path = childrel->cheapest_startup_path;
+
 			/* cheapest_startup_path must not be a parameterized path. */
-			Assert(childrel->cheapest_startup_path->param_info == NULL);
-			accumulate_append_subpath(childrel->cheapest_startup_path,
+			Assert(cheapest_path->param_info == NULL);
+			accumulate_append_subpath(cheapest_path,
 	  &startup_subpaths,
 	  NULL);
 		}
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index b665a7762e..0eed566185 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -6590,6 +6590,10 @@ get_cheapest_fractional_path(RelOptInfo *rel, double tuple_fraction)
 	{
 		Path	   *path = (Path *) lfirst(l);
 
+		/* Skip parameterized paths if requested */
+		if (path->param_info)
+			continue;
+
 		if (path == rel->cheapest_total_path ||
 			compare_fractional_path_costs(best_path, path, tuple_fraction) <= 0)
 			continue;
diff --git a/src/test/regress/expected/partition_join.out b/src/test/regress/expected/partition_join.out
index 108f9ecb44..3f7543074f 100644
--- a/src/test/regress/expected/partition_join.out
+++ b/src/test/regress/expected/partition_join.out
@@ -5225,6 +5225,122 @@ SELECT x.id, y.id FROM fract_t x LEFT JOIN fract_t y USING (id) ORDER BY x.id DE
  Index Cond: (id = x_2.id)
 (11 ro

Re: Popcount optimization using SVE for ARM

[devanga.susmi...@fujitsu.com]

Hi Kirill,
This work has been conducted independently and is not connected to 
https://www.postgresql.org/message-id/010101936e4aaa70-b474ab9e-b9ce-474d-a3ba-a3dc223d295c-00%40us-west-2.amazonses.com.

Our patch uses the existing infrastructure, i.e. the 
"choose_popcount_functions" method, to determine the correct popcount 
implementation based on the architecture, thereby requiring fewer code changes. 
The patch also includes implementations for popcount32, popcount64 and popcount 
masked. We'd be happy to discuss any potential overlaps and collaborate further 
to ensure the best solution is integrated.
Looking forward to your feedback!


Thanks & regards,
Susmitha Devanga.



From: Kirill Reshke 
Sent: Friday, December 6, 2024 12:52
To: Susmitha, Devanga 
Cc: pgsql-hack...@postgresql.org ; Hajela, Ragesh 
; Bhattacharya, Chiranmoy 
; M A, Rajat 
Subject: Re: Popcount optimization using SVE for ARM



On Fri, 6 Dec 2024 at 10:54, 
devanga.susmi...@fujitsu.com 
mailto:devanga.susmi...@fujitsu.com>> wrote:
Hello,   This email is to discuss the contribution of the speed-up popcount and 
popcount mask feature we have developed for the ARM architecture using SVE 
intrinsics.
The current method for popcount on ARM relies on compiler intrinsics or C code, 
which processes data in a scalar fashion, handling one integer at a time. By 
leveraging SVE intrinsics for popcount, the execution can process multiple 
integers simultaneously, depending on the vector length, thereby significantly 
enhancing the performance of the functionality.
We have designed this feature to ensure compatibility and robustness. It 
includes compile-time and runtime checks for SVE compatibility with both the 
compiler and hardware. If either check fails, the code falls back to the 
existing scalar implementation, ensuring fail-safe operation. Additionally, we 
leveraged the existing infrastructure to select between different popcount 
implementations, avoiding additional complexity.

Algorithm Overview:
1. For larger inputs, align the buffers to avoid double loads. For smaller 
inputs alignment is not necessary and might even degrade the performance.
2. Process the aligned buffer chunk by chunk till the last incomplete chunk.
3. Process the last incomplete chunk.
Our setup:
Machine: AWS EC2 c7g.8xlarge - 32vcpu, 64gb RAM
OS : Ubuntu 22.04.5 LTS
GCC: 11.4

Benchmark and Result:
We have used John Naylor's popcount-test-module [0] for benchmarking and 
observed a speed-up of more than 3x for larger buffers. Even for smaller inputs 
of size 8 and 32 bytes there aren't any performance degradations observed.

   [cid:ii_1939ad8bcdacb971f161]
 
[cid:ii_1939ad8bcdacb971f162]
We would like to contribute our above work so that it can be available for the 
community to utilize. To do so, we are following the procedure mentioned in 
Submitting a Patch - PostgreSQL 
wiki. Please find the 
attachments for the patch and performance results.
Please let us know if you have any queries or suggestions.


Thanks & Regards,
Susmitha Devanga.
Hi! Is this patch somehow related to [0] ?


[0] 
https://www.postgresql.org/message-id/010101936e4aaa70-b474ab9e-b9ce-474d-a3ba-a3dc223d295c-00%40us-west-2.amazonses.com

--
Best regards,
Kirill Reshke

Re: generic plans and "initial" pruning

[Amit Langote]

On Thu, Dec 5, 2024 at 11:07 PM Tomas Vondra  wrote:
> On 12/5/24 12:28, Amit Langote wrote:
> > On Thu, Dec 5, 2024 at 3:53 PM Amit Langote  wrote:
> >> On Thu, Dec 5, 2024 at 2:20 AM Tomas Vondra  wrote:
> >>> Sure, changing the APIs is allowed, I'm just wondering if maybe there
> >>> might be a way to not have this issue, or at least notice the missing
> >>> call early.
> >>>
> >>> I haven't tried, wouldn't it be better to modify ExecutorStart() to do
> >>> the retries internally? I mean, the extensions wouldn't need to check if
> >>> the plan is still valid, ExecutorStart() would take care of that. Yeah,
> >>> it might need some new arguments, but that's more obvious.
> >>
> >> One approach could be to move some code from standard_ExecutorStart()
> >> into ExecutorStart(). Specifically, the code responsible for setting
> >> up enough state in the EState to perform ExecDoInitialPruning(), which
> >> takes locks that might invalidate the plan. If the plan does become
> >> invalid, the hook and standard_ExecutorStart() are not called.
> >> Instead, the caller, ExecutorStartExt() in this case, creates a new
> >> plan.
> >>
> >> This avoids the need to add ExecPlanStillValid() checks anywhere,
> >> whether in core or extension code. However, it does mean accessing the
> >> PlannedStmt earlier than InitPlan(), but the current placement of the
> >> code is not exactly set in stone.
> >
> > I tried this approach and found that it essentially disables testing
> > of this patch using the delay_execution module, which relies on the
> > ExecutorStart_hook(). The way the testing works is that the hook in
> > delay_execution.c pauses the execution of a cached plan to allow a
> > concurrent session to drop an index referenced in the plan. When
> > unpaused, execution initialization resumes by calling
> > standard_ExecutorStart(). At this point, obtaining the lock on the
> > partition whose index has been dropped invalidates the plan, which the
> > hook detects and reports. It then also reports the successful
> > re-execution of an updated plan that no longer references the dropped
> > index.  Hmm.
> >
>
> It's not clear to me why the change disables this testing, and I can't
> try without a patch. Could you explain?

Sorry, PFA the delta patch for the change I described above.  It
applies on top of v58 series of patches that I posted yesterday.
You'll notice that delay_execution test fails if you apply and do
check-world.

As for how the change breaks the testing, here is a before and after
of the flow of a isolation test in
src/test/modules/delay_execution/specs/cached-plan-inval.spec (s1 is
the session used to run a cached plan, s2 to perform concurrent DDL
that invalidates the plan):

* Before (working):

1. s2 takes advisory lock
2. s1 runs cached plan -> goes to ExecutorStart_hook -> waits for the
advisory lock
3. s2 drops an index referenced in the plan
4. s2 unlocks advisory lock
5. s1 locks unpruned partitions -> detects plan invalidation due to
dropped index.

* After (stops working because initial pruning and locking are done
before calling ExecutorStart_hook):

1. s2 takes advisory lock
2. s1 runs cached plan -> locks unpruned partitions -> goes to
ExecutorStart_hook to get advisory lock -> waits for advisory lock
3. s2 drops an index referenced in the plan -> waits for lock on the
unpruned partition -> deadlock!

One idea I had after sending the email yesterday is to introduce
ExecutorStartCachedPlan_hook for the advisory lock based waiting.
ExecutorStartCachedPlan() is the new function that you will find in
v58-0004 that wraps ExecutorStart() to handle plan invalidation.  This
new hook would be called before ExecutorStartCachedPlan() calls
ExecutorStart(), so the original testing flow can still work.

Another idea might be to use injection points infra to introduce the
wait instead of the combination of a executor hook and advisory lock.

-- 
Thanks, Amit Langote


pruning-in-ExecutorStart.diff
Description: Binary data

RE: Memory leak in WAL sender with pgoutput (v10~)

[Zhijie Hou (Fujitsu)]

On Thursday, December 5, 2024 11:39 PM Euler Taveira   wrote:

> Thanks for taking care of it. I suggest 2 small adjustments: (a) use
> ALLOCSET_SMALL_SIZES instead of ALLOCSET_DEFAULT_SIZES and (b) replace
> pubmemcxt with pubmemctx (that's the same abbreviation used by
> cachectx). I think you could remove 'mem' from this variable. My
> suggestions are pubcxt or pubnamescxt. Although, I prefer the former, if
> other publication elements are added to this context in the future.

Thanks for the suggestions. They make sense to me.

Please see the updated version as attached.

Best Regards,
Hou zj


v2-0001-Fix-memory-leak-in-pgoutput-with-publication-list.patch
Description:  v2-0001-Fix-memory-leak-in-pgoutput-with-publication-list.patch

RE: Memory leak in WAL sender with pgoutput (v10~)

[Zhijie Hou (Fujitsu)]

On Thursday, December 5, 2024 12:52 PM Michael Paquier  
wrote:

Hi,

> 
> On Thu, Dec 05, 2024 at 04:31:56AM +, Zhijie Hou (Fujitsu) wrote:
> > I realized that this patch cannot be backpatched because it introduces
> > a new field into the public PGOutputData structure. Therefore, I think
> > we may need to use Alvaro's version [1] for the back branches.
> >
> 
> Thanks for the patch.
> 
> For HEAD it should be as good as it can be as it avoids the problem of
> CacheMemoryContext bloating for your case and my case.  Alvaro's patch
> would not take care of your case, unfortunately, but I'm less worried about 
> this
> case in the back branches and we don't track the parent context where
> StartupDecodingContext() has begun its work when building PGOutputData.
> Thoughts?

I am fine with the plan. Thanks.

Best Regards,
Hou zj

Re: generic plans and "initial" pruning

[Amit Langote]

On Thu, Dec 5, 2024 at 10:53 PM Tomas Vondra  wrote:
> On 12/5/24 07:53, Amit Langote wrote:
> > On Thu, Dec 5, 2024 at 2:20 AM Tomas Vondra  wrote:
> >> ...
> >>
>  What if an
>  extension doesn't do that? What weirdness will happen?
> >>>
> >>> The QueryDesc.planstate won't contain a PlanState tree for starters
> >>> and other state information that InitPlan() populates in EState based
> >>> on the PlannedStmt.
> >>
> >> OK, and the consequence is that the query will fail, right?
> >
> > No, the core executor will retry the execution with a new updated
> > plan.  In the absence of the early return, the extension might even
> > crash when accessing such incomplete QueryDesc.
> >
> > What the patch makes the ExecutorStart_hook do is similar to how
> > InitPlan() will return early when locks taken on partitions that
> > survive initial pruning invalidate the plan.
>
> Isn't that what I said? My question was what happens if the extension
> does not add the new ExecPlanStillValid() call - sorry if that wasn't
> clear. If it can crash, that's what I meant by "fail".

Ok, I see.  So, I suppose you meant to confirm if the invalid plan
won't silently be executed returning wrong results.  Yes, I don't
think that would happen given the kinds of invalidations that are
possible.  The various checks in the ExecInitNode() path, such as the
one that catches a missing index, will prevent the plan from running.
I may not have searched exhaustively enough though.

>  Maybe it'd be
>  possible to at least check this in some other executor hook? Or at least
>  we could ensure the check was done in assert-enabled builds? Or
>  something to make extension authors aware of this?
> >>>
> >>> I've added a note in the commit message, but if that's not enough, one
> >>> idea might be to change the return type of ExecutorStart_hook so that
> >>> the extensions that implement it are forced to be adjusted. Say, from
> >>> void to bool to indicate whether standard_ExecutorStart() succeeded
> >>> and thus created a "valid" plan.  I had that in the previous versions
> >>> of the patch.  Thoughts?
> >>
> >> Maybe. My concern is that this case (plan getting invalidated) is fairly
> >> rare, so it's entirely plausible the extension will seem to work just
> >> fine without the code update for a long time.
> >
> > You might see the errors like the one below when the core executor or
> > a hook tries to initialize or process in some other way a known
> > invalid plan, for example, because an unpruned partition's index got
> > concurrently dropped before the executor got the lock:
> >
> > ERROR: could not open relation with OID xxx
>
> Yeah, but how likely is that? How often get plans invalidated in regular
> application workload. People don't create or drop indexes very often,
> for example ...

Yeah, that's a valid point.  Andres once mentioned that ANALYZE can
invalidate plans and that can occur frequently in busy systems.

> Again, I'm not saying requiring the call would be unacceptable, I'm sure
> we made similar changes in the past. But if it wasn't needed without too
> much contortion, that would be nice.

I tend to agree.

Another change introduced by the patch that extensions might need to
mind (noted in the commit message of v58-0004) is the addition of the
es_unpruned_relids field to EState. This field tracks the RT indexes
of relations that are locked and therefore safe to access during
execution. Importantly, it does not include the RT indexes of leaf
partitions that are pruned during "initial" pruning and thus remain
unlocked.

This change means that executor extensions can no longer assume that
all relations in the range table are locked and safe to access.
Instead, extensions must account for the possibility that some
relations, specifically pruned partitions, are not locked. Normally,
executor code accesses relations using ExecGetRangeTableRelation(),
which does not take a lock before returning the Relation pointer,
assuming that locks are already managed upstream.

-- 
Thanks, Amit Langote

refactor AlterDomainAddConstraint (alter domain add constraint)

[jian he]

hi.
attached patch refactor AlterDomainAddConstraint

* change the error message:
alter domain d_int add constraint nn not null no inherit;
from
ERROR:  NOT NULL constraints cannot be marked NO INHERIT
to
ERROR:  DOMAIN with NOT NULL constraints cannot be marked NO INHERIT

basically processCASbits
from
processCASbits($3, @3, "NOT NULL")
processCASbits($5, @5, "CHECK")
to
processCASbits($3, @3, "DOMAIN with NOT NULL")
processCASbits($5, @5, "DOMAIN with CHECK")


* error out check constraint no inherit with domain. so the following
should fail.
alter domain d_int add constraint cc check(value > 1) no inherit; --should fail

* delete code in AlterDomainAddConstraint, since it will be unreachable.

* alter domain d_int add constraint cc2 check(value > 11) not
deferrable initially immediate not valid;
"not deferrable", "initially immediate" cannot error out at the moment.
maybe we can just document it in create_domain.sgml?

* some failed regress test, similar to thread (Pass ParseState as down
to utility functions)

you may also see the patch draft commit message.
From 23186a7ee0b7ebc10ecdab87558d1158676c35d9 Mon Sep 17 00:00:00 2001
From: jian he 
Date: Fri, 6 Dec 2024 16:13:51 +0800
Subject: [PATCH v1 1/1] refactor AlterDomainAddConstraint

In gram.y only CHECK and NOT-NULL constraint are allowed for the ALTER DOMAIN ...
ADD CONSTRAINT statement.  so we can safely remove the code handles errors for
other constraint type.

as of constraint other property ([ DEFERRABLE | NOT DEFERRABLE ] [
INITIALLY DEFERRED | INITIALLY IMMEDIATE ]) we alerady handled in processCASbits
(gram.y).

however AlterDomainAddConstraint only pass single (Node *newConstraint).  that
means we cannot trigger an error for constraints specified as "NOT DEFERRABLE"
or "INITIALLY IMMEDIATE" because "NOT DEFERRABLE" or "INITIALLY IMMEDIATE" is
actualy a seperated Constraint node.

To error out case like:
alter domain d_int add constraint cc1 check(value > 1) not deferrable initially immediate;
we need to make DomainConstraint in gram.y more like TableConstraint.
that means this patch, "not deferrable" and "initially immediate" words is accepted,
maybe i should change create_domain.sgml Accordingly.

i also made "alter domain d_int add constraint cc check(value > 1) no inherit;"
fail.

the error message is not so good, for example in master, for
`alter domain d_int add constraint nn not null no inherit;`
you get:
ERROR:  NOT NULL constraints cannot be marked NO INHERIT

but NOT NULL constraints can be marked NO INHERIT.
for domain constraint part, i slightly changed the third parameter of
processCASbits while calling.  so now the error message becomes:

ERROR:  DOAMIN with NOT NULL constraints cannot be marked NO INHERIT
---
 src/backend/commands/typecmds.c  | 33 
 src/backend/parser/gram.y|  6 +--
 src/test/regress/expected/domain.out | 58 
 src/test/regress/sql/domain.sql  | 22 +++
 4 files changed, 83 insertions(+), 36 deletions(-)

diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c
index da591c0922..aa715402e1 100644
--- a/src/backend/commands/typecmds.c
+++ b/src/backend/commands/typecmds.c
@@ -2942,39 +2942,6 @@ AlterDomainAddConstraint(List *names, Node *newConstraint,
 			/* processed below */
 			break;
 
-		case CONSTR_UNIQUE:
-			ereport(ERROR,
-	(errcode(ERRCODE_SYNTAX_ERROR),
-	 errmsg("unique constraints not possible for domains")));
-			break;
-
-		case CONSTR_PRIMARY:
-			ereport(ERROR,
-	(errcode(ERRCODE_SYNTAX_ERROR),
-	 errmsg("primary key constraints not possible for domains")));
-			break;
-
-		case CONSTR_EXCLUSION:
-			ereport(ERROR,
-	(errcode(ERRCODE_SYNTAX_ERROR),
-	 errmsg("exclusion constraints not possible for domains")));
-			break;
-
-		case CONSTR_FOREIGN:
-			ereport(ERROR,
-	(errcode(ERRCODE_SYNTAX_ERROR),
-	 errmsg("foreign key constraints not possible for domains")));
-			break;
-
-		case CONSTR_ATTR_DEFERRABLE:
-		case CONSTR_ATTR_NOT_DEFERRABLE:
-		case CONSTR_ATTR_DEFERRED:
-		case CONSTR_ATTR_IMMEDIATE:
-			ereport(ERROR,
-	(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-	 errmsg("specifying constraint deferrability not supported for domains")));
-			break;
-
 		default:
 			elog(ERROR, "unrecognized constraint subtype: %d",
  (int) constr->contype);
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 67eb96396a..ccab075bea 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -4321,9 +4321,9 @@ DomainConstraintElem:
 	n->location = @1;
 	n->raw_expr = $3;
 	n->cooked_expr = NULL;
-	processCASbits($5, @5, "CHECK",
+	processCASbits($5, @5, "DOMAIN with CHECK",
    NULL, NULL, &n->skip_validation,
-   &n->is_no_inherit, yyscanner);
+   NULL, yyscanner);
 	n->initially_valid = !n->skip_validation;
 	$$ = (Node *) n;
 }
@@ -4335,7 +4335,7 @@ DomainConstraintElem:
 	n->loc

Re: Why we need to check for local buffers in BufferIsExclusiveLocked and BufferIsDirty?

[Nazir Bilal Yavuz]

Hi,

On Fri, 6 Dec 2024 at 07:03, Srinath Reddy Sadipiralla
 wrote:
>
> >  On Thu, 05 Dec 2024 21:11:42 +0530 Andres Freund  
> > wrote ---
>
> > Hi,
>
> > On 2024-12-05 18:38:16 +0530, Srinath Reddy Sadipiralla wrote:
> >> Why we need to check for local buffers in BufferIsExclusiveLocked and
> >> BufferIsDirty?,these 2 functions are called only from
> >> XlogRegisterBuffer,AFAIK which will be called only for permanent
> >> relations.Please correct me if i am wrong.
>
> > That's maybe true for in-core code today, but what guarantees that that's 
> > true
> > for the future? And what about code in extensions?
>
> > The gain by not dealing with local buffers in these functions is fairly 
> > small
> > too, so there's not really any reason for a change like yours.
>
> > - Andres
>
>
> hmm got it,if thats the case, for local buffers lockbuffer will skip 
> acquiring content lock, so assert will fail in BufferIsDirty.

LGTM.

Adding Daniil to CC as he too started a similar thread [1].

[1] 
postgr.es/m/CAJDiXggGznOttwREfyZRE4f7oLRz1%3DjTA4xA7u-t6_8CX7j%3D0g%40mail.gmail.com

--
Regards,
Nazir Bilal Yavuz
Microsoft

Re: Fix tiny memory leaks

[Daniel Gustafsson]

> On 6 Dec 2024, at 09:38, Tofig Aliev  wrote:

> There is a memory leak in functions check_application_name() and 
> check_cluster_name().
> Functions are located in src/backend/commands/variable.c

LGTM.

--
Daniel Gustafsson

Re: NOT ENFORCED constraint feature

[Amul Sul]

On Thu, Dec 5, 2024 at 11:02 AM jian he  wrote:
>
> hi.
> accidentally hit segfault.
> create table c11 (a int not enforced);
> create table c11 (a int enforced);
> we can solve it via the following or changing SUPPORTS_ATTRS accordingly.
>
> diff --git a/src/backend/parser/parse_utilcmd.c
> b/src/backend/parser/parse_utilcmd.c
> index 5ab44149e5..fe1116c092 100644
> --- a/src/backend/parser/parse_utilcmd.c
> +++ b/src/backend/parser/parse_utilcmd.c
> @@ -3965,7 +3965,7 @@ transformConstraintAttrs(CreateStmtContext *cxt,
> List *constraintList)
> break;
> case CONSTR_ATTR_ENFORCED:
> -   if (lastprimarycon &&
> +   if (lastprimarycon == NULL ||
> lastprimarycon->contype != 
> CONSTR_CHECK)
> ereport(ERROR,
>
> (errcode(ERRCODE_SYNTAX_ERROR),
> @@ -3981,7 +3981,7 @@ transformConstraintAttrs(CreateStmtContext *cxt,
> List *constraintList)
> break;
> case CONSTR_ATTR_NOT_ENFORCED:
> -   if (lastprimarycon &&
> +   if (lastprimarycon == NULL ||
> lastprimarycon->contype != 
> CONSTR_CHECK)
> ereport(ERROR,
>
> (errcode(ERRCODE_SYNTAX_ERROR),
>

Yes, that was a logical oversight on my part. Your suggestion looks
good to me, thanks.

>
> ALTER DOMAIN constraint_comments_dom ADD CONSTRAINT the_constraint
> CHECK (value > 0) NOT ENFORCED;
> ERROR:  CHECK constraints cannot be marked NOT ENFORCED
>
> the error message is not good?  maybe better option would be:
> ERROR:  DOMAIN CHECK constraints cannot be marked NOT ENFORCED
>
> we can do it like:
> index 833b3be02b..4a7ab0c2a3 100644
> --- a/src/backend/parser/gram.y
> +++ b/src/backend/parser/gram.y
> @@ -4342,7 +4342,7 @@ DomainConstraintElem:
> n->location = @1;
> n->raw_expr = $3;
> n->cooked_expr = NULL;
> -   processCASbits($5, @5, "CHECK",
> +  processCASbits($5, @5, "DOMAIN CHECK",
>
> NULL, NULL, NULL, &n->skip_validation,
>
> &n->is_no_inherit, yyscanner);

I believe this should either be a separate patch or potentially
included in your "Refactor AlterDomainAddConstraint" proposal[1].

Regards,
Amul

1] 
https://postgr.es/m/cacjufxhitd5lglbssapshhtdwxt0vivkthinkyw-skbx93t...@mail.gmail.com

logical replication: patch to ensure timely cleanup of aborted transactions in ReorderBuffer

[Vaijayanti Bharadwaj]

Hello,

This patch is a proposed fix for an issue that can occur in logical
replication where:
1. streaming of transactions is disabled.
2. First, say there is a transaction that's PREPAREd. This prepared
transaction is not resolved for some time. It holds back the
oldestRunningXid.
3. Large transactions with xid higher than that of the prepared transaction
spill to disk as they are decoded in the ReorderBuffer.
4. There is an unclean shutdown due to which the large transactions get
aborted. But there is no explicit abort record.
5. On startup, the ReorderBuffer continues to decode and hold the aborted
transactions and they continue to accumulate as spill files.
6. Once the prepared transaction is resolved, these transactions will be
cleaned up, but if there is a delay, the aborted transactions could hold up
considerable space.
(A customer had run into this issue at EDB)

0001: is a TAP test that reproduces this and tests for this. It fails
without this and passes with it.
0002: is the patch that fixes this issue.

Thanks,
Vaijayanti


0001-TAP-test-that-passes-with-the-ReorderBuffer-cleanup-.patch
Description: Binary data


0002-cleanup-incomplete-transactions-on-startup-in-Reorde.patch
Description: Binary data

Fix tiny memory leaks

[Tofig Aliev]

Hi, hackers!

There is a memory leak in functions check_application_name() and 
check_cluster_name().

Functions are located in src/backend/commands/variable.c

The leak can be triggered using SQL command: SET 
application_name=new_name;
You can run pgbench with this command for a long time by passing this 
command in file via '-f' flag.


regards,
Tofig
From a801d03c57819d3de181967d72260cb42bbcf6a3 Mon Sep 17 00:00:00 2001
From: Tofig Aliev 
Date: Fri, 6 Dec 2024 14:58:38 +0700
Subject: [PATCH] Fix tiny memory leaks

Memory for (*newval) allocated earlier
in the parse_and_validate_value() method.
Since this method re-allocates memory,
it is necessary to free the previously allocated memory.
---
 src/backend/commands/variable.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/src/backend/commands/variable.c b/src/backend/commands/variable.c
index 0ecff94d0e..2c8059e8d8 100644
--- a/src/backend/commands/variable.c
+++ b/src/backend/commands/variable.c
@@ -1092,6 +1092,8 @@ check_application_name(char **newval, void **extra, GucSource source)
 		return false;
 	}
 
+	guc_free(*newval);
+
 	pfree(clean);
 	*newval = ret;
 	return true;
@@ -1128,6 +1130,8 @@ check_cluster_name(char **newval, void **extra, GucSource source)
 		return false;
 	}
 
+	guc_free(*newval);
+
 	pfree(clean);
 	*newval = ret;
 	return true;
-- 
2.43.0

Re: Pass ParseState as down to utility functions.

[jian he]

hi.

extensive test for
ATExecAddOf
DefineType
ATPrepAlterColumnType
ATExecAlterColumnType
DefineDomain
AlterType
transformAlterTableStmt

only AlterType, ATExecAlterColumnType function code change no tests.
AlterType doesn't have location info, can not print it out.
ATExecAlterColumnType is unreachable, because ATPrepAlterColumnType
catched most of the error.

especially extensive tests for DefineDomain.
AlterDomainAddConstraint related error case, i created another thread
(refactor AlterDomainAddConstraint (alter domain add constraint))
From 6bf657c3b62b7460b317c42ce2f4fa0988acf1a0 Mon Sep 17 00:00:00 2001
From: jian he 
Date: Fri, 6 Dec 2024 16:37:18 +0800
Subject: [PATCH v6 1/1] print out error position for some DDL command

doing this by passing the source_string to the existing ParseState
or by making a new ParseState passing source_string to it.

With this patch, the following functions will printout the error position for certain error cases.

ATExecAddOf
DefineType
ATPrepAlterColumnType
ATExecAlterColumnType
DefineDomain
AlterType
transformAlterTableStmt

ATExecAlterColumnType code change maybe not necessary, since
ATPrepAlterColumnType will catach most of the error.  AlterType function changes
no effect since struct AlterTypeStmt don't have location info.

ATExecAlterColumnType, AlterType because of the above mentioned reason, don't
have regress test.  all other have tests.

This can be particularly helpful when working with a sequence of DML commands,
such as `create schema create schema_element`.
It also makes it easier to quickly identify the relevant error area in a single DDL command

extensive regerss tests (failed case) for CREATE DOMAIN added.

discussion: https://postgr.es/m/CALdSSPhqfvKbDwqJaY=yeepi_aq61gmmpw88i6zh7cmg_2z...@mail.gmail.com
---
 src/backend/commands/tablecmds.c  | 43 +++
 src/backend/commands/typecmds.c   | 56 --
 src/backend/parser/parse_utilcmd.c|  8 +-
 src/backend/tcop/utility.c|  4 +-
 src/include/commands/typecmds.h   |  4 +-
 src/test/regress/expected/alter_table.out | 21 ++
 .../regress/expected/collate.icu.utf8.out |  2 +
 .../regress/expected/collate.linux.utf8.out   |  2 +
 src/test/regress/expected/collate.out |  2 +
 .../expected/collate.windows.win1252.out  |  2 +
 src/test/regress/expected/domain.out  | 74 +++
 src/test/regress/expected/float8.out  |  5 ++
 src/test/regress/expected/identity.out|  4 +
 src/test/regress/expected/typed_table.out |  4 +
 src/test/regress/sql/alter_table.sql  |  7 ++
 src/test/regress/sql/domain.sql   | 21 ++
 src/test/regress/sql/float8.sql   |  2 +
 src/test/regress/sql/identity.sql |  1 +
 18 files changed, 218 insertions(+), 44 deletions(-)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 6ccae4cb4a..efa38b1470 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -593,7 +593,8 @@ static void ATPrepAlterColumnType(List **wqueue,
   AlterTableUtilityContext *context);
 static bool ATColumnChangeRequiresRewrite(Node *expr, AttrNumber varattno);
 static ObjectAddress ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel,
-		   AlterTableCmd *cmd, LOCKMODE lockmode);
+		   AlterTableCmd *cmd, LOCKMODE lockmode,
+		   AlterTableUtilityContext *context);
 static void RememberAllDependentForRebuilding(AlteredTableInfo *tab, AlterTableType subtype,
 			  Relation rel, AttrNumber attnum, const char *colName);
 static void RememberConstraintForRebuilding(Oid conoid, AlteredTableInfo *tab);
@@ -639,7 +640,9 @@ static ObjectAddress ATExecAddInherit(Relation child_rel, RangeVar *parent, LOCK
 static ObjectAddress ATExecDropInherit(Relation rel, RangeVar *parent, LOCKMODE lockmode);
 static void drop_parent_dependency(Oid relid, Oid refclassid, Oid refobjid,
    DependencyType deptype);
-static ObjectAddress ATExecAddOf(Relation rel, const TypeName *ofTypename, LOCKMODE lockmode);
+static ObjectAddress ATExecAddOf(Relation rel, const TypeName *ofTypename,
+ LOCKMODE lockmode,
+ AlterTableUtilityContext *context);
 static void ATExecDropOf(Relation rel, LOCKMODE lockmode);
 static void ATExecReplicaIdentity(Relation rel, ReplicaIdentityStmt *stmt, LOCKMODE lockmode);
 static void ATExecGenericOptions(Relation rel, List *options);
@@ -5413,7 +5416,7 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab,
 			break;
 		case AT_AlterColumnType:	/* ALTER COLUMN TYPE */
 			/* parse transformation was done earlier */
-			address = ATExecAlterColumnType(tab, rel, cmd, lockmode);
+			address = ATExecAlterColumnType(tab, rel, cmd, lockmode, context);
 			break;
 		case AT_AlterColumnGenericOptions:	/* ALTER COLUMN OPTIONS */
 			address =
@@ -5537,7 +5540,7 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab,
 			

Re: Make pg_stat_io view count IOs as bytes instead of blocks

[Nazir Bilal Yavuz]

Hi,

On Thu, 5 Dec 2024 at 12:13, Bertrand Drouvot
 wrote:
>
> Hi,
>
> On Wed, Dec 04, 2024 at 02:49:11PM +0300, Nazir Bilal Yavuz wrote:
> > On Thu, 28 Nov 2024 at 16:39, Bertrand Drouvot
> >  wrote:
> > >
> > You are right, no need to have this check; it can not be less than 0.
> > I completely removed the function now.
>
> Thanks! Yeah I think that makes sense.
>
> > > What about ordering the enum in IOOp (no bytes/bytes) so that we could 
> > > check
> > > that io_op >= "our firt bytes enum" instead?
> > >
> > > Also we could create a macro on top of that to make it clear. And a 
> > > comment
> > > would be needed around the IOOp definition.
> > >
> > > I think that would be simpler to maintain should we add no bytes or bytes 
> > > op in
> > > the future.
> >
> > I think this is a good idea. I applied all the comments.
>
> Thanks!
>
> + * non-byte-measured and byte-measured operations. So, that makes is easier
> + * to check whether IO is measured in bytes.
>
> s/that makes is/that makes it/

Done.

> + *
> + * Note: If this enum is modified, ensure that both `IOOP_NUM_TYPES` macro
> + * and the `ioop_measured_in_bytes` function are updated accordingly.
>
> Yeah, or?
>
> "Ensure IOOP_EXTEND is the first and IOOP_WRITE the last ones in the measured 
> in
> bytes" group and that the groups stay in that order.
>
> That would make the "checks" local to the enum def should someone modify it.

Makes sense, done.

> > Created an
> > inline function instead of macro and added this 'Assert((unsigned int)
> > io_object < IOOBJECT_NUM_TYPES);' to function.
>
> An inline function seems the right choice for me too.
>
> > Done. I moved these checks to the pgstat_count_io_op_n() function. The
> > code looks more like its previous version now.
>
> Thanks! Yeah, more easier to follow now.
>
> > > Not sure about "can do IO in bytes" (same wording is used in multiple 
> > > places).
> >
> > I changed it to 'measured in bytes' but I am not sure if this is
> > better, open to suggestions.
>
> I'm tempted to say something around "track", would mean things like:
>
> 1.
>
> ioop_measured_in_bytes => is_ioop_tracked_in_bytes
>
> 2.
>
> s/If an op does not do IO in bytes/If an op does not track bytes/
>
> 3.
>
> s/not every IO measured in bytes/not every IO tracks bytes/
>
> 4.
>
> s/non-byte-measured and byte-measured/non-tracking and tracking bytes/
>
> Thoughts?

Thanks! I think 'track' is a better word in this context. I used
'tracked in ...', as it sounded more correct to me (I hope it is).

--
Regards,
Nazir Bilal Yavuz
Microsoft
From 992ec6ebf116358fb70eb4d6c42c0f6ef5e1500e Mon Sep 17 00:00:00 2001
From: Nazir Bilal Yavuz 
Date: Fri, 6 Dec 2024 12:06:17 +0300
Subject: [PATCH v3] Make pg_stat_io count IOs as bytes instead of blocks

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

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

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

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

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

Re: Popcount optimization using SVE for ARM

[Kirill Reshke]

I did not yet look into this in detail,  but please note that PostgreSQL
comments style is /**/ not //. Also, please,  do not top post on this list

Re: Track the amount of time waiting due to cost_delay

[Bertrand Drouvot]

Hi,

On Thu, Dec 05, 2024 at 10:43:51AM +, Bertrand Drouvot wrote:
> Yeah, people would likely use this new field to monitor long running vacuum.
> Long enough that this error should be acceptable. Do you agree?

OTOH, adding the 100% accuracy looks as simple as v9-0002 attached (0001 is
same as for v8), so I think we should provide it.

Thoughts?

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
>From 8be8a71eb3c010d51bd6749dce33794f763c8572 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot 
Date: Mon, 24 Jun 2024 08:43:26 +
Subject: [PATCH v9 1/2] Report the total amount of time that vacuum has been
 delayed due to cost delay

This commit adds one column: time_delayed to the pg_stat_progress_vacuum system
view to show the total amount of time in milliseconds that vacuum has been
delayed.

This uses the new parallel message type for progress reporting added
by f1889729dd.

In case of parallel worker, to avoid the leader to be interrupted too frequently
(while it might be sleeping for cost delay), the report is done only if the last
report has been done more than 1 second ago.

Having a time based only approach to throttle the reporting of the parallel
workers sounds reasonable.

Indeed when deciding about the throttling:

1. The number of parallel workers should not come into play:

 1.1) the more parallel workers is used, the less the impact of the leader on
 the vacuum index phase duration/workload is (because the repartition is done
 on more processes).

 1.2) the less parallel workers is, the less the leader will be interrupted (
 less parallel workers would report their delayed time).

2. The cost limit should not come into play as that value is distributed
proportionally among the parallel workers (so we're back to the previous point).

3. The cost delay does not come into play as the leader could be interrupted at
the beginning, the midle or whatever part of the wait and we are more interested
about the frequency of the interrupts.

3. A 1 second reporting "throttling" looks a reasonable threshold as:

 3.1 the idea is to have a significant impact when the leader could have been
interrupted say hundred/thousand times per second.

 3.2 it does not make that much sense for any tools to sample pg_stat_progress_vacuum
multiple times per second (so a one second reporting granularity seems ok).

Would need to bump catversion because this changes the definition of
pg_stat_progress_vacuum.
---
 doc/src/sgml/monitoring.sgml | 13 +++
 src/backend/catalog/system_views.sql |  2 +-
 src/backend/commands/vacuum.c| 53 
 src/include/commands/progress.h  |  1 +
 src/test/regress/expected/rules.out  |  3 +-
 5 files changed, 70 insertions(+), 2 deletions(-)
  22.8% doc/src/sgml/
   4.0% src/backend/catalog/
  67.6% src/backend/commands/
   3.8% src/test/regress/expected/

diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 840d7f8161..7386f7333d 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -6428,6 +6428,19 @@ FROM pg_stat_get_backend_idset() AS backendid;
cleaning up indexes.
   
  
+
+ 
+  
+   time_delayed bigint
+  
+  
+   Total amount of time spent in milliseconds waiting due to 
+   or . In case of parallel
+   vacuum the reported time is across all the workers and the leader. The
+   workers update the column no more frequently than once per second, so it
+   could show slightly old values.
+  
+ 
 

   
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index da9a8fe99f..013bd06222 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -1222,7 +1222,7 @@ CREATE VIEW pg_stat_progress_vacuum AS
 S.param4 AS heap_blks_vacuumed, S.param5 AS index_vacuum_count,
 S.param6 AS max_dead_tuple_bytes, S.param7 AS dead_tuple_bytes,
 S.param8 AS num_dead_item_ids, S.param9 AS indexes_total,
-S.param10 AS indexes_processed
+S.param10 AS indexes_processed, S.param11 AS time_delayed
 FROM pg_stat_get_progress_info('VACUUM') AS S
 LEFT JOIN pg_database D ON S.datid = D.oid;
 
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index bb639ef51f..6f9e515f56 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -39,6 +39,7 @@
 #include "catalog/pg_inherits.h"
 #include "commands/cluster.h"
 #include "commands/defrem.h"
+#include "commands/progress.h"
 #include "commands/vacuum.h"
 #include "miscadmin.h"
 #include "nodes/makefuncs.h"
@@ -59,6 +60,16 @@
 #include "utils/snapmgr.h"
 #include "utils/syscache.h"
 
+/*
+ * Minimum amount of time (in ms) between two reports of the delayed time from a
+ * parallel worker to the leader. The goal is to avoid the leader to be
+ * interrupted 

[BUG] temp_table_max_size parameter may entail an error within ReadBuffer function

[Daniil Davydov]

Hi,
I noticed that this sequence of actions (for temporary table) leads to
an error, if
temp_table_max_size parameter is set :
***
LockRelationForExtension(relation, ExclusiveLock);
buf = ReadBuffer(relation, P_NEW);
***

Error occurs during total temporary table size calculation
(find_total_temp_relation_size function,
that can be called only if temp_table_max_size parameter > 0) : we are
trying to open all table's
indexes and append their size to the total size of the temporary
table. But inside relation_open function
(called for index) we meet this assert (and, of course, fail on it):

***
/*
 * We don't acquire any other heavyweight lock while holding the relation
 * extension lock.  We do allow to acquire the same relation extension
 * lock more than once but that case won't reach here.
 */
Assert(!IsRelationExtensionLockHeld);
***

I suppose that the simplest way out of the situation would be to skip
locking temporary tables for
extension

--
Best regards,
Daniil Davydov
From da84d916e568be0d2414303f4a1e1d01b0bc6abd Mon Sep 17 00:00:00 2001
From: Daniil Davidov 
Date: Fri, 6 Dec 2024 16:41:53 +0700
Subject: [PATCH] Fix bug with locking temp relation for extension

---
 src/backend/storage/lmgr/lmgr.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/backend/storage/lmgr/lmgr.c b/src/backend/storage/lmgr/lmgr.c
index 094522acb4..0a91109d03 100644
--- a/src/backend/storage/lmgr/lmgr.c
+++ b/src/backend/storage/lmgr/lmgr.c
@@ -421,6 +421,9 @@ LockRelationForExtension(Relation relation, LOCKMODE lockmode)
 {
 	LOCKTAG		tag;
 
+	if (relation->rd_islocaltemp)
+		return;
+
 	SET_LOCKTAG_RELATION_EXTEND(tag,
 relation->rd_lockInfo.lockRelId.dbId,
 relation->rd_lockInfo.lockRelId.relId);
-- 
2.43.0

Re: why there is not VACUUM FULL CONCURRENTLY?

[Alvaro Herrera]

On 2024-Oct-09, Antonin Houska wrote:

> diff --git a/doc/src/sgml/ref/vacuum.sgml b/doc/src/sgml/ref/vacuum.sgml
> index 9110938fab..f1008f5013 100644
> --- a/doc/src/sgml/ref/vacuum.sgml
> +++ b/doc/src/sgml/ref/vacuum.sgml

> @@ -61,8 +62,12 @@ VACUUM [ (  class="parameter">option [, ...] ) ] [ 
> Without a table_and_columns
> list, VACUUM processes every table and materialized 
> view
> -   in the current database that the current user has permission to vacuum.
> -   With a list, VACUUM processes only those table(s).
> +   in the current database that the current user has permission to vacuum. If
> +   the CONCURRENTLY is specified (see below), tables which
> +   have not been clustered yet are silently skipped. With a
> +   list, VACUUM processes only those table(s). If
> +   the CONCURRENTLY is specified, the list may only 
> contain
> +   tables which have already been clustered.
>

The idea that VACUUM CONCURRENTLY can only process tables that have been
clustered sounds very strange to me.  I don't think such a restriction
would really fly.  However, I think this may just be a documentation
mistake; can you please clarify?  I am tempted to suggest that VACUUM
CONCURRENTLY should receive a table list; without a list, it should
raise an error.  This is not supposed to be a routine maintenance
command that you can run on all your tables, after all.  Heck, maybe
don't even accept a partitioned table -- the user can process one
partition at a time, if they need that.


I don't believe in the need for the LOCK_CLUSTER_CONCURRENT define; IMO
the code should just use ShareUpdateExclusiveLock where needed.

In 0001, the new API of make_new_heap() is somewhat bizarre regarding
the output lockmode_new_p parameter.  I didn't find any place in the
patch series where we use that to return a different lock level that the
caller gave; the only case were we do something that looks funny is when
a toast table is involved.  But I don't think I fully understand what is
going on in that case.  I'm likely missing something here, but isn't it
simpler to just state that make_new_heap will obtain a lock on the new
heap, and that the immediately following table_open needn't acquire a
lock (or, in the case of RefreshMatViewByOid, no LockRelationOid is
necessary)?

Anyway, I propose some cosmetic cleanups for 0001 in attachment,
including changing make_new_heap to assume a non-null value of
lockmode_new_p.  I didn't go as far as making it no longer a pointer,
but if it can be done, then I suggest we should do that.  I didn't try
to apply the next patches in the series after this one.

-- 
Álvaro HerreraBreisgau, Deutschland  —  https://www.EnterpriseDB.com/
>From eec9e6dfc4aa5a4f52a82065e3d4973cdbbff09f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=81lvaro=20Herrera?= 
Date: Fri, 6 Dec 2024 14:39:02 +0100
Subject: [PATCH] Minor code review

---
 src/backend/commands/cluster.c   | 72 
 src/backend/commands/matview.c   |  7 +++-
 src/backend/commands/tablecmds.c |  5 ++-
 src/backend/commands/vacuum.c|  5 +--
 4 files changed, 36 insertions(+), 53 deletions(-)

diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index e32abf15e69..4a62aff46bd 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -191,13 +191,11 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool 
isTopLevel)

stmt->indexname, stmt->relation->relname)));
}
 
+   /* For non-partitioned tables, do what we came here to do. */
if (rel->rd_rel->relkind != RELKIND_PARTITIONED_TABLE)
{
-   /*
-* Do the job. (The function will close the relation, 
lock is kept
-* till commit.)
-*/
cluster_rel(rel, indexOid, ¶ms);
+   /* cluster_rel closes the relation, but keeps lock */
 
return;
}
@@ -284,11 +282,9 @@ cluster_multiple_rels(List *rtcs, ClusterParams *params)
 
rel = table_open(rtc->tableOid, AccessExclusiveLock);
 
-   /*
-* Do the job. (The function will close the relation, lock is 
kept
-* till commit.)
-*/
+   /* Process this table */
cluster_rel(rel, rtc->indexOid, params);
+   /* cluster_rel closes the relation, but keeps lock */
 
PopActiveSnapshot();
CommitTransactionCommand();
@@ -301,8 +297,7 @@ cluster_multiple_rels(List *rtcs, ClusterParams *params)
  * This clusters the table by creating a new, clustered table and
  * swapping the relfilenumbers of the new table and the old table, so
  * the OID of the original table is preserved.  Thus we do not lose
- * GRANT, inheritance nor references to this table (

Re: Proposal to add a new URL data type.

[Daniel Gustafsson]

> On 6 Dec 2024, at 13:59, Alexander Borisov  wrote:

> As I've written before, there is a difference between parsing URLs
> according to the RFC 3986 specification and WHATWG URLs.  This is
> especially true for host.  Here are a couple more examples.

As someone who wears another open-source hat which is heavily involved in
parsing URLs I cannot stress enough how much I think postgres should avoid
this.  The example url http://http://http://@http://http://?http://#http:// is
a valid url, but is rejected by a number of implementations and parsed
differently by most that accept it.

A URL datatype is a *good idea* but one which I personally believe is best
handled as an external extension.

--
Daniel Gustafsson

Re: Pass ParseState as down to utility functions.

[Alvaro Herrera]

On 2024-Dec-06, jian he wrote:

> From 6bf657c3b62b7460b317c42ce2f4fa0988acf1a0 Mon Sep 17 00:00:00 2001
> From: jian he 
> Date: Fri, 6 Dec 2024 16:37:18 +0800
> Subject: [PATCH v6 1/1] print out error position for some DDL command
> 
> doing this by passing the source_string to the existing ParseState
> or by making a new ParseState passing source_string to it.
> 
> With this patch, the following functions will printout the error position for 
> certain error cases.
> 
> ATExecAddOf
> DefineType
> ATPrepAlterColumnType
> ATExecAlterColumnType
> DefineDomain
> AlterType
> transformAlterTableStmt

I think it would make more sense to write the commit message in terms of
the DDL commands that now report error position, than the C functions.
Such a list of commands does not need to be exhaustive; a
representative-enough sample probably suffices.

> @@ -943,11 +942,13 @@ DefineDomain(CreateDomainStmt *stmt)

>   if (constr->is_no_inherit)
>   ereport(ERROR,
> - 
> errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
> - errmsg("not-null 
> constraints for domains cannot be marked NO INHERIT"));
> + 
> (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
> +  errmsg("not-null 
> constraints for domains cannot be marked NO INHERIT"),
> +  
> parser_errposition(pstate, constr->location)));

Once upon a time, ereport() was a simpler macro that did not
use variadic arguments.  Back then, the list of functions embedded in it
(errcode, errmsg etc) were forced to be in an additional level of
parentheses so that the macro would work at all (IIRC failure to do that
resulted in strange compile-time problems).  This is why a majority of
code is written in the style with those parens.  But commit e3a87b4991cc
changed ereport to use __VA_ARGS__, so the auxiliary functions are
actual arguments to errstart() -- which means that the parentheses
you're adding here are unnecessary and discouraged.  Just add the
parser_errposition() call and it'll be fine.

-- 
Álvaro HerreraBreisgau, Deutschland  —  https://www.EnterpriseDB.com/
"XML!" Exclaimed C++.  "What are you doing here? You're not a programming
language."
"Tell that to the people who use me," said XML.
https://burningbird.net/the-parable-of-the-languages/

inconsistent use of SearchSysCacheCopy

[Jeff Davis]

When modifying catalog contents, some callers use SearchSysCacheN() and
some use SearchSysCacheCopyN().

for instance, these callers use SearchSysCacheN():

  - AggregateCreate()
  - ProcedureCreate()

while these callers that use SearchSysCacheCopyN():

  - OperatorCreate()
  - TypeCreate()

I generally thought that the non-copy variant was preferred if you are
using heap_modify_tuple() or similar, and the latter used when
necessary (e.g. modifying through the Form_pg_foo pointer). Aside from
avoiding a needless copy, the non-copy variant is also a bit safer
because you'll get a WARNING if you forget to ReleaseSysCache().

On the other hand, if you don't use the copy variant, and you actually
do modify it, that would be quite dangerous; so perhaps that's why the
copy variants are used in so many places where it seems unnecessary.

Is there a preferred convention?

Regards,
Jeff Davis

Re: Statistics Import and Export

[Jeff Davis]

On Wed, 2024-11-27 at 00:08 -0500, Corey Huinker wrote:
> 
> 0003 - Re-enabling in-place updates because catalog bloat bad.

Attached is my version of this patch, which I intend to commit soon.

I added docs and tests, and I refactored a bit to check the arguments
first.

Also, I separated the mvcc and in-place paths, so that it was easier to
review that each one is following the right protocol.

Regards,
Jeff Davis

From a41503b82469297797db78163426c40a7a86317f Mon Sep 17 00:00:00 2001
From: Jeff Davis 
Date: Fri, 6 Dec 2024 12:32:29 -0800
Subject: [PATCH v33j] Use in-place updates for pg_restore_relation_stats().

This matches the behavior of vac_update_relstats(), which is important
to avoid bloating pg_class.

Author: Corey Huinker
Discussion: https://postgr.es/m/CADkLM=fc3je+ufv3gshqjjssf+t8674rxpuxw62el55mueq...@mail.gmail.com
---
 doc/src/sgml/func.sgml |   8 +
 src/backend/statistics/relation_stats.c| 198 +
 src/test/regress/expected/stats_import.out |  61 +++
 src/test/regress/sql/stats_import.sql  |  37 
 4 files changed, 233 insertions(+), 71 deletions(-)

diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 8b81106fa23..2c35252dc06 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -30175,6 +30175,14 @@ postgres=# SELECT '0/0'::pg_lsn + pd.segment_number * ps.setting::int + :offset
  function is to maintain a consistent function signature to avoid
  errors when restoring statistics from previous versions.
 
+
+ To match the behavior of  and  when updating relation statistics,
+ pg_restore_relation_stats() does not follow MVCC
+ transactional semantics (see ). New relation
+ statistics may be durable even if the transaction aborts, and the
+ changes are not isolated from other transactions.
+
 
  Arguments are passed as pairs of argname
  and argvalue, where
diff --git a/src/backend/statistics/relation_stats.c b/src/backend/statistics/relation_stats.c
index e619d5cf5b1..f84157b16ab 100644
--- a/src/backend/statistics/relation_stats.c
+++ b/src/backend/statistics/relation_stats.c
@@ -20,6 +20,7 @@
 #include "access/heapam.h"
 #include "catalog/indexing.h"
 #include "statistics/stat_utils.h"
+#include "utils/fmgroids.h"
 #include "utils/fmgrprotos.h"
 #include "utils/syscache.h"
 
@@ -50,59 +51,28 @@ static struct StatsArgInfo relarginfo[] =
 	[NUM_RELATION_STATS_ARGS] = {0}
 };
 
-static bool relation_statistics_update(FunctionCallInfo fcinfo, int elevel);
+static bool relation_statistics_update(FunctionCallInfo fcinfo, int elevel,
+	   bool inplace);
 
 /*
  * Internal function for modifying statistics for a relation.
  */
 static bool
-relation_statistics_update(FunctionCallInfo fcinfo, int elevel)
+relation_statistics_update(FunctionCallInfo fcinfo, int elevel, bool inplace)
 {
 	Oid			reloid;
 	Relation	crel;
-	HeapTuple	ctup;
-	Form_pg_class pgcform;
-	int			replaces[3] = {0};
-	Datum		values[3] = {0};
-	bool		nulls[3] = {0};
-	int			ncols = 0;
-	TupleDesc	tupdesc;
+	int32		relpages = DEFAULT_RELPAGES;
+	bool		update_relpages = false;
+	float		reltuples = DEFAULT_RELTUPLES;
+	bool		update_reltuples = false;
+	int32		relallvisible = DEFAULT_RELALLVISIBLE;
+	bool		update_relallvisible = false;
 	bool		result = true;
 
-	stats_check_required_arg(fcinfo, relarginfo, RELATION_ARG);
-	reloid = PG_GETARG_OID(RELATION_ARG);
-
-	if (RecoveryInProgress())
-		ereport(ERROR,
-(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
- errmsg("recovery is in progress"),
- errhint("Statistics cannot be modified during recovery.")));
-
-	stats_lock_check_privileges(reloid);
-
-	/*
-	 * Take RowExclusiveLock on pg_class, consistent with
-	 * vac_update_relstats().
-	 */
-	crel = table_open(RelationRelationId, RowExclusiveLock);
-
-	tupdesc = RelationGetDescr(crel);
-	ctup = SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(reloid));
-	if (!HeapTupleIsValid(ctup))
-	{
-		ereport(elevel,
-(errcode(ERRCODE_OBJECT_IN_USE),
- errmsg("pg_class entry for relid %u not found", reloid)));
-		table_close(crel, RowExclusiveLock);
-		return false;
-	}
-
-	pgcform = (Form_pg_class) GETSTRUCT(ctup);
-
-	/* relpages */
 	if (!PG_ARGISNULL(RELPAGES_ARG))
 	{
-		int32		relpages = PG_GETARG_INT32(RELPAGES_ARG);
+		relpages = PG_GETARG_INT32(RELPAGES_ARG);
 
 		/*
 		 * Partitioned tables may have relpages=-1. Note: for relations with
@@ -116,17 +86,13 @@ relation_statistics_update(FunctionCallInfo fcinfo, int elevel)
 	 errmsg("relpages cannot be < -1")));
 			result = false;
 		}
-		else if (relpages != pgcform->relpages)
-		{
-			replaces[ncols] = Anum_pg_class_relpages;
-			values[ncols] = Int32GetDatum(relpages);
-			ncols++;
-		}
+		else
+			update_relpages = true;
 	}
 
 	if (!PG_ARGISNULL(RELTUPLES_ARG))
 	{
-		float		reltuples = PG_GETARG_FLOAT4(RELTUPLES_ARG);
+		reltuples = PG_GETARG_FLOAT4(RELTUPLES

Re: Using Expanded Objects other than Arrays from plpgsql

[Michel Pelletier]

On Tue, Dec 3, 2024 at 4:42 PM Tom Lane  wrote:

> Michel Pelletier  writes:
> > Here's a WIP patch for a pgexpanded example in src/test/modules.
>
> I didn't look at your patch yet, but in the meantime here's an update
> that takes the next step towards what I promised.
>

Awesome!  I made a support function for my set_element(matrix, i, j, value)
function and it works great, but I do have a couple questions about how to
move forward with some of my other methods.  For reference, here's the
set_element test function, and a trace of function calls, after
implementing the support function for set_element, the expand_matrix
function is called twice, first by nvals(), and later by the first
set_element, but not the subsequent sets, so the true clause of the
PLPGSQL_RWOPT_INPLACE case in plpgsql_param_eval_var_check works great and
as expected:

postgres=# create or replace function test_se(graph matrix) returns matrix
language plpgsql as
$$
declare
nvals bigint;
begin
graph = wait(graph);
nvals = nvals(graph);
raise notice 'nvals: %', nvals;
graph = set_element(graph, 4, 2, 42);
graph = set_element(graph, 4, 3, 43);
graph = set_element(graph, 4, 4, 44);
return graph;
end;
$$;
CREATE FUNCTION
postgres=# select nvals(test_se('int32'::matrix));
DEBUG:  new_matrix
DEBUG:  matrix_get_flat_size
DEBUG:  flatten_matrix
DEBUG:  matrix_wait
DEBUG:  DatumGetMatrix
DEBUG:  expand_matrix<- wait expands with support function
DEBUG:  new_matrix
DEBUG:  matrix_nvals
DEBUG:  DatumGetMatrix
DEBUG:  matrix_get_flat_size
DEBUG:  flatten_matrix
DEBUG:  expand_matrix<- nvals() reexpands
DEBUG:  new_matrix
DEBUG:  context_callback_matrix_free
NOTICE:  nvals: 0
DEBUG:  scalar_int32
DEBUG:  new_scalar
DEBUG:  matrix_set_element
DEBUG:  DatumGetMatrix< set_element does not reexpand, yay!
DEBUG:  DatumGetScalar
DEBUG:  context_callback_scalar_free
DEBUG:  scalar_int32
DEBUG:  new_scalar
DEBUG:  matrix_set_element
DEBUG:  DatumGetMatrix
DEBUG:  DatumGetScalar
DEBUG:  context_callback_scalar_free
DEBUG:  scalar_int32
DEBUG:  new_scalar
DEBUG:  matrix_set_element
DEBUG:  DatumGetMatrix
DEBUG:  DatumGetScalar
DEBUG:  context_callback_scalar_free
DEBUG:  matrix_nvals
DEBUG:  DatumGetMatrix
DEBUG:  context_callback_matrix_free
DEBUG:  context_callback_matrix_free
┌───┐
│ nvals │
├───┤
│ 3 │
└───┘

 My question is about nvals = nvals(graph) in that function above, the
object is flattened and then rexpanded, even after the object was expanded
and returned by wait() [1] into a r/w pointer.  set_element honors the
expanded object from wait(), but nvals does not.  It seems like I want to
be able to pass the argument as a RO pointer, but I'm not sure how to
trigger the "else" clause in the PLPGSQL_RWOPT_INPLACE case.  I can see how
the support function triggers the if, but I don't see a similar way to
trigger the else.  I'm almost certainly missing something.

[1](Due to the asynchronous nature of the GraphBLAS API, there is a
GrB_wait() function to wait until an object is "complete" computationally,
but since this object was just expanded, there is no pending work, so the
wait() is essentially a noop but a handy way for me to return a r/w pointer
for subsequent operations).

set_element and wait are the simple case where there is only one reference
to the r/w object in the argument list.  As we've discussed I have many
functions that can possibly have multiple references to the same object.
The core operation of the library is matrix multiplication, and all other
complex functions follow a very similar pattern, so I'll just focus on that
one here:

CREATE FUNCTION mxm(
a matrix,
b matrix,
op semiring default null,
inout c matrix default null,
mask matrix default null,
accum binaryop default null,
descr descriptor default null
)
RETURNS matrix

The order of arguments mostly follows the order in the C API.  a and b are
the left and right matrix operands and the matrix product is the return
value.  If c is not null, then it is a pre-created return value which may
contain partial results already from some previous operations, otherwise
mxm creates a new matrix of the correct dimensions and returns that.  I
think the inout is meaningless as it doesn't seem to change anything, but
I'm using it as a visual indication in code that c can be the return value
if it's not null.

Here's an example of doing Triangle Counting using Burkhardt's method [2],
where a, b, and the mask are all the same adjacency matrix (here the
Newman/karate graph.  The 'plus_pair' semiring is optimized for structural
counting, and the descriptor 's' tells suitesparse to only use the
structure of the mask and not to consider the values):

[2] https://doi.org/10.1177/147387161393

CREATE OR REPLACE FUNCTION public.tcount_burkhardt(graph matrix)
 RETURNS bigint
 LANGUAGE plpgsql
AS $$
begin
graph = wait(

Re: Potential null pointer dereference in postgres.c

[Maxim Orlov]

I'm glad you are bringing up this issue. By the way, there are two more
annoying places in postmaster.c for pg16 and older. See, strdup() also may
fail if insufficient memory available.

PFA patch for a REL_16_STABLE. It also applies to older versions.

-- 
Best regards,
Maxim Orlov.


v2-0001-Use-pstrdup-for-remote_host-and-remote_port-save-.patch
Description: Binary data

Re: refactor AlterDomainAddConstraint (alter domain add constraint)

[jian he]

On Fri, Dec 6, 2024 at 10:48 PM Alvaro Herrera  wrote:
>
> On 2024-Dec-06, jian he wrote:
>
> > basically processCASbits
> > from
> > processCASbits($3, @3, "NOT NULL")
> > processCASbits($5, @5, "CHECK")
> > to
> > processCASbits($3, @3, "DOMAIN with NOT NULL")
> > processCASbits($5, @5, "DOMAIN with CHECK")
>
> This doesn't actually work from a translation point of view, because the
> actual error message is split in two parts.  I think it might be better
> to pass a non-NULL variable to processCASbits, then in the caller check
> whether that flag is true; if so, raise the error in a single ereport().
>

i let the error fail at AlterDomainAddConstraint.

what do you think?
From 106ec073dfae8d5854a948ffc3e89c4fae79e129 Mon Sep 17 00:00:00 2001
From: jian he 
Date: Fri, 6 Dec 2024 23:58:34 +0800
Subject: [PATCH v2 1/1] refactor AlterDomainAddConstraint

In gram.y only CHECK and NOT-NULL constraint are allowed for the ALTER DOMAIN ...
ADD CONSTRAINT statement.  so we can safely remove the code handles errors for
other constraint type.

as of constraint other property ([ DEFERRABLE | NOT DEFERRABLE ] [
INITIALLY DEFERRED | INITIALLY IMMEDIATE ]) we alerady handled in processCASbits
(gram.y).

however AlterDomainAddConstraint only pass single (Node *newConstraint).  that
means we cannot trigger an error for constraints specified as "NOT DEFERRABLE"
or "INITIALLY IMMEDIATE" because "NOT DEFERRABLE" or "INITIALLY IMMEDIATE" is
actualy a seperated Constraint node.

To error out case like:
alter domain d_int add constraint cc1 check(value > 1) not deferrable initially immediate;
we need to make DomainConstraint in gram.y more like TableConstraint.
that means this patch, "not deferrable" and "initially immediate" words is accepted,
maybe i should change create_domain.sgml Accordingly.

i also made "alter domain d_int add constraint cc check(value > 1) no inherit;"
fail.

the error message is not so good, for example in master, for
`alter domain d_int add constraint nn not null no inherit;`
you get:
ERROR:  NOT NULL constraints cannot be marked NO INHERIT

but NOT NULL constraints can be marked NO INHERIT.
for domain constraint part, i slightly changed the third parameter of
processCASbits while calling.  so now the error message becomes:

ERROR:  DOAMIN with NOT NULL constraints cannot be marked NO INHERIT

discussion: https://postgr.es/m/cacjufxhitd5lglbssapshhtdwxt0vivkthinkyw-skbx93t...@mail.gmail.com
---
 src/backend/commands/tablecmds.c |  2 +-
 src/backend/commands/typecmds.c  | 68 +---
 src/backend/parser/gram.y| 12 ++---
 src/backend/tcop/utility.c   |  3 +-
 src/include/commands/typecmds.h  |  2 +-
 src/test/regress/expected/domain.out | 58 
 src/test/regress/sql/domain.sql  | 22 +
 7 files changed, 121 insertions(+), 46 deletions(-)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 6ccae4cb4a..2dd33c0435 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -5390,7 +5390,7 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab,
 			address =
 AlterDomainAddConstraint(((AlterDomainStmt *) cmd->def)->typeName,
 		 ((AlterDomainStmt *) cmd->def)->def,
-		 NULL);
+		 NULL, context->queryString);
 			break;
 		case AT_ReAddComment:	/* Re-add existing comment */
 			address = CommentObject((CommentStmt *) cmd->def);
diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c
index da591c0922..96da0fd11d 100644
--- a/src/backend/commands/typecmds.c
+++ b/src/backend/commands/typecmds.c
@@ -2903,7 +2903,7 @@ AlterDomainDropConstraint(List *names, const char *constrName,
  */
 ObjectAddress
 AlterDomainAddConstraint(List *names, Node *newConstraint,
-		 ObjectAddress *constrAddr)
+		 ObjectAddress *constrAddr, const char *querystring)
 {
 	TypeName   *typename;
 	Oid			domainoid;
@@ -2913,10 +2913,13 @@ AlterDomainAddConstraint(List *names, Node *newConstraint,
 	Constraint *constr;
 	char	   *ccbin;
 	ObjectAddress address = InvalidObjectAddress;
+	ParseState 	*pstate;
+	pstate = make_parsestate(NULL);
+	pstate->p_sourcetext = querystring;
 
 	/* Make a TypeName so we can use standard type lookup machinery */
 	typename = makeTypeNameFromNameList(names);
-	domainoid = typenameTypeId(NULL, typename);
+	domainoid = typenameTypeId(pstate, typename);
 
 	/* Look up the domain in the type table */
 	typrel = table_open(TypeRelationId, RowExclusiveLock);
@@ -2938,43 +2941,34 @@ AlterDomainAddConstraint(List *names, Node *newConstraint,
 	switch (constr->contype)
 	{
 		case CONSTR_CHECK:
+			if (constr->is_no_inherit)
+ereport(ERROR,
+		errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+		errmsg("DOMAIN with CHECK constraints cannot be marked NO INHERIT"),
+		parser_errposition(pstate, constr->location));
+			if (constr->deferrable || constr->initdeferred)
+ereport(ERROR,
+		errcode(ERRCODE_FEA

Re: Giving the shared catalogues a defined encoding

[Tom Lane]

Thomas Munro  writes:
> Problem #1:  You can have two databases with different encodings, and
> they both pretend that pg_database, pg_authid, pg_db_role_setting etc
> are in the local database encoding.  That doesn't work too well:
> non-ASCII text can be reinterpreted in the wrong encoding.

> There's no problem if you only use one encoding everywhere (probably
> UTF8).  There's also no problem if you use multiple database
> encodings, but put only ASCII in the shared catalogues (because ASCII
> is a subset of every supported server encoding).  This patch is about
> formalising and enforcing those two working arrangements, hopefully
> invisibly to most users.  There's still an escape hatch mode if you
> need it, e.g. for a non-conforming pg_upgrade'd system.

Over in the discussion of bug #18735, I've come to the realization
that these problems apply equally to the filesystem path names that
the server deals with: not only the data directory path, but the
path to the installation files [1].  Can we apply the same sort of
restrictions to those?  I'm envisioning that initdb would check
either encoding-validity or all-ASCII-ness of those path names
depending on which mode it's setting the server up in.

> The patch invents a new setting CLUSTER CATALOG ENCODING, which can be
> inspected with SHOW and changed with ALTER SYSTEM.

Changing the catalog encoding would also have to re-verify the
suitability of the paths.  Of course this isn't 100% bulletproof
since someone could rename those directories later.  But I think
that's in "if you break it you get to keep both pieces" territory.

regards, tom lane

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

Re: refactor AlterDomainAddConstraint (alter domain add constraint)

[Alvaro Herrera]

On 2024-Dec-06, jian he wrote:

> basically processCASbits
> from
> processCASbits($3, @3, "NOT NULL")
> processCASbits($5, @5, "CHECK")
> to
> processCASbits($3, @3, "DOMAIN with NOT NULL")
> processCASbits($5, @5, "DOMAIN with CHECK")

This doesn't actually work from a translation point of view, because the
actual error message is split in two parts.  I think it might be better
to pass a non-NULL variable to processCASbits, then in the caller check
whether that flag is true; if so, raise the error in a single ereport().

BTW the way to test this is to apply your patch, then do "make
update-po", then look at the src/backend/po/*.po.new files which contain
the merged strings.  In this case, your new "DOMAIN with NOT NULL" string
is not going to appear in the message catalog, because processCASbits()
is not listed in GETTEXT_TRIGGERS in nls.mk.

-- 
Álvaro Herrera   48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Nunca se desea ardientemente lo que solo se desea por razón" (F. Alexandre)

Re: meson missing test dependencies

[Peter Eisentraut]

On 05.12.24 21:10, Andres Freund wrote:

Hi,

On 2024-12-05 20:08:24 +0100, Peter Eisentraut wrote:

On 03.12.24 17:01, Andres Freund wrote:

On 2024-12-02 11:10:56 +0100, Peter Eisentraut wrote:
That's unfortunately a very partial fix - because we insist on tests being run
against a temporary install, we don't just need to rebuild the code, we also
need to re-install it.  Without that you don't, e.g., see server changes.


Right, I was willing to accept that as a compromise, but see below.


However, it looks like the tmp_install test *does* miss dependencies too and I
see no reason to not fix that.



diff --git i/meson.build w/meson.build
index ff3848b1d85..55b751a0c6b 100644
--- i/meson.build
+++ w/meson.build
@@ -3263,6 +3263,7 @@ test('tmp_install',
   priority: setup_tests_priority,
   timeout: 300,
   is_parallel: false,
+depends: all_built,
   suite: ['setup'])

   test('install_test_files',


Yes, that addresses my cube example.


Let's do that. I'm inclined to only do so on master, but I can also see
backpatching it. Arguments?


master is enough for me.

Re: Proposal to add a new URL data type.

[Alexander Borisov]

Hi Daniel,

06.12.2024 16:46, Daniel Gustafsson пишет:

On 6 Dec 2024, at 13:59, Alexander Borisov  wrote:



As I've written before, there is a difference between parsing URLs
according to the RFC 3986 specification and WHATWG URLs.  This is
especially true for host.  Here are a couple more examples.


As someone who wears another open-source hat which is heavily involved in
parsing URLs I cannot stress enough how much I think postgres should avoid
this.  The example url http://http://http://@http://http://?http://#http:// is
a valid url, but is rejected by a number of implementations and parsed
differently by most that accept it.


Your example is valid, yes, it looks scary, t might catch someone off
guard.  At the same time your URL is correctly parsed both RFC 3986
and WHATWG URL.
There are many examples of “scary” URLs that you can't even understand
how they are parsed.  You can write a URL with any intimidating host,
path, scheme, but that's not what I mean.
There are generally accepted standards for URL/URI parsing RFC 3986 and
WHATWG URL.  We are not talking about self-written implementations
(without relying on any specifications) or those who made a mistake
while implementing one of the standards.
I propose to implement support for one of the standards that looks
promising.  On the contrary, everything is quite clear.  All we need to
do is point out that we have a URL data type in extension by WHATWG
specification.  I would even say that by creating a new type we will
contribute to the standardization of this zoo.

It's about creating a new URL data type according to the
specification WHATWG and including it in contrib as an extension.


--
Alexander Borisov

Re: Proposal to add a new URL data type.

[Matthias van de Meent]

On Thu, 5 Dec 2024 at 15:02, Alexander Borisov  wrote:
> What is the main difference between WHATWG and RFC 3986?
[snip]
> [host]
> Source: https://exаmple.com/ (а — U+0430)
> RFC 3986: https://exаmple.com/.
> WHATWG: https://xn--exmple-4nf.com/.
[snip]
> [path]
> Source: https://example.com/a/./b/../c
> RFC 3986: https://example.com/a/./b/../c.
> WHATWG: https://example.com/a/c.
[snip]
> Proposal
>
> I propose to add a new data type for PostgreSQL as an extension, in
> contrib.  Name the new type URL and use the WHATWG URL specification to
> implement the new type.

I'd be extremely annoyed if URLs I wrote into the database didn't
return in identical manner when fetched from the database. See also
how numeric has different representations of the same value: 2.0 and
2.00 are equivalent for sorting purposes, they aren't the same and
cannot just truncate those zeroes. Note that a path of "/%2e/" could
well be interpreted differently from "/./" or "/" by a server.

> The choice of URL parsing specification is
> justified by the following factors:
> 1. Live specification, adopts to modern realities.

I don't think choosing to defer to a living standard is a good idea
for contrib extensions, which are expected to be supported and stable
with the major PostgreSQL release they're bundled with. If (when) that
living standard gets updated, as tends to happen to such standards,
we'd suddenly lose compatibility with the standard we said we
supported, which isn't a nice outlook. Compare that to RFCs, which
AFAIK don't change in specification once released.

Kind regards,

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

Re: Popcount optimization using SVE for ARM

[Nathan Bossart]

I suggest we move this discussion to the existing thread on this subject:


https://www.postgresql.org/message-id/flat/010101936e4aaa70-b474ab9e-b9ce-474d-a3ba-a3dc223d295c-00%40us-west-2.amazonses.com

-- 
nathan

Re: Remove dependence on integer wrapping

[Nathan Bossart]

On Thu, Dec 05, 2024 at 09:58:29PM -0600, Nathan Bossart wrote:
> Thanks for reviewing.  In v28, I fixed a silly mistake revealed by cfbot's
> Windows run.  I also went ahead and tried to fix most of the issues
> reported in a nearby thread [0].  The only one I haven't tracked down yet
> is the "ISO week" one (case 7).

The overflow hazard for the ISO week case seems to be in isoweek2j(), which
would require quite a bit of restructuring to work with the soft-error
handling from its callers.  I'm not feeling particularly inspired to do
that, so here's a v29 with a comment added above that function.

-- 
nathan
>From 99032579840c75c15acb88e6d9fb9a8a76a39dcd Mon Sep 17 00:00:00 2001
From: Nathan Bossart 
Date: Fri, 6 Dec 2024 10:16:51 -0600
Subject: [PATCH v29 1/1] Fix various overflow hazards in date and timestamp
 functions.

Reported-by: Alexander Lakhin
Author: Matthew Kim, Nathan Bossart
Reviewed-by: Joseph Koshakow, Jian He
Discussion: https://postgr.es/m/31ad2cd1-db94-bdb3-f91a-65ffdb4bef95%40gmail.com
Discussion: https://postgr.es/m/18585-db646741dd649abd%40postgresql.org
Backpatch-through: 13
---
 src/backend/utils/adt/date.c   |   9 ++-
 src/backend/utils/adt/formatting.c | 104 +++--
 src/backend/utils/adt/timestamp.c  |   4 +
 src/include/common/int.h   |  48 
 src/test/regress/expected/date.out |   2 +
 src/test/regress/expected/horology.out |  16 
 src/test/regress/sql/date.sql  |   1 +
 src/test/regress/sql/horology.sql  |   8 ++
 8 files changed, 183 insertions(+), 9 deletions(-)

diff --git a/src/backend/utils/adt/date.c b/src/backend/utils/adt/date.c
index 8130f3e8ac..da61ac0e86 100644
--- a/src/backend/utils/adt/date.c
+++ b/src/backend/utils/adt/date.c
@@ -256,8 +256,15 @@ make_date(PG_FUNCTION_ARGS)
/* Handle negative years as BC */
if (tm.tm_year < 0)
{
+   int year = tm.tm_year;
+
bc = true;
-   tm.tm_year = -tm.tm_year;
+   if (pg_neg_s32_overflow(year, &year))
+   ereport(ERROR,
+   
(errcode(ERRCODE_DATETIME_FIELD_OVERFLOW),
+errmsg("date field value out of range: 
%d-%02d-%02d",
+   tm.tm_year, tm.tm_mon, 
tm.tm_mday)));
+   tm.tm_year = year;
}
 
dterr = ValidateDate(DTK_DATE_M, false, false, bc, &tm);
diff --git a/src/backend/utils/adt/formatting.c 
b/src/backend/utils/adt/formatting.c
index 2bcc185708..a9daa5c591 100644
--- a/src/backend/utils/adt/formatting.c
+++ b/src/backend/utils/adt/formatting.c
@@ -77,6 +77,7 @@
 
 #include "catalog/pg_collation.h"
 #include "catalog/pg_type.h"
+#include "common/int.h"
 #include "common/unicode_case.h"
 #include "common/unicode_category.h"
 #include "mb/pg_wchar.h"
@@ -3826,7 +3827,14 @@ DCH_from_char(FormatNode *node, const char *in, 
TmFromChar *out,
ereturn(escontext,,

(errcode(ERRCODE_INVALID_DATETIME_FORMAT),
 
errmsg("invalid input string for \"Y,YYY\"")));
-   years += (millennia * 1000);
+
+   /* years += (millennia * 1000); */
+   if (pg_mul_s32_overflow(millennia, 
1000, &millennia) ||
+   pg_add_s32_overflow(years, 
millennia, &years))
+   ereturn(escontext,,
+   
(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+errmsg("value 
for \"Y,YYY\" in source string is out of range")));
+
if (!from_char_set_int(&out->year, 
years, n, escontext))
return;
out->yysz = 4;
@@ -4785,10 +4793,35 @@ do_to_timestamp(text *date_txt, text *fmt, Oid collid, 
bool std,
tm->tm_year = tmfc.year % 100;
if (tm->tm_year)
{
+   int tmp;
+
if (tmfc.cc >= 0)
-   tm->tm_year += (tmfc.cc - 1) * 100;
+   {
+   /* tm->tm_year += (tmfc.cc - 1) * 100; 
*/
+   tmp = tmfc.cc - 1;
+   if (pg_mul_s32_overflow(tmp, 100, &tmp) 
||
+   
pg_add_s32_overflow(tm->tm_year, tmp, &tm->tm_year))
+   {
+

Re: Proposal to Enable/Disable Index using ALTER INDEX (with patch)

[Shayon Mukherjee]

On Mon, Nov 25, 2024 at 6:19 PM David Rowley  wrote:

> Another safer option could be to disallow the enable/disable ALTER
> TABLE if indcheckxmin is true. We do have and use
> ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE for these sorts of issues.
> There are other existing failure modes relating to indexes that can
> depend on what another session has done, e.g. MarkInheritDetached()
> can fail if another session detached an index concurrently. I could
> respect an argument that this one might be worse than that as its
> timing dependent rather than dependent on what another session has
> done.
>

Thank you for that feedback David. It's very useful.

I have attached a v4 patch that now raises an error if indcheckxmin is true
before attempting to enable/disable an index, and asks the caller to wait
until indcheckxmin is balanced again via log message.

I couldn't come up with any reliable (non-flaky) way of getting
indcheckxmin to report true in regression specs for extra coverage. So, I
ended up "simulating" it by directly updating the relevant row for an index
and marking indcheckxmin as true in specs and accordingly asserting. The
specs now cover all the previous cases and also the new case where the
error would be raised if the caller attempts to enable/disable an index
while indcheckxmin is true.

The patch is also rebased against the latest master and passing in CI.
Would love to receive any further feedback on it.

Thank you everyone!
Shayon


v4-0001-Introduce-the-ability-to-enable-disable-indexes-u.patch
Description: Binary data

Unable to Recover a Deleted Database Using PITR

[Scott Taylor]

Created database randomdata with one table for test purposes of point in
time recovery.
However, the database did not fully restore.
This consistently does not work.
Am I missing a step or concept about how PITR works or can you not restore
a delete database using PITR?
I am using postgres version: PostgreSQL 17.2 on x86_64-windows

Steps:
1) Updated postgresql.conf:
archive_mode = on
archive_command = 'copy "%p" "C:\\PostgresArchive\\Wal-Archive\\%f"'
log_statement = mod
summarize_wal = on

2) Re-started postgres server

3) Created a database "randomdata" and table "somedata", inserted data:
*2024-12-02 13:57:24*.049 EST [31268] LOG:  statement: insert into
somedata (serialnumber, firstname, lastname)

4) Ran pg_basebackup

5) Dropped database:
   * 2024-12-02 14:01:27*.243 EST [19148] LOG:  statement: DROP DATABASE
randomdata;

6) Stopped postgres server

7) Removed contents of data folder: C:\Program Files\PostgreSQL\17\data

8) Extracted base.tar.gz (from pg_basebackup) into C:\Program
Files\PostgreSQL\17\data

9) Removed contents of pg_wal folder: C:\Program
Files\PostgreSQL\17\data\pg_wal

10) Added to postgresql.conf:
restore_command = 'copy "C:\\PostgresArchive\\Wal-Archive\\%f" "%p"'
recovery_target_time = '*2024-12-02 13:57:24*' (time from log file when
insert occured, see step 2)
recovery_target_action = promote

11) Created recovery.signal file in postgres data folder

12) Started postgres server

13) Attempted to connect to database using psql:
postgres=# \c randomdata;
connection to server at "localhost" (::1), port 5432 failed: FATAL:
 cannot connect to invaliddatabase "randomdata"
HINT:  Use DROP DATABASE to drop invalid databases.
Previous connection kept

14) Noticed recovery.signal file was removed

*Log file:*
2024-12-02 13:57:23.852 EST [32352] LOG:  statement: create database
randomdata;
2024-12-02 13:57:24.043 EST [31268] LOG:  statement: create table somedata
(id serial primary key, serialnumber integer, firstname text, lastname
text);
2024-12-02 13:57:24.049 EST [31268] LOG:  statement: insert into somedata
(serialnumber, firstname, lastname)

*- Ran pg_basebackup*

*- Dropped database*
2024-12-02 14:01:27.243 EST [19148] LOG:  statement: DROP DATABASE
randomdata;

*- Stopped server*
*- Deleted Data folder*
*- Restored from backup file*
*- Added restore_command, recovery_target_time, recovery_target_action to
postgresql.conf*
*- Created reovery.signal file*
*- Restarted server*

2024-12-02 14:04:21.630 EST [20156] LOG:  database system was interrupted;
last known up at 2024-12-02 14:00:22 EST
2024-12-02 14:04:21.630 EST [20156] LOG:  creating missing WAL directory
"pg_wal/archive_status"
2024-12-02 14:04:21.630 EST [20156] LOG:  creating missing WAL directory
"pg_wal/summaries"
2024-12-02 14:04:22.792 EST [20156] LOG:  starting backup recovery with
redo LSN 0/328, checkpoint LSN 0/380, on timeline ID 1
2024-12-02 14:04:22.805 EST [20156] LOG:  restored log file
"00010003" from archive
2024-12-02 14:04:22.818 EST [20156] LOG:  starting point-in-time recovery
to 2024-12-02 13:57:24-05
2024-12-02 14:04:22.825 EST [20156] LOG:  redo starts at 0/328
2024-12-02 14:04:22.826 EST [20156] LOG:  completed backup recovery with
redo LSN 0/328 and end LSN 0/3000120
2024-12-02 14:04:22.826 EST [20156] LOG:  consistent recovery state reached
at 0/3000120
2024-12-02 14:04:22.843 EST [20156] LOG:  restored log file
"00010004" from archive
2024-12-02 14:04:22.871 EST [20156] LOG:  recovery stopping before commit
of transaction 969, time 2024-12-02 14:01:27.281164-05
2024-12-02 14:04:22.871 EST [20156] LOG:  redo done at 0/40006F8 system
usage: CPU: user: 0.00 s, system: 0.00 s, elapsed: 0.04 s
2024-12-02 14:04:22.879 EST [20156] LOG:  selected new timeline ID: 2
2024-12-02 14:04:22.940 EST [20156] LOG:  archive recovery complete
2024-12-02 14:04:22.941 EST [30656] LOG:  checkpoint starting:
end-of-recovery immediate wait
2024-12-02 14:04:22.952 EST [30656] LOG:  checkpoint complete: wrote 4
buffers (0.0%); 0 WAL file(s) added, 0 removed, 1 recycled; write=0.001 s,
sync=0.003 s, total=0.012 s; sync files=3, longest=0.001 s, average=0.001
s; distance=16385 kB, estimate=16385 kB; lsn=0/40006F8, redo lsn=0/40006F8

2024-12-02 14:05:38.142 EST [15876] FATAL:  cannot connect to invalid
database "randomdata"
2024-12-02 14:05:38.142 EST [15876] HINT:  Use DROP DATABASE to drop
invalid databases.

Re: Remove dependence on integer wrapping

[Nathan Bossart]

On Fri, Dec 06, 2024 at 10:22:44AM -0600, Nathan Bossart wrote:
> On Thu, Dec 05, 2024 at 09:58:29PM -0600, Nathan Bossart wrote:
>> Thanks for reviewing.  In v28, I fixed a silly mistake revealed by cfbot's
>> Windows run.  I also went ahead and tried to fix most of the issues
>> reported in a nearby thread [0].  The only one I haven't tracked down yet
>> is the "ISO week" one (case 7).
> 
> The overflow hazard for the ISO week case seems to be in isoweek2j(), which
> would require quite a bit of restructuring to work with the soft-error
> handling from its callers.  I'm not feeling particularly inspired to do
> that, so here's a v29 with a comment added above that function.

I seem to have a knack for picking patches that take an entire afternoon to
back-patch...  Here's what I have staged for commit early next week.

-- 
nathan
>From c97730ef52d99832bf74971d40dba915a93c1017 Mon Sep 17 00:00:00 2001
From: Nathan Bossart 
Date: Fri, 6 Dec 2024 14:07:11 -0600
Subject: [PATCH v30 1/1] Fix various overflow hazards in date and timestamp
 functions.

This commit makes use of the overflow-aware routines in int.h to
fix a variety of reported overflow bugs in the date and timestamp
code.  It seems unlikely that this fixes all such bugs in this
area, but since the problems seem limited to cases that are far
beyond any realistic use-case, I'm not going to worry too much.
Note that for one bug, I've chosen to simply add a comment about
the overflow hazard because fixing it would require quite a bit of
code restructuring that doesn't seem worth the risk.

Reported-by: Alexander Lakhin
Author: Matthew Kim, Nathan Bossart
Reviewed-by: Joseph Koshakow, Jian He
Discussion: https://postgr.es/m/31ad2cd1-db94-bdb3-f91a-65ffdb4bef95%40gmail.com
Discussion: https://postgr.es/m/18585-db646741dd649abd%40postgresql.org
Backpatch-through: 13
---
 src/backend/utils/adt/date.c   |   9 ++-
 src/backend/utils/adt/formatting.c | 104 +++--
 src/backend/utils/adt/timestamp.c  |   4 +
 src/include/common/int.h   |  48 
 src/test/regress/expected/date.out |   2 +
 src/test/regress/expected/horology.out |  16 
 src/test/regress/sql/date.sql  |   1 +
 src/test/regress/sql/horology.sql  |   8 ++
 8 files changed, 183 insertions(+), 9 deletions(-)

diff --git a/src/backend/utils/adt/date.c b/src/backend/utils/adt/date.c
index 8130f3e8ac..da61ac0e86 100644
--- a/src/backend/utils/adt/date.c
+++ b/src/backend/utils/adt/date.c
@@ -256,8 +256,15 @@ make_date(PG_FUNCTION_ARGS)
/* Handle negative years as BC */
if (tm.tm_year < 0)
{
+   int year = tm.tm_year;
+
bc = true;
-   tm.tm_year = -tm.tm_year;
+   if (pg_neg_s32_overflow(year, &year))
+   ereport(ERROR,
+   
(errcode(ERRCODE_DATETIME_FIELD_OVERFLOW),
+errmsg("date field value out of range: 
%d-%02d-%02d",
+   tm.tm_year, tm.tm_mon, 
tm.tm_mday)));
+   tm.tm_year = year;
}
 
dterr = ValidateDate(DTK_DATE_M, false, false, bc, &tm);
diff --git a/src/backend/utils/adt/formatting.c 
b/src/backend/utils/adt/formatting.c
index 2bcc185708..a9daa5c591 100644
--- a/src/backend/utils/adt/formatting.c
+++ b/src/backend/utils/adt/formatting.c
@@ -77,6 +77,7 @@
 
 #include "catalog/pg_collation.h"
 #include "catalog/pg_type.h"
+#include "common/int.h"
 #include "common/unicode_case.h"
 #include "common/unicode_category.h"
 #include "mb/pg_wchar.h"
@@ -3826,7 +3827,14 @@ DCH_from_char(FormatNode *node, const char *in, 
TmFromChar *out,
ereturn(escontext,,

(errcode(ERRCODE_INVALID_DATETIME_FORMAT),
 
errmsg("invalid input string for \"Y,YYY\"")));
-   years += (millennia * 1000);
+
+   /* years += (millennia * 1000); */
+   if (pg_mul_s32_overflow(millennia, 
1000, &millennia) ||
+   pg_add_s32_overflow(years, 
millennia, &years))
+   ereturn(escontext,,
+   
(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+errmsg("value 
for \"Y,YYY\" in source string is out of range")));
+
if (!from_char_set_int(&out->year, 
years, n, escontext))
return;
out->yysz = 4;
@@ -4785,10 +4793,35 @@ do_to_timestamp(text *date_txt, text *fmt, Oid collid, 
bool std,
  

Re: proposal: schema variables

[jian he]

On Thu, Dec 5, 2024 at 2:52 PM Pavel Stehule  wrote:
>
> Hi
>
> only rebase

hi.
disclaimer, i *only* applied
v20241205-0001-Enhancing-catalog-for-support-session-variables-and-.patch.

create variable v2 as text;
alter variable v2 rename to v2;
ERROR:  session variable "v2" already exists in schema "public"

the above is  coverage tests for report_namespace_conflict:
case VariableRelationId:
Assert(OidIsValid(nspOid));
msgfmt = gettext_noop("session variable \"%s\" already
exists in schema \"%s\"");
break;

create type t1 as (a int, b int);
CREATE VARIABLE var1 AS t1;
alter type t1 drop attribute a;
should "alter type t1 drop attribute a;" not allowed?


GetCommandLogLevel also needs to deal with  case T_CreateSessionVarStmt?


there are no regress tests for the change we made in
find_composite_type_dependencies?
It looks like it will be reachable for sure.


CreateVariable, print out error position:
if (get_typtype(typid) == TYPTYPE_PSEUDO)
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
 errmsg("session variable cannot be pseudo-type %s",
format_type_be(typid)),
 parser_errposition(pstate, stmt->typeName->location)));

Alvaro Herrera told me actually, you don't need the extra parentheses
around errcode.
so you can:
if (get_typtype(typid) == TYPTYPE_PSEUDO)
ereport(ERROR,
errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg("session variable cannot be pseudo-type %s",
format_type_be(typid)),
parser_errposition(pstate, stmt->typeName->location))


pg_variable_is_visible seems to have done twice the system cache call.
maybe follow through with the pg_table_is_visible, pg_type_is_visible
code pattern.


IN src/bin/psql/describe.c
+ appendPQExpBufferStr(&buf, "\nWHERE true\n");
this is not needed?

some of the `foreach` can change to foreach_oid, foreach_node
see: 
https://git.postgresql.org/cgit/postgresql.git/commit/?id=14dd0f27d7cd56ffae9ecdbe324965073d01a9ff

doc/src/sgml/ref/create_variable.sgml

CREATE VARIABLE var1 AS date;
LET var1 = current_date;
SELECT var1;


v20241205-0001-Enhancing-catalog-for-support-session-variables-and-.patch
alone cannot do `LET var1 = current_date;`, `SELECT var1;`
maybe the following patch can do it. but that makes
we cannot commit
v20241205-0001-Enhancing-catalog-for-support-session-variables-and-.patch
alone.

since CREATE VARIABLE is an extension to standard, so in create_schema.sgml
Compatibility section,
do we need to mention CREATE SCHEMA CREATE VARIABLE is an extension
from standard
?
---

/*
 * Drop variable by OID, and register the needed session variable
 * cleanup.
 */
void
DropVariableById(Oid varid)
i am not sure of the meaning of "register the needed session variable cleanup".