[patch] Make "invalid record length at : expected at least 24, got 0" message less scary

[Michael Banck]

Hi,

we had another case on irc today were a user saw Postgres doing crash
recovery due to an unclean shutdown and was very worried about a
"invalid record length at : expected at least 24, got 0" message
and went on to reindex all databases. This is also a frequent hit on
Stack Overflow etc.

AFAICT this is a normal message in case the record length is 0 and we
have reached end of WAL. So I propose to treat a length of 0 as a
special case and emit a less-scary message. Up until 9.4 we did have a
message "record with zero length at ", but 2c03216d831 ("Revamp the
WAL record format") removed it. I propose to reinstate it, see attached
patch.

I guess it would be even nicer if we could hint here that we likely
reached end-of-WAL, but the helper function report_invalid_record() does
not take an errhint and I guess we are too deep into the WAL reader
machinery to check for an end-of-WAL condition at that spot.


Michael
>From 015a9bfb2bd57f37f7c9601e9014d7560b76c21d Mon Sep 17 00:00:00 2001
From: Michael Banck 
Date: Sat, 30 Nov 2024 11:51:34 +0100
Subject: [PATCH v1] Re-introduce less scary message for possible end-of-WAL
 invalid record.

A lot of users are worried about messages like "invalid record length at
: expected at least 24, got 0" even though for the case of "got 0"
this usually just means that we have reached the end of WAL. So make
that case less scary by re-introducing a "record with zero length at
" message for it. This message was there up until version 9.4, but
got removed during a large WAL record format revamp in 2c03216d831.
---
 src/backend/access/transam/xlogreader.c | 15 +++
 1 file changed, 15 insertions(+)

diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
index 0c5e040a94..43ec295ee7 100644
--- a/src/backend/access/transam/xlogreader.c
+++ b/src/backend/access/transam/xlogreader.c
@@ -662,6 +662,14 @@ restart:
 	}
 	else
 	{
+		/* Record length is zero. */
+		if (total_len == 0)
+		{
+			report_invalid_record(state,
+  "record with zero length at %X/%X",
+  LSN_FORMAT_ARGS(RecPtr));
+			goto err;
+		}
 		/* There may be no next page if it's too small. */
 		if (total_len < SizeOfXLogRecord)
 		{
@@ -1128,6 +1136,13 @@ ValidXLogRecordHeader(XLogReaderState *state, XLogRecPtr RecPtr,
 	  XLogRecPtr PrevRecPtr, XLogRecord *record,
 	  bool randAccess)
 {
+	if (record->xl_tot_len == 0)
+	{
+		report_invalid_record(state,
+			  "record with zero length at %X/%X",
+			  LSN_FORMAT_ARGS(RecPtr));
+		return false;
+	}
 	if (record->xl_tot_len < SizeOfXLogRecord)
 	{
 		report_invalid_record(state,
-- 
2.39.5

Re: Unclear code - please elaborate

[Tom Lane]

Dmitry Nikitin  writes:
>   
> https://github.com/postgres/postgres/blob/5d39becf8ba0080c98fee4b63575552f6800b012/src/backend/optimizer/prep/prepjointree.c#L3856
> bms_next_member() is allowed to return the zero as a valid value. Subsequent 
> rt_fetch() offsets that
> zero to -1 which leads to the assertion down the code. Nothing wrong here? 
> Either zero is simply not
> possible after that bms_next_member() because of some factors behind the code?

Zero isn't a valid relid.  If we were to find such a value in that
bitmapset, an assertion would be a fine outcome.

regards, tom lane

Re: Unclear code - please elaborate

[Junwang Zhao]

On Sat, Nov 30, 2024 at 4:15 PM Dmitry Nikitin
 wrote:
>
> Hello ,
>
>   
> https://github.com/postgres/postgres/blob/5d39becf8ba0080c98fee4b63575552f6800b012/src/backend/optimizer/prep/prepjointree.c#L3856
> bms_next_member() is allowed to return the zero as a valid value. Subsequent 
> rt_fetch() offsets that
> zero to -1 which leads to the assertion down the code. Nothing wrong here? 
> Either zero is simply not
> possible after that bms_next_member() because of some factors behind the code?
>

rtindex 0 is not used, see the logic of setup_simple_rel_arrays and the comments
of PlannerInfo.simple_rel_array.

/*
* simple_rel_array holds pointers to "base rels" and "other rels" (see
* comments for RelOptInfo for more info). It is indexed by rangetable
* index (so entry 0 is always wasted). Entries can be NULL when an RTE
* does not correspond to a base relation, such as a join RTE or an
* unreferenced view RTE; or if the RelOptInfo hasn't been made yet.
*/

>
> --
> Best regards,
>  Dmitry  mailto:pgsql-hack...@dima.nikitin.name
>
>
>


-- 
Regards
Junwang Zhao

speedup ALTER TABLE ADD CHECK CONSTRAINT.

[jian he]

hi.
attached  patch trying to speedup ALTER TABLE ADD CHECK CONSTRAINT.
demo:

drop table if exists t7;
create table t7 (a int not  null, b int, c int);
insert into t7 select g, g+1, g %100 from generate_series(1,1_000_000) g;
create index on t7(a,b);

alter table t7 add check (a > 0);
patch: Time: 4.281 ms
master: Time: 491.689 ms

implementation:

step1.  during execute command `ALTER TABLE ADD CHECK CONSTRAINT`
in AddRelationNewConstraints->StoreRelCheck
after StoreRelCheck add a CommandCounterIncrement();
so previously added CHECK pg_constraint can be seen by us.
we need to use pg_get_constraintdef to retrieve the CHECK constraint definition.

step2. check if this relation has any suitable index (not expression
index, not predicate index)
--whole patch hinges on SPI query can use indexscan to quickly
retrieve certain information.

step3: construct and execute these three SPI query:
("set enable_seqscan to off")
(SELECT 1 FROM the_table WHERE NOT (check_constraint_def)
AND check_constraint_def IS NOT NULL LIMIT 1)
("reset enable_seqscan")

the SPI query will be faster, if the qual(check_constraint_def) can be
utilized by indexscan.
if SPI_processed == 0 means we toggle this check constraint as
"already_validated" (bool) is true.
we stored already_validated in CookedConstraint. later pass it to
AlteredTableInfo->constraints.
then phrase 3 within ATRewriteTable, we can do
```
if (constraint->already_validated)
needscan = false;
```


concern:
only certain kinds of check constraint expressions can be used for
this optimization.
i do all the CHECK constraint expressions filter in checkconstraint_expr_walker.

use contain_volatile_functions to filter out volatile expressions,
add (check_constraint_def IS NOT NULL) in the above SPI query, i think
null handling is correct?

ALTER TABLE ADD CHECK CONSTRAINT is using ACCESS EXCLUSIVE lock.
but i need to think more about concurrently related issues.

idea come from this thread:
https://postgr.es/m/canwftzk2mz7js_56v+zclxzyh1utbzx4ueg03p7cee86k2r...@mail.gmail.com
From 5a4039d058d34467696fd6e6238fdb1132d9af14 Mon Sep 17 00:00:00 2001
From: jian he 
Date: Sat, 30 Nov 2024 19:20:22 +0800
Subject: [PATCH v1 1/1] speedup alter table add check constraint

dicussion: https://postgr.es/m/
---
 src/backend/catalog/heap.c| 218 ++
 src/backend/catalog/index.c   |  74 
 src/backend/catalog/pg_constraint.c   |   1 +
 src/backend/commands/tablecmds.c  |   9 +-
 src/include/catalog/heap.h|   1 +
 src/include/catalog/index.h   |   1 +
 src/test/regress/expected/constraints.out |  20 ++
 src/test/regress/sql/constraints.sql  |  22 +++
 8 files changed, 345 insertions(+), 1 deletion(-)

diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 003af4bf21..5eb3450208 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -57,6 +57,7 @@
 #include "commands/tablecmds.h"
 #include "commands/typecmds.h"
 #include "common/int.h"
+#include "executor/spi.h"
 #include "miscadmin.h"
 #include "nodes/nodeFuncs.h"
 #include "optimizer/optimizer.h"
@@ -115,6 +116,17 @@ static Node *cookConstraint(ParseState *pstate,
 			Node *raw_constraint,
 			char *relname);
 
+typedef struct checkexpr_context
+{
+	Bitmapset  *bms_indexattno;
+	bool		cannot_be_indexed;
+} checkexpr_context;
+static bool
+checkconstraint_expr_walker(Node *node, checkexpr_context *context);
+static bool
+index_validate_check_constraint(Oid constrOid, Relation rel,
+Node *expr,
+const char *constrname);
 
 /* 
  *XXX UGLY HARD CODED BADNESS FOLLOWS XXX
@@ -2409,6 +2421,7 @@ AddRelationNewConstraints(Relation rel,
 		cooked->is_local = is_local;
 		cooked->inhcount = is_local ? 0 : 1;
 		cooked->is_no_inherit = false;
+		cooked->already_validated = false;
 		cookedConstraints = lappend(cookedConstraints, cooked);
 	}
 
@@ -2527,6 +2540,8 @@ AddRelationNewConstraints(Relation rel,
 StoreRelCheck(rel, ccname, expr, cdef->initially_valid, is_local,
 			  is_local ? 0 : 1, cdef->is_no_inherit, is_internal);
 
+			/* we need this for index_validate_check_constraint*/
+			CommandCounterIncrement();
 			numchecks++;
 
 			cooked = (CookedConstraint *) palloc(sizeof(CookedConstraint));
@@ -2539,6 +2554,13 @@ AddRelationNewConstraints(Relation rel,
 			cooked->is_local = is_local;
 			cooked->inhcount = is_local ? 0 : 1;
 			cooked->is_no_inherit = cdef->is_no_inherit;
+			/* internal, readd check constraint cannot be prevalidated */
+			if (is_internal)
+cooked->already_validated = false;
+			else
+cooked->already_validated =
+	index_validate_check_constraint(constrOid, rel, expr, ccname);
+
 			cookedConstraints = lappend(cookedConstraints, cooked);
 		}
 		else if (cdef->contype == CONSTR_NOTNULL)
@@ -2609,6 +2631,7 @@ AddRelatio

Unclear code - please elaborate

[Dmitry Nikitin]

Hello ,

  
https://github.com/postgres/postgres/blob/5d39becf8ba0080c98fee4b63575552f6800b012/src/backend/optimizer/prep/prepjointree.c#L3856
bms_next_member() is allowed to return the zero as a valid value. Subsequent 
rt_fetch() offsets that
zero to -1 which leads to the assertion down the code. Nothing wrong here? 
Either zero is simply not
possible after that bms_next_member() because of some factors behind the code?


-- 
Best regards,
 Dmitry  mailto:pgsql-hack...@dima.nikitin.name

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

[Alvaro Herrera]

On 2024-Nov-30, Michael Paquier wrote:

> After sleeping on that, and because the leak is minimal, I'm thinking
> about just applying the fix only on HEAD and call it a day.  This
> changes the structure of Publication so as we use a char[NAMEDATALEN]
> rather than a char*, avoiding the pstrdup(), for the publication name
> and free it in the list_free_deep() when reloading the list of
> publications.

I'm not sure about your proposed fix.  Isn't it simpler to have another
memory context which we can reset instead of doing list_free_deep()?  It
doesn't have to be a global memory context -- since this is not
reentrant and not referenced anywhere else, it can be a simple static
variable in that block, as in the attached.  I ran the stock tests (no
sysbench) and at least it doesn't crash.

This should be easily backpatchable also, since there's no ABI change.

-- 
Álvaro HerreraBreisgau, Deutschland  —  https://www.EnterpriseDB.com/
>From 3ed287180a007447bbee152177139cf1d4347625 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=81lvaro=20Herrera?= 
Date: Sat, 30 Nov 2024 09:02:10 +0100
Subject: [PATCH] untested try at fixing memleak

---
 src/backend/replication/pgoutput/pgoutput.c | 16 ++--
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/src/backend/replication/pgoutput/pgoutput.c b/src/backend/replication/pgoutput/pgoutput.c
index 5e23453f071..010b341e0f5 100644
--- a/src/backend/replication/pgoutput/pgoutput.c
+++ b/src/backend/replication/pgoutput/pgoutput.c
@@ -2063,12 +2063,16 @@ get_rel_sync_entry(PGOutputData *data, Relation relation)
 		/* Reload publications if needed before use. */
 		if (!publications_valid)
 		{
-			oldctx = MemoryContextSwitchTo(CacheMemoryContext);
-			if (data->publications)
-			{
-list_free_deep(data->publications);
-data->publications = NIL;
-			}
+			static MemoryContext pubmemcxt = NULL;
+
+			if (pubmemcxt == NULL)
+pubmemcxt = AllocSetContextCreate(CacheMemoryContext,
+  "publication list context",
+  ALLOCSET_DEFAULT_SIZES);
+			else
+MemoryContextReset(pubmemcxt);
+			oldctx = MemoryContextSwitchTo(pubmemcxt);
+
 			data->publications = LoadPublications(data->publication_names);
 			MemoryContextSwitchTo(oldctx);
 			publications_valid = true;
-- 
2.39.5

Re: Truncate logs by max_log_size

[Kirill Gavrilov]

>
>
> Hi
>
> > +for (my $attempts = 0; $attempts < $max_attempts; $attempts++)
> > +{
> > + eval {
> > + $current_logfiles = slurp_file($node->data_dir . '/current_logfiles');
> > + };
> > + last unless $@;
> > + usleep(100_000);
> > +}
>
>
> `usleep` in tap tests is usually a bad pattern. Do we have a chance to
> test this using `wait_for_log` or similar?
>

I'm not sure we can use `wait_for_log` because it checks for only one
logfile. But even if it's possible, I don't think it's a good idea to use
different checks in the same file or to change tests for another feature. I
used test case from above as an example for mine.

Re: UUID v7

[Daniel Verite]

Andrey M. Borodin wrote:


> I'm sending amendments addressing your review as a separate step in patch
> set. Step 1 of this patch set is identical to v39.

Some comments about the implementation of monotonicity:

+/*
+ * Get the current timestamp with nanosecond precision for UUID generation.
+ * The returned timestamp is ensured to be at least SUBMS_MINIMAL_STEP
greater
+ * than the previous returned timestamp (on this backend).
+ */
+static inline int64
+get_real_time_ns_ascending()
+{
+   static int64 previous_ns = 0;

[...]

+   /* Guarantee the minimal step advancement of the timestamp */
+   if (previous_ns + SUBMS_MINIMAL_STEP_NS >= ns)
+   ns = previous_ns + SUBMS_MINIMAL_STEP_NS;
+   previous_ns = ns;

In the case of parallel execution (uuidv7() being parallel-safe), if
there have been previous calls to uuidv7() in that backend,
previous_ns will be set in the backend process,
but zero in a newly spawned worker process.
If (previous_ns + SUBMS_MINIMAL_STEP_NS >= ns) ever happens
to be true in the main process, it will start at false in the workers,
leading to non-monotonic results within the same query.


Also in the case of a backward clock change, we can end up with some
backends sticking to the "old time" plus increment per invocation
until they die, while some other backends spawned after the clock
change are on the "new time". These backends may produce series of
UUIDv7 that would be completely out of sync with each others.
A backward clock change is an abnormality, but if it occurs, what's
the best choice? Take the bullet and switch to the new time , or
stick to a time that is permanently decorrelated from the OS
clock? I would think that the latter is worse.


Best regards,
-- 
Daniel Vérité
https://postgresql.verite.pro/
Twitter: @DanielVerite

Re: how to get MAJORVERSION in meson

[jian he]

On Fri, Nov 29, 2024 at 7:09 PM Pavel Stehule  wrote:
>
>
>
> pá 29. 11. 2024 v 10:42 odesílatel Pavel Stehule  
> napsal:
>>
>> Hi
>>
>> I tried to add meson support to plpgsql_check. As template I used 
>> https://raw.githubusercontent.com/petere/plsh/refs/heads/meson/meson.build
>>
>> Unfortunately, for tests I need to know MAJORVERSION. In makefile I use
>>
>> ifndef MAJORVERSION
>> MAJORVERSION := $(basename $(VERSION))
>> endif
>>
>> Is there some similar pattern for meson?
>>
>

> at the end I wrote (based on Postgres's meson build).
>
> https://github.com/okbob/plpgsql_check/blob/master/meson.build
>
> Is there some other solution?
>

google around, then i found out this
https://github.com/mesonbuild/meson/issues/3535

Re: [PATCH] Add sortsupport for range types and btree_gist

[Bernd Helmle]

Hi Michael,

Am Freitag, dem 29.11.2024 um 13:42 +0900 schrieb Michael Paquier:


 [...]

> Any module that requires the module injection_points to be installed
> can do a few things, none of them are done this way in this patch:
> - Add EXTRA_INSTALL=src/test/modules/injection_points.
> - You could make a test able to run installcheck, but you should use
> an extra query that checks if the module is available for
> installation
> by querying pg_available_extensions and use two possible output
> files:
> one for the case where the module is *not* installed and one for the
> case where the module is installed.  A simpler way would be to block
> installcheck, or add SQL tests in src/test/modules/injection_points.
> Both options don't seem adapted to me here as they impact the
> portability of existing tests.
> 

I don't like this. This smells like we use the wrong tool. Andrey had
the idea to use them because it looked as a compelling idea to check
whether sortsupport is used in the code path or not. 

Maybe we should just use a specific DEBUG message and make sure it's
handled by the tests accordingly.

> As a whole, I'm very dubious about the need for injection points at
> all here.  The sortsupport property claimed for this patch tells that
> this results in smaller index sizes, but the tests don't really check
> that: they just make sure that sortsupport routine paths are taken.
> What this should test is not the path taken, but how the new code
> affects the index data generated.  Perhaps pageinspect could be
> something to use to show the difference in contents, not sure though.
> 

No, this isn't the goal of sortsupport for btree_gist. It was a side
effect before PG15 that it yielded a better index quality, but the
primary goal is and was index creation speed. With sortsupport we're
much much faster that the traditional buffered method.

Originally this patch was pushed by a customer who had complains about
the long build times of btree_gist indexes used by exclusion
constraints they're heavily relying on. See a benchmark i've posted in
this thread here with test data:

https://www.postgresql.org/message-id/2f2e88bf1d44d06c1baf27bbf3db880f42a5cb87.ca...@oopsware.de

> The number of tests added to contrib/btree_gist/Makefile is not
> acceptable for a patch of this size, leading to a large bloat.  And
> that's harder to maintain in the long-term.

Yes, it is not really nice, but i have no better idea atm on how to
test both code paths equally without repeating the tests again. Index
creation must yield the same results than buffered index build
strategy. Originally i just added the tests to the same test file, but
then decided to split them up to make it simpler to review and
maintain.

I'm open for better ideas.

Thanks for your comments

Bernd

Re: how to get MAJORVERSION in meson

[Pavel Stehule]

so 30. 11. 2024 v 16:42 odesílatel jian he 
napsal:

> On Fri, Nov 29, 2024 at 7:09 PM Pavel Stehule 
> wrote:
> >
> >
> >
> > pá 29. 11. 2024 v 10:42 odesílatel Pavel Stehule <
> pavel.steh...@gmail.com> napsal:
> >>
> >> Hi
> >>
> >> I tried to add meson support to plpgsql_check. As template I used
> https://raw.githubusercontent.com/petere/plsh/refs/heads/meson/meson.build
> >>
> >> Unfortunately, for tests I need to know MAJORVERSION. In makefile I use
> >>
> >> ifndef MAJORVERSION
> >> MAJORVERSION := $(basename $(VERSION))
> >> endif
> >>
> >> Is there some similar pattern for meson?
> >>
> >
>
> > at the end I wrote (based on Postgres's meson build).
> >
> > https://github.com/okbob/plpgsql_check/blob/master/meson.build
> >
> > Is there some other solution?
> >
>
> google around, then i found out this
> https://github.com/mesonbuild/meson/issues/3535


Unfortunately this is not available for extensions - the extensions are
independent projects and take any information from pg_config.

Re: CREATE SCHEMA ... CREATE DOMAIN support

[Kirill Reshke]

On Sun, 1 Dec 2024 at 04:33, Tom Lane  wrote:
> I looked at several iterations of the SQL standard
> and cannot find any support for the idea that CREATE SCHEMA needs to
> be any smarter than that.  I'd also argue that doing anything else is
> a POLA violation.  It's especially a POLA violation if the code
> rearranges a valid user-written command order into an invalid order,
> which is inevitable if we stick with the current approach.

Agreed.

> So attached is a draft patch that simplifies the rule to "do the
> subcommands in the order written".

+1 on this idea.

Here is my 2c to this draft:
1) Report error position on newly added check for temp relation in
non-temp schema.

> + errmsg("cannot create temporary relation in non-temporary 
> schema"),
> + parser_errposition(pstate, relation->location)));

2)
I added some regression tests that might be worth adding:
a) check for a temporary table created within a non-temporary schema
in create_table.sql, akin to existing check in create_view.sql.
b) Also explicitly check that old-style sql creation does not work.
That is, for example,
CREATE SCHEMA test_ns_schema_3
   CREATE VIEW abcd_view AS
  SELECT a FROM abcd
   CREATE TABLE abcd (
  a serial
   );

fails.

3) Why do we delete this in `create_schema.sgml`? Is this untrue? It
is about order of definition, not creation, isn't it?

> -   The SQL standard specifies that the subcommands in CREATE
> -   SCHEMA can appear in any order.


P.S.
This section in SQL-92 is the only information I could find about
order of creation.

```
 3) Those objects defined by s (base tables, views,
constraints, domains, assertions, character sets, translations,
collations, privileges) and their associated descriptors are
effectively created.
```
Look like we are 100% to do it in order of definition


-- 
Best regards,
Kirill Reshke


v2-0001-Don-t-try-to-re-order-the-subcommands-of-CREATE-S.patch
Description: Binary data

Re: CREATE SCHEMA ... CREATE DOMAIN support

[Tom Lane]

Kirill Reshke  writes:
> 3) Why do we delete this in `create_schema.sgml`? Is this untrue? It
> is about order of definition, not creation, isn't it?

>> -   The SQL standard specifies that the subcommands in CREATE
>> -   SCHEMA can appear in any order.

In context with the following sentence, what that is really trying
to say is that the spec requires us to re-order the subcommands
to eliminate forward references.  After studying the text I cannot
find any such statement.  Maybe I missed something --- there's a
lot of text --- but it's sure not to be detected in any obvious
place like 11.1 .

(I'd be curious to know how other major implementations handle
this.  Are we the only implementation that ever read the spec
that way?)

regards, tom lane

Re: [PATCH] Add sortsupport for range types and btree_gist

[Andrey Borodin]

> On 30 Nov 2024, at 18:14, Bernd Helmle  wrote:
> 
> I don't like this. This smells like we use the wrong tool. Andrey had
> the idea to use them because it looked as a compelling idea to check
> whether sortsupport is used in the code path or not. 
> 
> Maybe we should just use a specific DEBUG message and make sure it's
> handled by the tests accordingly.

We tried to go that route, random DEBUG1 lines were breaking tests (autovacuum 
or something like that, there was something non-deterministic). I think we can 
dig exact reason from 2021 thread why we abandoned that idea...

I think we have two options:
1. Just do not commit tests. We ran it manually, saw that paths are taken, we 
are happy.
2. Have just one file that builds sorted index on a table with few tuples for 
each data type.

We do not need to test that core sorting (or buffered) build works. AFAIR 
there's plenty of other tests to verify that.

Injection points seemed to me exactly the technogy that could help us to have 
tests for this patch. But at this point It looks like it's reasonable to take 
approach 1, as we did before.


Best regards, Andrey Borodin.

Re: Pass ParseState as down to utility functions.

[jian he]

hi.

ATExecAddOf
DefineType
ATPrepAlterColumnType
ATExecAlterColumnType
DefineDomain
AlterType
i changed the above function, so the above related function errors may
print out error position.
reason for change mainly because these functions have
`typenameType(NULL, typeName, &targettypmod);`
we want to pass not NULL pstate (typenameType(pstate, typeName, &targettypmod);)

why do we want printout error position
1. it can quickly locate DDL command error positions, beginner friendly.
2. in the thread `CREATE SCHEMA ... CREATE DOMAIN support`, case like:
  CREATE SCHEMA regress_schema_2
  create domain ss1 as ss
  create domain ss as text;
ERROR:  type "ss" does not exist
obviously the error is not helpful at all.

As you can see, in cases like a single DDL, multiple sub DDL within
it, error position is quite important
I also added some tests for DefineDomain.
added parser_errposition for many places in DefineDomain.

the attached patch (based on Kirill Reshke 's v2 patch)
either passing the source_string to the existing ParseState
or by making a new ParseState passing source_string to it.
then add
`parser_errposition(pstate, location)))`
in various places optionally.

So I guess bundling it into a single patch should be fine?
From 181b362c66a124721c56ddd461e44158283e6548 Mon Sep 17 00:00:00 2001
From: jian he 
Date: Sat, 30 Nov 2024 14:44:38 +0800
Subject: [PATCH v3 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

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.
---
 src/backend/commands/tablecmds.c  | 43 +---
 src/backend/commands/typecmds.c   | 49 +--
 src/backend/parser/parse_utilcmd.c|  2 +-
 src/backend/tcop/utility.c|  4 +-
 src/include/commands/typecmds.h   |  4 +-
 src/test/regress/expected/alter_table.out | 13 ++
 src/test/regress/expected/domain.out  | 17 
 src/test/regress/sql/alter_table.sql  |  5 +++
 src/test/regress/sql/domain.sql   |  5 +++
 9 files changed, 102 insertions(+), 40 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,
 			address = ATExecDropInherit(rel, (RangeVar *) cmd->def, lockmode);
 			break;
 		case AT_AddOf:
-			address = ATExecAddOf(rel, (TypeName *) cmd->def, lockmode);
+			address = ATExecAddOf(rel, (TypeName *) cmd->def, lockmode, context);
 			break;
 		case AT_DropOf:
 			ATExecDropOf(rel, lockmode);
@@ -13218,10 +13221,12 @@ ATPrepAl

Re: CREATE SCHEMA ... CREATE DOMAIN support

[Tom Lane]

[ Looping in Peter E. for commentary on SQL-spec compatibility ]

I spent some time looking at this patch, and came away with
two main thoughts:

1. It doesn't make any sense to me to support CREATE DOMAIN within
CREATE SCHEMA but not any of our other commands for creating types.
It's not a consistent feature this way, and there's no support for
it in the SQL standard either, because the spec calls out both
 and  as permissible
schema elements.  So I think we need a bit more ambition in the scope
of the patch: it should allow every variant of CREATE TYPE too.
(Since the spec also lists , I'd personally like to
see us cover functions/procedures as well as types.  But functions
could be left for later I guess.)

2. transformCreateSchemaStmtElements is of the opinion that it's
responsible for ordering the schema elements in a way that will work,
but it just about completely fails at that task.  Ordering the objects
by kind is surely not sufficient, and adding CREATE DOMAIN will make
that worse.  (Example: a domain could be used in a table definition,
but we also allow domains to be created over tables' composite types.)
Yet we have no infrastructure that would allow us to discover the real
dependencies between unparsed DDL commands, nor is it likely that
anyone will ever undertake building such.  I think we ought to nuke
that concept from orbit and just execute the schema elements in the
order presented.  I looked at several iterations of the SQL standard
and cannot find any support for the idea that CREATE SCHEMA needs to
be any smarter than that.  I'd also argue that doing anything else is
a POLA violation.  It's especially a POLA violation if the code
rearranges a valid user-written command order into an invalid order,
which is inevitable if we stick with the current approach.

The notion that we ought to sort the objects by kind appears to go
all the way back to 95ef6a344 of 2002-03-21, which I guess makes it
my fault.  There must have been some prior mailing list discussion,
but I couldn't find much.  There is a predecessor of the committed
patch in
https://www.postgresql.org/message-id/flat/3C7F8A49.CC4EF0BE%40redhat.com
but no discussion of why sorting by kind is a good idea.  (The last
message in the thread suggests that there was more discussion among
the Red Hat RHDB team, but if so it's lost to history now.)

Thoughts?

regards, tom lane

Re: Accessing other session's temp table

[Mikhail Gribkov]

> Recently I saw a report and proposed fix here [0]. I did not dig into,
just connection internets. Thanks!

Oh, I missed it. Then I'll consider my issue closed and continue discussion
in the older thread if there is something to discuss.
Thanks a lot!

--
 best regards,
Mikhail A. Gribkov

e-mail: youzh...@gmail.com
*http://www.flickr.com/photos/youzhick/albums
*
http://www.strava.com/athletes/5085772
phone: +7(916)604-71-12
Telegram: @youzhick

Re: CREATE SCHEMA ... CREATE DOMAIN support

[Tom Lane]

I wrote:
> 2. transformCreateSchemaStmtElements is of the opinion that it's
> responsible for ordering the schema elements in a way that will work,
> but it just about completely fails at that task.  Ordering the objects
> by kind is surely not sufficient, and adding CREATE DOMAIN will make
> that worse.  (Example: a domain could be used in a table definition,
> but we also allow domains to be created over tables' composite types.)
> Yet we have no infrastructure that would allow us to discover the real
> dependencies between unparsed DDL commands, nor is it likely that
> anyone will ever undertake building such.  I think we ought to nuke
> that concept from orbit and just execute the schema elements in the
> order presented.  I looked at several iterations of the SQL standard
> and cannot find any support for the idea that CREATE SCHEMA needs to
> be any smarter than that.  I'd also argue that doing anything else is
> a POLA violation.  It's especially a POLA violation if the code
> rearranges a valid user-written command order into an invalid order,
> which is inevitable if we stick with the current approach.

Further to this: I don't think "re-order into a safe order" is even
a well-defined requirement.  What should happen with

CREATE VIEW public.v1 AS SELECT * FROM foo;

CREATE SCHEMA s1

CREATE VIEW v0 AS SELECT * FROM v1;

CREATE VIEW v1 AS SELECT * FROM bar;

If we re-order the CREATE VIEW subcommands, this means something
different than if we don't.  Maybe the user meant us to re-order,
but it's hardly an open-and-shut argument.  And historically we
have not re-ordered CREATE VIEW subcommands, so there's a hazard
of compatibility problems if we did ever try to do that.

So attached is a draft patch that simplifies the rule to "do the
subcommands in the order written".  I took the opportunity to clean up
some other small infelicities about transformCreateSchemaStmtElements,
too.

regards, tom lane

From 14e47a5b64fb115928630613d9ab0ae2d6e05eec Mon Sep 17 00:00:00 2001
From: Tom Lane 
Date: Sat, 30 Nov 2024 18:11:04 -0500
Subject: [PATCH v1] Don't try to re-order the subcommands of CREATE SCHEMA.

transformCreateSchemaStmtElements has always believed that it is
supposed to re-order the subcommands of CREATE SCHEMA into a safe
execution order.  However, it is nowhere near being capable of doing
that correctly.  Nor is there reason to think that it ever will be,
or that that is a well-defined requirement, or that there's any basis
in the SQL standard for it.  Moreover, the problem will get worse as
we add more subcommand types.  Let's just drop the whole idea and
execute the commands in the order given, which seems like a much less
astonishment-prone definition anyway.

Along the way, pass down a ParseState so that we can provide an
error cursor for the "wrong schema name" error, and fix
transformCreateSchemaStmtElements so that it doesn't scribble
on the parsetree passed to it.

Discussion: https://postgr.es/m/1075425.1732993...@sss.pgh.pa.us
---
 doc/src/sgml/ref/create_schema.sgml |  10 +-
 src/backend/commands/extension.c|   9 +-
 src/backend/commands/schemacmds.c   |  22 ++--
 src/backend/parser/parse_utilcmd.c  | 130 
 src/backend/tcop/utility.c  |   7 +-
 src/include/commands/schemacmds.h   |   7 +-
 src/include/parser/parse_utilcmd.h  |   3 +-
 src/test/regress/expected/create_schema.out |  30 +
 src/test/regress/expected/event_trigger.out |   2 +-
 src/test/regress/expected/namespace.out |   9 +-
 src/test/regress/sql/namespace.sql  |  12 +-
 11 files changed, 123 insertions(+), 118 deletions(-)

diff --git a/doc/src/sgml/ref/create_schema.sgml b/doc/src/sgml/ref/create_schema.sgml
index ed69298ccc..625793a6b6 100644
--- a/doc/src/sgml/ref/create_schema.sgml
+++ b/doc/src/sgml/ref/create_schema.sgml
@@ -193,12 +193,10 @@ CREATE VIEW hollywood.winners AS
   
 
   
-   The SQL standard specifies that the subcommands in CREATE
-   SCHEMA can appear in any order.  The present
-   PostgreSQL implementation does not
-   handle all cases of forward references in subcommands; it might
-   sometimes be necessary to reorder the subcommands in order to avoid
-   forward references.
+   PostgreSQL executes the subcommands
+   in CREATE SCHEMA in the order given.  Other
+   implementations may try to rearrange the subcommands into dependency
+   order, but that is hard if not impossible to do correctly.
   
 
   
diff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.c
index af6bd8ff42..a2eb42dc7f 100644
--- a/src/backend/commands/extension.c
+++ b/src/backend/commands/extension.c
@@ -1687,14 +1687,19 @@ CreateExtensionInternal(char *extensionName,
 
 		if (!OidIsValid(schemaOid))
 		{
+			ParseState *pstate = make_parsestate(NULL);
 			CreateSchemaStmt *csstmt = makeNode(CreateSchemaStmt);
 
+			pstate->p_sourcetext = "(generated C