Re: create role manual

[David G. Johnston]

On Sat, Jun 15, 2024 at 7:25 PM Tatsuo Ishii  wrote:

>The rules for which initial
>role membership options are enabled described below in the
>IN ROLE, ROLE, and
>ADMIN clauses.
>
> Maybe we need "are" in front of "described"?
>
>
Agreed.

David J.

create role manual

[Tatsuo Ishii]

While my colleague is working on translating
doc/src/sgml/ref/create_role.sgml into Japanese, he has found
following sentence is hard to parse:

   The rules for which initial
   role membership options are enabled described below in the
   IN ROLE, ROLE, and
   ADMIN clauses.

Maybe we need "are" in front of "described"?

Attached is the patch for that.

Best reagards,
--
Tatsuo Ishii
SRA OSS LLC
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
diff --git a/doc/src/sgml/ref/create_role.sgml b/doc/src/sgml/ref/create_role.sgml
index 27e48da12a..f72ba9affc 100644
--- a/doc/src/sgml/ref/create_role.sgml
+++ b/doc/src/sgml/ref/create_role.sgml
@@ -71,7 +71,7 @@ in sync when changing the above synopsis!
During role creation it is possible to immediately assign the newly created
role to be a member of an existing role, and also assign existing roles
to be members of the newly created role.  The rules for which initial
-   role membership options are enabled described below in the
+   role membership options are enabled are described below in the
IN ROLE, ROLE, and
ADMIN clauses.  The 
command has fine-grained option control during membership creation,

Re: race condition in pg_class

[Michael Paquier]

On Thu, Jun 13, 2024 at 07:42:25PM -0700, Noah Misch wrote:
> On Fri, Jun 14, 2024 at 09:58:59AM +0900, Michael Paquier wrote:
>> GetWaitEventCustomIdentifier() is incorrect, and should return
>> "InjectionPoint" in the default case of this class name, no?
> 
> I intentionally didn't provide a default event ID for InjectionPoint.
> PG_WAIT_EXTENSION needs a default case for backward compatibility, if nothing
> else.  For this second custom type, it's needless complexity.  The value
> 0x0B00U won't just show up like PG_WAIT_EXTENSION does.
> GetLWLockIdentifier() also has no default case.  How do you see it?

I would add a default for consistency as this is just a few extra
lines, but if you feel strongly about that, I'm OK as well.  It makes
a bit easier the detection of incorrect wait event numbers set
incorrectly in extensions depending on the class wanted.

> The patch added to xfunc.sgml an example of using it.  I'd be more inclined to
> delete the WaitEventExtensionNew() docbook documentation than to add its level
> of detail for WaitEventInjectionPointNew().  We don't have that kind of
> documentation for most extension-facing C functions.

It's one of the areas where I think that we should have more
documentation, not less of it, so I'd rather keep it and maintaining
it is not really a pain (?).  The backend gets complicated enough
these days that limiting what developers have to guess on their own is
a better long-term approach because the Postgres out-of-core ecosystem
is expanding a lot (aka have also in-core documentation for hooks,
even if there's been a lot of reluctance historically about having
them).
--
Michael


signature.asc
Description: PGP signature

IPC::Run accepts bug reports

[Noah Misch]

Separating this from the pytest thread:

On Sat, Jun 15, 2024 at 01:26:57PM -0400, Robert Haas wrote:
> The one
> thing I know about that *I* think is a pretty big problem about Perl
> is that IPC::Run is not really maintained.

I don't see in https://github.com/cpan-authors/IPC-Run/issues anything
affecting PostgreSQL.  If you know of IPC::Run defects, please report them.
If I knew of an IPC::Run defect affecting PostgreSQL, I likely would work on
it before absurdity like https://github.com/cpan-authors/IPC-Run/issues/175
NetBSD-10-specific behavior coping.

Backup and Restore of Partitioned Table in PG-15

[Gayatri Singh]

Hi Team,

Greetings of the day!!

We are planning to partition tables using pg_partman. Like we are planning
for their backup and restoration process.

Got a few URLs where pg_dump had issues while restoring some data that was
lost.

kindly guide me the process or steps I need to follow for backing up
partitioned tables correctly so that while restoration I don't face any
issue.

Another question, currently we are using pg_dump for database backup which
locks tables and completely puts db transactions on hold. For this I want
tables shouldnt get locked also the backup process should complete in less
time.

Thanks in advance!!

Thanks & Regards,
Gayatri

Re: Inval reliability, especially for inplace updates

[Noah Misch]

On Wed, May 22, 2024 at 05:05:48PM -0700, Noah Misch wrote:
> https://postgr.es/m/20240512232923.aa.nmi...@google.com wrote:
> > Separable, nontrivial things not fixed in the attached patch stack:
> > 
> > - Inplace update uses transactional CacheInvalidateHeapTuple().  ROLLBACK of
> >   CREATE INDEX wrongly discards the inval, leading to the relhasindex=t loss
> >   still seen in inplace-inval.spec.  CacheInvalidateRelmap() does this 
> > right.
> 
> I plan to fix that like CacheInvalidateRelmap(): send the inval immediately,
> inside the critical section.  Send it in heap_xlog_inplace(), too.

> a. Within logical decoding, cease processing invalidations for inplace

I'm attaching the implementation.  This applies atop the v3 patch stack from
https://postgr.es/m/20240614003549.c2.nmi...@google.com, but the threads are
mostly orthogonal and intended for independent review.  Translating a tuple
into inval messages uses more infrastructure than relmapper, which needs just
a database ID.  Hence, this ended up more like a miniature of inval.c's
participation in the transaction commit sequence.

I waffled on whether to back-patch inplace150-inval-durability-atcommit.  The
consequences of that bug are plenty bad, but reaching them requires an error
between TransactionIdCommitTree() and AtEOXact_Inval().  I've not heard
reports of that, and I don't have a recipe for making it happen on demand.
For now, I'm leaning toward back-patch.  The main risk would be me overlooking
an LWLock deadlock scenario reachable from the new, earlier RelCacheInitLock
timing.  Alternatives for RelCacheInitLock:

- RelCacheInitLock before PreCommit_Notify(), because notify concurrency
  matters more than init file concurrency.  I chose this.
- RelCacheInitLock after PreCommit_Notify(), because PreCommit_Notify() uses a
  heavyweight lock, giving it less risk of undetected deadlock.
- Replace RelCacheInitLock with a heavyweight lock, and keep it before
  PreCommit_Notify().
- Fold PreCommit_Inval() back into AtCommit_Inval(), accepting that EIO in
  unlink_initfile() will PANIC.

Opinions on that?

The patch changes xl_heap_inplace of XLOG_HEAP_INPLACE.  For back branches, we
could choose between:

- Same change, no WAL version bump.  Standby must update before primary.  This
  is best long-term, but the transition is more disruptive.  I'm leaning
  toward this one, but the second option isn't bad:

- heap_xlog_inplace() could set the shared-inval-queue overflow signal on
  every backend.  This is more wasteful, but inplace updates might be rare
  enough (~once per VACUUM) to make it tolerable.

- Use LogStandbyInvalidations() just after XLOG_HEAP_INPLACE.  This isn't
  correct if one ends recovery between the two records, but you'd need to be
  unlucky to notice.  Noticing would need a procedure like the following.  A
  hot standby backend populates a relcache entry, then does DDL on the rel
  after recovery ends.

Future cleanup work could eliminate LogStandbyInvalidations() and the case of
!markXidCommitted && nmsgs != 0.  Currently, the src/test/regress suite still
reaches that case:

- AlterDomainDropConstraint() queues an inval even if !found; it can stop
  that.

- ON COMMIT DELETE ROWS nontransactionally rebuilds an index, which sends a
  relcache inval.  The point of that inval is, I think, to force access
  methods like btree and hash to reload the metapage copy that they store in
  rd_amcache.  Since no assigned XID implies no changes to the temp index, the
  no-XID case could simply skip the index rebuild.  (temp.sql reaches this
  with a read-only transaction that selects from an ON COMMIT DELETE ROWS
  table.  Realistic usage will tend not to do that.)  ON COMMIT DELETE ROWS
  has another preexisting problem for indexes, mentioned in a code comment.

Thanks,
nm
Author: Noah Misch 
Commit: Noah Misch 

Fix comments in and referring to AtEOXact_RelationCache().

The first point has prompted this change.  We can fix a bug by calling
AtEOXact_Inval(true) earlier, in the COMMIT critical section.  Remove
comment text that would have required AtEOXact_RelationCache() to move
with it.  Full list of defects fixed:

- Commit 8de3e410faa06ab20ec1aa6d0abb0a2c040261ba (2014-02) made
  relcache.c defer rebuilds if !IsTransactionState().  That removed the
  functional implications of swapping the order of
  AtEOXact_RelationCache() and AtEOXact_Inval().  Even without that
  change, we'd have other layers of defense.  At COMMIT:

  - Code that opened rels already closed them.
  - AtEOXact_RelationCache() essentially sets some non-pointer fields
and calls RelationDestroyRelation() to pfree() rels for which we're
committing both a CREATE and a DROP.  None of that needs a
transaction.
  - AtEOXact_Inval() doesn't locally-execute messages.  The next
AcceptInvalidationMessages() does that.

  At ABORT:

  - resowner.c already

Re: RFC: adding pytest as a supported test framework

[Melanie Plageman]

On Sat, Jun 15, 2024 at 5:53 PM Greg Sabino Mullane  wrote:
>
> On Sat, Jun 15, 2024 at 12:48 PM Jelte Fennema-Nio  wrote:
>>
>> Afaict, there's a significant part of our current community who feel the 
>> same way (and I'm pretty sure every sub-30 year old person who
>> newly joins the community would feel the exact same way too).
>
>
> Those young-uns are also the same group who hold their nose when coding in C, 
> and are always clamoring for rewriting Postgres in Rust. And before that, 
> C++. And next year, some other popular language that is clearly better and 
> more popular than C.

Writing a new test framework in a popular language that makes it more
likely that more people will write more tests and test infrastructure
is such a completely different thing than suggesting we rewrite
Postgres in Rust that I feel that this comparison is unfair and,
frankly, a distraction from the discussion at hand.

- Melanie

Re: RFC: adding pytest as a supported test framework

[Greg Sabino Mullane]

On Fri, Jun 14, 2024 at 5:09 PM Jelte Fennema-Nio 
wrote:

> Test::More on the other hand, while indeed still maintained, it's
> definitely not getting significant new feature development or
> improvements[2]. Especially when comparing it to pytest[3].
>

That's fair, although it's a little hard to tell if the lack of new
features is because they are not needed for a stable, mature project, or
because few people are asking for and developing new features. Probably a
bit of both. But I'll be the first to admit Perl is dying; I just don't
know what should replace it (or how - or when). Python has its quirks, but
all languages do, and your claim that it will encourage more and easier
test writing by developers is a good one.

Cheers,
Greg

Re: RFC: adding pytest as a supported test framework

[Greg Sabino Mullane]

On Sat, Jun 15, 2024 at 12:48 PM Jelte Fennema-Nio 
wrote:

> Afaict, there's a significant part of our current community who feel the
> same way (and I'm pretty sure every sub-30 year old person who
> newly joins the community would feel the exact same way too).
>

Those young-uns are also the same group who hold their nose when coding in
C, and are always clamoring for rewriting Postgres in Rust. And before
that, C++. And next year, some other popular language that is clearly
better and more popular than C.

And I agree with Robbert that Python seems like the best choice for this
> other language, given its current popularity level. But as I said
> before, I'm open to other languages as well.
>

Despite my previous posts, I am open to other languages too, including
Python, but the onus is really on the new language promoters to prove that
the very large amount of time and trouble is worth it, and worth it for
language X.

Cheers,
Greg

Re: Columnar format export in Postgres

[Sutou Kouhei]

Hi,

In 
  "Re: Columnar format export in Postgres" on Thu, 13 Jun 2024 22:30:24 +0530,
  Sushrut Shivaswamy  wrote:

>  - To facilitate efficient querying it would help to export multiple
> parquet files for the table instead of a single file.
>Having multiple files allows queries to skip chunks if the key range in
> the chunk does not match query filter criteria.
>Even within a chunk it would help to be able to configure the size of a
> row group.
>   - I'm not sure how these parameters will be exposed within `COPY TO`.
> Or maybe the extension implementing the `COPY TO` handler will
> allow this configuration?

Yes. But adding support for custom COPY TO options is
out-of-scope in the first version. We will focus on only the
minimal features in the first version. We can improve it
later based on use-cases.

See also: 
https://www.postgresql.org/message-id/20240131.141122.279551156957581322.kou%40clear-code.com

>  - Regarding using file_fdw to read Apache Arrow and Apache Parquet file
> because file_fdw is based on COPY FROM:
>  - I'm not too clear on this. file_fdw seems to allow creating a table
> from  data on disk exported using COPY TO.

Correct.

>But is the newly created table still using the data on disk(maybe in
> columnar format or csv) or is it just reading that data to create a row
> based table.

The former.

>I'm not aware of any capability in the postgres planner to read
> columnar files currently without using an extension like parquet_fdw.

Correct. We still need another approach such as parquet_fdw
with the COPY format extensible feature to optimize query
against Apache Parquet data. file_fdw can just read Apache
Parquet data by SELECT. Sorry for confusing you.


Thanks,
-- 
kou

Re: Shouldn't jsonpath .string() Unwrap?

[David E. Wheeler]

On Jun 15, 2024, at 12:48, Andrew Dunstan  wrote:

> Not really needed, I will commit shortly.

Ah, okay, I wasn’t sure so just defaulted to making sure it was tracked. :-)

Thanks Andrew,

D

Re: CREATE INDEX CONCURRENTLY on partitioned index

[Justin Pryzby]

On Thu, May 23, 2024 at 10:14:57PM +0100, Ilya Gladyshev wrote:
> Hi,
> 
> I think it's well worth the effort to revive the patch, so I rebased it on
> master, updated it and will return it back to the commitfest. Alexander,
> Justin feel free to add yourselves as authors

Thanks -- I was intending to write about this.

I realized that the patch will need some isolation tests to exercise its
concurrent behavior.

-- 
Justin

Re: CREATE INDEX CONCURRENTLY on partitioned index

[Ilya Gladyshev]

On 28.05.2024 07:05, Alexander Pyhalov wrote:

Ilya Gladyshev писал(а) 2024-05-28 02:52:

Also I'd like to note that in new patch version there's a strange 
wording in documentation:


"This can be very convenient as not only will all existing 
partitions be

 indexed, but any future partitions will be as well.
 CREATE INDEX ... CONCURRENTLY can incur long 
lock times

 on huge partitioned tables, to avoid that you can
 use CREATE INDEX ON ONLY the partitioned table, 
which
 creates the new index marked as invalid, preventing automatic 
application

 to existing partitions."

All the point of CIC is to avoid long lock times. So it seems this 
paragraph should be rewritten in the following way:


"To avoid long lock times, you can use CREATE INDEX CONCURRENTLY or 
CREATE INDEX ON ONLY the partitioned table..."



True, the current wording doesn't look right. Right now CREATE INDEX 
ON ONLY is described as a workaround for the missing CIC. I think it 
rather makes sense to say that it gives more fine-grained control of 
partition locking than both CIC and ordinary CREATE INDEX. See the 
updated patch.


Hi.

Not sure if it's worth removing mentioning of CIC in

  creates the new index marked as invalid, preventing automatic 
application
  to existing partitions.  Instead, indexes can then be created 
individually

- on each partition using CONCURRENTLY and
+ on each partition and
  attached to the partitioned index on the 
parent
  using ALTER INDEX ... ATTACH PARTITION.  Once 
indexes for
  all the partitions are attached to the parent index, the parent 
index will


but at least now it looks better.


The current patch version locks all the partitions in the first 
transaction up until each of them is built, which makes for long lock 
times for partitions that are built last. Having looked at the 
implementation of REINDEX CONCURRENTLY for partitioned tables, I think 
we can improve this by using the same approach of just skipping the 
relations that we find out are dropped when trying to lock them. 
Incidentally, this implementation in the new patch version is also simpler.


In addition, I noticed that progress tracking is once again broken for 
partitioned tables, while looking at REINDEX implementation, attaching 
the second patch to fix it.


From 884be03aaeabee5c6eeb5a3f639ac9afe712c24b Mon Sep 17 00:00:00 2001
From: Ilya Gladyshev 
Date: Tue, 11 Jun 2024 17:48:08 +0100
Subject: [PATCH v4 2/2] Fix progress report for partitioned REINDEX

---
 src/backend/catalog/index.c  | 11 --
 src/backend/commands/indexcmds.c | 63 +---
 src/include/catalog/index.h  |  1 +
 3 files changed, 66 insertions(+), 9 deletions(-)

diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 55fdde4b24..c5bc72b350 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -3559,6 +3559,7 @@ reindex_index(const ReindexStmt *stmt, Oid indexId,
 	volatile bool skipped_constraint = false;
 	PGRUsage	ru0;
 	bool		progress = ((params->options & REINDEXOPT_REPORT_PROGRESS) != 0);
+	bool		partition = ((params->options & REINDEXOPT_PARTITION) != 0);
 	bool		set_tablespace = false;
 
 	pg_rusage_init(&ru0);
@@ -3604,8 +3605,9 @@ reindex_index(const ReindexStmt *stmt, Oid indexId,
 			indexId
 		};
 
-		pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX,
-	  heapId);
+		if (!partition)
+			pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX,
+		  heapId);
 		pgstat_progress_update_multi_param(2, progress_cols, progress_vals);
 	}
 
@@ -3845,8 +3847,11 @@ reindex_index(const ReindexStmt *stmt, Oid indexId,
 	index_close(iRel, NoLock);
 	table_close(heapRelation, NoLock);
 
-	if (progress)
+	if (progress && !partition)
+	{
+		/* progress for partitions is tracked in the caller */
 		pgstat_progress_end_command();
+	}
 }
 
 /*
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 5da2df2d3b..17b30ad6aa 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -118,6 +118,7 @@ static bool ReindexRelationConcurrently(const ReindexStmt *stmt,
 		const ReindexParams *params);
 static void update_relispartition(Oid relationId, bool newval);
 static inline void set_indexsafe_procflags(void);
+static inline void progress_index_partition_done(void);
 
 /*
  * callback argument type for RangeVarCallbackForReindexIndex()
@@ -1550,7 +1551,7 @@ DefineIndex(Oid tableId,
  * Update progress for an intermediate partitioned index
  * itself
  */
-pgstat_progress_incr_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, 1);
+progress_index_partition_done();
 			}
 
 			return address;
@@ -1577,7 +1578,7 @@ DefineIndex(Oid tableId,
 		if (!OidIsValid(parentIndexId))
 			pgstat_progress_end_command();
 		else if (!concurrent)
-			pgstat_progress_incr_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, 1);
+			progress_index_partition_done();
 
 		retu

Re: DROP OWNED BY fails to clean out pg_init_privs grants

[Tom Lane]

I wrote:
> The semantics I've implemented on top of that are:
> * ALTER OWNER does not touch pg_init_privs entries.
> * REASSIGN OWNED replaces pg_init_privs references with the new
> role, whether the references are as grantor or grantee.
> * DROP OWNED removes pg_init_privs mentions of the doomed role
> (as grantor or grantee), removing the pg_init_privs entry
> altogether if it goes to zero clauses.  (This is what happened
> already, but only if if a SHARED_DEPENDENCY_INITACL entry existed.)

> I'm not terribly thrilled with this, because it's still possible
> to get into a state where a pg_init_privs entry is based on
> an owning role that's no longer the owner: you just have to use
> ALTER OWNER directly rather than via REASSIGN OWNED.  While
> I don't think the backend has much problem with that, it probably
> will confuse pg_dump to some extent.

I poked at this some more, and I'm now moderately convinced that
this is a good place to stop for v17, primarily because ALTER OWNER
doesn't touch pg_init_privs in the older branches either.  Thus,
while we've not made things better for pg_dump, at least we've not
introduced a new behavior it will have to account for in the future.

I experimented with this test scenario (to set up, do
"make install" in src/test/modules/test_pg_dump):

-
create role regress_dump_test_role;
create user test_super superuser;
create user joe;
create database tpd;
\c tpd test_super 
create extension test_pg_dump;
alter function regress_pg_dump_schema.test_func() owner to joe;
\df+ regress_pg_dump_schema.test_func()
-

The \df+ will correctly show that test_func() is owned by joe
and has ACL

|  Access privileges   |
+--+
| =X/joe  +|
| joe=X/joe   +|
| regress_dump_test_role=X/joe |

Now, if you pg_dump this database, what you get is

CREATE EXTENSION IF NOT EXISTS test_pg_dump WITH SCHEMA public;
...
--
-- Name: FUNCTION test_func(); Type: ACL; Schema: regress_pg_dump_schema; 
Owner: joe
--

REVOKE ALL ON FUNCTION regress_pg_dump_schema.test_func() FROM PUBLIC;
REVOKE ALL ON FUNCTION regress_pg_dump_schema.test_func() FROM test_super;
REVOKE ALL ON FUNCTION regress_pg_dump_schema.test_func() FROM 
regress_dump_test_role;
GRANT ALL ON FUNCTION regress_pg_dump_schema.test_func() TO joe;
GRANT ALL ON FUNCTION regress_pg_dump_schema.test_func() TO PUBLIC;
GRANT ALL ON FUNCTION regress_pg_dump_schema.test_func() TO 
regress_dump_test_role;

So pg_dump realizes that the privileges are not what they were,
but it's fairly confused about what to do about it.  And it really
can't get that right without better modeling (er, more than none at
all) of the ownership of extension-member objects.  If you restore
this dump as postgres, you'll find that test_func is now owned by
postgres and has ACL

| Access privileges |
+---+
| postgres=X/postgres  +|
| joe=X/postgres   +|
| =X/postgres  +|
| regress_dump_test_role=X/postgres |

The grantees are okay, more or less, but we've totally failed to
replicate the owner/grantor.  But this is exactly the same as what
you get if you do the experiment in v16 or before.  Given the lack
of complaints about that, I think it's okay to stop here for now.
We've at least made REASSIGN OWNED work better.

regards, tom lane

Re: RFC: adding pytest as a supported test framework

[Jelte Fennema-Nio]

On Sat, 15 Jun 2024 at 19:27, Robert Haas  wrote:
> This surprises me. I agree that the current state of affairs is kind
> of annoying, but the contents of regress_log_whatever are usually
> quite long. Printing all of that out to standard output seems like
> it's just going to flood the terminal with output. I don't think I'd
> be a fan of that change.

I think at the very least the locations of the different logs should
be listed in the output.

Re: RFC: adding pytest as a supported test framework

[Andres Freund]

Hi,

On 2024-06-15 10:45:16 -0400, Andrew Dunstan wrote:
> > 4. Pytest has autodiscovery of test files and functions, so we
> > probably wouldn't have to specify all of the exact test files anymore
> > in the meson.build files.
> 
> 
> I find this comment a bit ironic. We don't need to do that with the
> Makefiles, and the requirement to do so was promoted as a meson feature
> rather than a limitation, ISTR.

The reason its good to have the list of tests somewhere explicit is that we
have different types of test. With make, there is a single target for all tap
tests. If you want to run tests concurrently, make can only schedule the tap
tests at the granularity of a directory. If you want concurrency below that,
you need to use concurrency on the prove level. But that means that you have
extremely varying concurrency, depending on whether make runs targets that
have no internal concurrency or make runs e.g. the recovery tap tests.

I don't think we should rely on global test discovery via pytest. That'll lead
to uncontrollable concurrency again, which means much longer test times. We'll
always have different types of tests, just scheduling running them via
"top-level" tools for different test types just won't work well. That's not
true for many projects where tests have vastly lower resource usage.

Greetings,

Andres Freund

Re: RFC: adding pytest as a supported test framework

[Robert Haas]

On Sat, Jun 15, 2024 at 12:48 PM Jelte Fennema-Nio  wrote:
> Honestly, my primary *objective* complaint about our current test
> suite, is that when a test fails, it's very often impossible for me to
> understand why the test failed, by only looking at the output of
> "meson test". I think logging the postgres log to stderr for Perl, as
> you proposed, would significantly improve that situation. I think the
> only thing that we cannot get from Perl Test::More that we can from
> pytest, is the fancy recursive introspection of the expression that
> pytest shows on error.

This surprises me. I agree that the current state of affairs is kind
of annoying, but the contents of regress_log_whatever are usually
quite long. Printing all of that out to standard output seems like
it's just going to flood the terminal with output. I don't think I'd
be a fan of that change.

I think I basically agree with all the nearby comments about how the
advantages you cite for Python aren't, I don't know, entirely
compelling. Switching from ok() to is() or cmp_ok() or like() is minor
stuff. Where the output goes is minor stuff. The former can be fixed,
and the latter can be worked around with scripts and aliases. The one
thing I know about that *I* think is a pretty big problem about Perl
is that IPC::Run is not really maintained. But I wonder if the
solution to that is to do something ourselves instead of depending on
IPC::Run. Beyond that, I think this is just a language popularity
contest.

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

Re: Shouldn't jsonpath .string() Unwrap?

[Andrew Dunstan]

On 2024-06-15 Sa 10:51, David E. Wheeler wrote:

On Jun 15, 2024, at 10:39, David E. Wheeler  wrote:


The changes are straightforward and look good to me. However, I have kept the 
existing test of an empty array as is, assuming that it is one of the valid 
tests. It now returns zero rows instead of an error. Your added test case 
covers this issue.

Makes sense, thank you.

Added https://commitfest.postgresql.org/48/5039/.



Not really needed, I will commit shortly.


cheers


andrew

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

Re: RFC: adding pytest as a supported test framework

[Jelte Fennema-Nio]

On Sat, 15 Jun 2024 at 16:45, Andrew Dunstan  wrote:
> I see the fact that we stash the output in a file as a feature. Without
> it, capturing failure information in the buildfarm client would be more
> difficult, especially if there are multiple failures. So this is
> actually something I think we would need for any alternative framework.

I indeed heard that the current behaviour was somehow useful to the
buildfarm client.

> Maybe we need an environment setting that would output the
> regress_log_00whatever file to stderr on failure.  That should be pretty
> easy to arrange in the END handler for PostgreSQL::Test::Utils.

That sounds awesome! But I'm wondering: do we really need a setting to
enable/disable that? Can't we always output it to stderr on failure?
If we output the log both to stderr and as a file, would that be fine
for the build farm? If not, a setting should work. (but I'd prefer the
default for that setting to be on in that case, it seems much easier
to turn it off in the buildfarm client, instead of asking every
developer to turn the feature on)

> > 4. Pytest has autodiscovery of test files and functions, so we
> > probably wouldn't have to specify all of the exact test files anymore
> > in the meson.build files.
>
>
> I find this comment a bit ironic. We don't need to do that with the
> Makefiles, and the requirement to do so was promoted as a meson feature
> rather than a limitation, ISTR.

Now, I'm very curious why that would be considered a feature. I
certainly have had many cases where I forgot to add the test file to
the meson.build file.

> > Regarding 2, there are ~150 checks that are using a suboptimal way of
> > testing for a comparison. Mostly a lot that could use "like(..., ...)"
> > instead of "ok(... ~= ...)"
> > ❯ grep '\bok(.*=' **.pl | wc -l
> > 151
>
>
> Well, let's fix those. I would be tempted to use cmp_ok() for just about
> all of them.

Sounds great to me.

> But the fact that Test::More has a handful of test primitives rather
> than just one strikes me as a relatively minor complaint.

It is indeed a minor paper cut, but paper-cuts add up.

Honestly, my primary *objective* complaint about our current test
suite, is that when a test fails, it's very often impossible for me to
understand why the test failed, by only looking at the output of
"meson test". I think logging the postgres log to stderr for Perl, as
you proposed, would significantly improve that situation. I think the
only thing that we cannot get from Perl Test::More that we can from
pytest, is the fancy recursive introspection of the expression that
pytest shows on error.


Apart from that my major *subjective* complaint is that I very much
dislike writing Perl code. I'm slow at writing it and I don't (want
to) improve at it because I don't have reasons to use it except for
Postgres tests. So currently I'm not really incentivised to write more
tests than the bare minimum, help improve the current test tooling, or
add new testing frameworks for things we currently cannot test.
Afaict, there's a significant part of our current community who feel
the same way (and I'm pretty sure every sub-30 year old person who
newly joins the community would feel the exact same way too).

As a project I think we would like to have more tests, and to have
more custom tooling to test things that we currently cannot (e.g.
oauth or manually messing with the wire-protocol). I think the only
way to achieve that is by encouraging more people to work on these
things. I very much appreciate that you and others are improving our
Perl tooling, because that makes our current tests easier to work
with. But I don't think it significantly increases the willingness to
write tests or test-tooling for people that don't want to write Perl
in the first place.

So I think the only way to get more people involved in contributing
tests and test-tooling is by allowing testing in another language than
Perl (but also still allow writing tests in Perl). Even if that means
that we have two partially-overlapping test frameworks, that are both
easy to use for different things. In my view that's even a positive
thing, because that means we are able to test more with two languages
than we would be able to with either one (and it's thus useful to have
both).

And I agree with Robbert that Python seems like the best choice for
this other language, given its current popularity level. But as I said
before, I'm open to other languages as well.

Re: jsonpath: Missing regex_like && starts with Errors?

[Chapman Flack]

On 06/15/24 10:47, David E. Wheeler wrote:
> these are predicate check expressions, supported and documented
> as an extension to the standard since Postgres 12[1].
> ...
> [1]: 
> https://www.postgresql.org/docs/devel/functions-json.html#FUNCTIONS-SQLJSON-CHECK-EXPRESSIONS

I see. Yes, that documentation now says "predicate check expressions return
the single three-valued result of the predicate: true, false, or unknown".

(Aside: are all readers of the docs assumed to have learned the habit
of calling SQL null "unknown" when speaking of a boolean? They can flip
back to 8.6 Boolean Type and see 'a third state, “unknown”, which is
represented by the SQL null value'. But would it save them some page
flipping to add " (represented by SQL null)" to the sentence here?)

As Unknown is typically what the predicates return within a filter (where
errors get trapped) when an error has occurred, the existing docs seem to
suggest they behave the same way in a "predicate check expression", so a
change to that behavior now would be a change to what we've documented.

OTOH, getting Unknown because some error occurred is strictly less
information than seeing the error, so perhaps you would want a way
to request non-error-trapping behavior for a "predicate check expression".

Can't really overload jsonb_path_query's 'silent' parameter for that,
because it is already false by default. If predicate check expressions
were nonsilent by default, the existing 'silent' parameter would be a
perfect way to silence them.

No appetite to add yet another optional boolean parameter to
jsonb_path_query for the sole purpose of controlling the silence of
our nonstandard syntax extension 

Maybe just see the nonstandard syntax extension and raise it another one:

expr_or_predicate
: expr
| predicate
| "nonsilent" '(' predicate ')'
;

or something like that.

Regards,
-Chap

Re: Shouldn't jsonpath .string() Unwrap?

[David E. Wheeler]

On Jun 15, 2024, at 10:39, David E. Wheeler  wrote:

>> The changes are straightforward and look good to me. However, I have kept 
>> the existing test of an empty array as is, assuming that it is one of the 
>> valid tests. It now returns zero rows instead of an error. Your added test 
>> case covers this issue.
> 
> Makes sense, thank you.

Added https://commitfest.postgresql.org/48/5039/.

D

Re: jsonpath: Missing regex_like && starts with Errors?

[David E. Wheeler]

On Jun 14, 2024, at 22:29, Chapman Flack  wrote:

> So I should go look at our code to see what grammar we've implemented,
> exactly. It is beginning to seem as if we have simply added
>  as another choice for an expression, not restricted
> to only appearing in a filter. If so, and we add documentation about how
> we diverge from the standard, that's probably the way to say it.

Yes, if I understand correctly, these are predicate check expressions, 
supported and documented as an extension to the standard since Postgres 12[1]. 
I found their behavior quite confusing for a while, and spent some time 
figuring it out and submitting a doc patch (committed in 7014c9a[2]) to 
hopefully clarify things in Postgres 17.

> So that's where the errors went.

Ah, great, that explains the error suppression in filters. Thank you. I still 
think the supression of `like_regex` and `starts with` errors in predicate path 
queries is odd, though.

> The question of what should happen to the errors when a
>  appears outside of a 
> of course isn't answered in the standard, because that's not supposed
> to be possible. So if we're allowing predicates to appear on their own
> as expressions, it's also up to us to say what should happen with errors
> when they do.

Right, and I think there’s an inconsistency right now.

Best,

David

[1]: 
https://www.postgresql.org/docs/devel/functions-json.html#FUNCTIONS-SQLJSON-CHECK-EXPRESSIONS
[2]: https://github.com/postgres/postgres/commit/7014c9a

Re: RFC: adding pytest as a supported test framework

[Andrew Dunstan]

On 2024-06-14 Fr 18:11, Jelte Fennema-Nio wrote:

On Fri, 14 Jun 2024 at 17:49, Tom Lane  wrote:

But what I'd really like to see is some comparison of the
language-provided testing facilities that we're proposing
to depend on.  Why is pytest (or whatever) better than Test::More?

Some advantages of pytest over Test::More:

1. It's much easier to debug failing tests using the output that
pytest gives. A good example of this is on pytest its homepage[1]
(i.e. it shows the value given to the call to inc in the error)
2. No need to remember a specific comparison DSL
(is/isnt/is_deeply/like/ok/cmp_ok/isa_ok), just put assert in front of
a boolean expression and your output is great (if you want to add a
message too for clarity you can use: assert a == b, "the world is
ending")
3. Very easy to postgres log files on stderr/stdout when a test fails.
This might be possible/easy with Perl too, but we currently don't do
that. So right now for many failures you're forced to traverse the
build/testrun/... directory tree to find the logs.



I see the fact that we stash the output in a file as a feature. Without 
it, capturing failure information in the buildfarm client would be more 
difficult, especially if there are multiple failures. So this is 
actually something I think we would need for any alternative framework.


Maybe we need an environment setting that would output the 
regress_log_00whatever file to stderr on failure.  That should be pretty 
easy to arrange in the END handler for PostgreSQL::Test::Utils.




4. Pytest has autodiscovery of test files and functions, so we
probably wouldn't have to specify all of the exact test files anymore
in the meson.build files.



I find this comment a bit ironic. We don't need to do that with the 
Makefiles, and the requirement to do so was promoted as a meson feature 
rather than a limitation, ISTR.




Regarding 2, there are ~150 checks that are using a suboptimal way of
testing for a comparison. Mostly a lot that could use "like(..., ...)"
instead of "ok(... ~= ...)"
❯ grep '\bok(.*=' **.pl | wc -l
151



Well, let's fix those. I would be tempted to use cmp_ok() for just about 
all of them.


But the fact that Test::More has a handful of test primitives rather 
than just one strikes me as a relatively minor complaint.




cheers


andrew


--

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

Re: Shouldn't jsonpath .string() Unwrap?

[David E. Wheeler]

On Jun 15, 2024, at 10:27, Jeevan Chalke  wrote:

> Sorry, I have missed this in the original patch. I am surprised how that 
> happened. But thanks for catching the same and fixing it.

No worries. :-)

> The changes are straightforward and look good to me. However, I have kept the 
> existing test of an empty array as is, assuming that it is one of the valid 
> tests. It now returns zero rows instead of an error. Your added test case 
> covers this issue.

Makes sense, thank you.

D

Re: Shouldn't jsonpath .string() Unwrap?

[Jeevan Chalke]

Hi,

Sorry, I have missed this in the original patch. I am surprised how that
happened. But thanks for catching the same and fixing it.

The changes are straightforward and look good to me. However, I have kept
the existing test of an empty array as is, assuming that it is one of the
valid tests. It now returns zero rows instead of an error. Your added test
case covers this issue.

Thanks



On Fri, Jun 14, 2024 at 9:34 PM David E. Wheeler 
wrote:

>
>
> > On Jun 14, 2024, at 11:25, Chapman Flack  wrote:
> >
> > I would s/extepsions/exceptions/ in the added documentation. :)
>
> Bah, fixed and attached, thanks.
>
> > Offhand (as GitHub PRs aren't really The PG Way), if they were The Way,
> > I would find this one a little hard to follow, being based at a point
> > 28 unrelated commits ahead of the ref it's opened against. I suspect
> > 'master' on theory/postgres could be fast-forwarded to f1affb6 and then
> > the PR would look much more approachable.
>
> Yeah, I pushed the PR and branch before I synced master, and GitHub was
> taking a while to notice and update the PR. I fixed it with `git commit
> --all --amend --date now --reedit-message HEAD` and force-pushed (then
> fixed the typo and fixed again).
>
> D
>
>
>
>

-- 



*Jeevan Chalke*

*Principal, ManagerProduct Development*

enterprisedb.com 


v3-0001-Teach-jsonpath-string-to-unwrap-in-lax-mode.patch
Description: Binary data

Re: Revive num_dead_tuples column of pg_stat_progress_vacuum

[Robert Treat]

On Thu, Jun 13, 2024 at 9:22 PM Masahiko Sawada  wrote:
> On Fri, Jun 14, 2024 at 9:57 AM Masahiko Sawada  wrote:
> > On Fri, Jun 14, 2024 at 9:41 AM Michael Paquier  wrote:
> > > On Thu, Jun 13, 2024 at 08:38:05PM -0400, Tom Lane wrote:
> > > > Masahiko Sawada  writes:
> > > >> I was about to push the patch but let me confirm just in case: is it
> > > >> okay to bump the catversion even after post-beta1?
> > > >
> > > > Yes, that happens somewhat routinely.
> > >
> > > Up to RC, even after beta2.  This happens routinely every year because
> > > tweaks are always required for what got committed.  And that's OK to
> > > do so now.
> >
> > Thank you both for confirmation. I'll push it shortly.
> >
>
> Pushed. Thank you for giving feedback and reviewing the patch!
>

One minor side effect of this change is the original idea of comparing
pg_stat_progress.num_dead_tuples to pg_stat_all_tables.n_dead_tup
column becomes less obvious. I presume the release notes for
pg_stat_progress_vacuum will be updated to also include this column
name change as well, so maybe that's enough for folks to figure things
out? At least I couldn't find anywhere in the docs where we have
described the relationship between these columns before. Thoughts?

Robert Treat
https://xzilla.net

Re: Pgoutput not capturing the generated columns

[Shlok Kyal]

On Thu, 6 Jun 2024 at 08:29, Hayato Kuroda (Fujitsu)
 wrote:
>
> Dear Shlok and Shubham,
>
> Thanks for updating the patch!
>
> I briefly checked the v5-0002. IIUC, your patch allows to copy generated
> columns unconditionally. I think the behavior affects many people so that it 
> is
> hard to get agreement.
>
> Can we add a new option like `GENERATED_COLUMNS [boolean]`? If the default is 
> set
> to off, we can keep the current specification.
>
> Thought?
Hi Kuroda-san,

I agree that we should not allow to copy generated columns unconditionally.
With patch v7-0002, I have used a different approach which does not
require any code changes in COPY.

Please refer [1] for patch v7-0002.
[1]: 
https://www.postgresql.org/message-id/CANhcyEUz0FcyR3T76b%2BNhtmvWO7o96O_oEwsLZNZksEoPmVzXw%40mail.gmail.com

Thanks and Regards,
Shlok Kyal

Re: Pgoutput not capturing the generated columns

[Shlok Kyal]

On Wed, 5 Jun 2024 at 05:49, Peter Smith  wrote:
>
> On Mon, Jun 3, 2024 at 9:52 PM Shlok Kyal  wrote:
> >
> > >
> > > The attached Patch contains the suggested changes.
> > >
> >
> > Hi,
> >
> > Currently, COPY command does not work for generated columns and
> > therefore, COPY of generated column is not supported during tablesync
> > process. So, in patch v4-0001 we added a check to allow replication of
> > the generated column only if 'copy_data = false'.
> >
> > I am attaching patches to resolve the above issues.
> >
> > v5-0001: not changed
> > v5-0002: Support COPY of generated column
> > v5-0003: Support COPY of generated column during tablesync process
> >
>
> Hi Shlok, I have a question about patch v5-0003.
>
> According to the patch 0001 docs "If the subscriber-side column is
> also a generated column then this option has no effect; the replicated
> data will be ignored and the subscriber column will be filled as
> normal with the subscriber-side computed or default data".
>
> Doesn't this mean it will be a waste of effort/resources to COPY any
> column value where the subscriber-side column is generated since we
> know that any copied value will be ignored anyway?
>
> But I don't recall seeing any comment or logic for this kind of copy
> optimisation in the patch 0003. Is this already accounted for
> somewhere and I missed it, or is my understanding wrong?
Your understanding is correct.
With v7-0002, if a subscriber-side column is generated, then we do not
include that column in the column list during COPY. This will address
the above issue.

Patch 7-0002 contains all the changes. Please refer [1]
[1]: 
https://www.postgresql.org/message-id/CANhcyEUz0FcyR3T76b%2BNhtmvWO7o96O_oEwsLZNZksEoPmVzXw%40mail.gmail.com

Thanks and Regards,
Shlok Kyal

Re: Pgoutput not capturing the generated columns

[Shlok Kyal]

On Tue, 4 Jun 2024 at 15:01, Peter Smith  wrote:
>
> Hi,
>
> Here are some review comments for patch v5-0003.
>
> ==
> 0. Whitespace warnings when the patch was applied.
>
> [postgres@CentOS7-x64 oss_postgres_misc]$ git apply
> ../patches_misc/v5-0003-Support-copy-of-generated-columns-during-tablesyn.patch
> ../patches_misc/v5-0003-Support-copy-of-generated-columns-during-tablesyn.patch:29:
> trailing whitespace.
>   has no effect; the replicated data will be ignored and the 
> subscriber
> ../patches_misc/v5-0003-Support-copy-of-generated-columns-during-tablesyn.patch:30:
> trailing whitespace.
>   column will be filled as normal with the subscriber-side computed or
> ../patches_misc/v5-0003-Support-copy-of-generated-columns-during-tablesyn.patch:189:
> trailing whitespace.
> (walrcv_server_version(LogRepWorkerWalRcvConn) >= 12 &&
> warning: 3 lines add whitespace errors.
>
Fixed

> ==
> src/backend/commands/subscriptioncmds.c
>
> 1.
> - res = walrcv_exec(wrconn, cmd.data, check_columnlist ? 3 : 2, tableRow);
> + column_count = (!include_generated_column && check_gen_col) ? 4 :
> (check_columnlist ? 3 : 2);
> + res = walrcv_exec(wrconn, cmd.data, column_count, tableRow);
>
> The 'column_count' seems out of control. Won't it be far simpler to
> assign/increment the value dynamically only as required instead of the
> tricky calculation at the end which is unnecessarily difficult to
> understand?
>
I have removed this piece of code.

> ~~~
>
> 2.
> + /*
> + * If include_generated_column option is false and all the column of
> the table in the
> + * publication are generated then we should throw an error.
> + */
> + if (!isnull && !include_generated_column && check_gen_col)
> + {
> + attlist = DatumGetArrayTypeP(attlistdatum);
> + gen_col_count = DatumGetInt32(slot_getattr(slot, 4, &isnull));
> + Assert(!isnull);
> +
> + attcount = ArrayGetNItems(ARR_NDIM(attlist), ARR_DIMS(attlist));
> +
> + if (attcount != 0 && attcount == gen_col_count)
> + ereport(ERROR,
> + errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> + errmsg("cannot use only generated column for table \"%s.%s\" in
> publication when generated_column option is false",
> +nspname, relname));
> + }
> +
>
> Why do you think this new logic/error is necessary?
>
> IIUC the 'include_generated_columns' should be false to match the
> existing HEAD behavior. So this scenario where your publisher-side
> table *only* has generated columns is something that could already
> happen, right? IOW, this introduced error could be a candidate for
> another discussion/thread/patch, but is it really required for this
> current patch?
>
Yes, this scenario can also happen in HEAD. For this patch I have
removed this check.

> ==
> src/backend/replication/logical/tablesync.c
>
> 3.
>   lrel->remoteid,
> - (walrcv_server_version(LogRepWorkerWalRcvConn) >= 12 ?
> -   "AND a.attgenerated = ''" : ""),
> + (walrcv_server_version(LogRepWorkerWalRcvConn) >= 12 &&
> + (walrcv_server_version(LogRepWorkerWalRcvConn) <= 16 ||
> + !MySubscription->includegeneratedcolumn) ? "AND a.attgenerated = ''" : ""),
>
> This ternary within one big appendStringInfo seems quite complicated.
> Won't it be better to split the appendStringInfo into multiple parts
> so the generated-cols calculation can be done more simply?
>
Fixed

> ==
> src/test/subscription/t/011_generated.pl
>
> 4.
> I think there should be a variety of different tablesync scenarios
> (when 'include_generated_columns' is true) tested here instead of just
> one, and all varieties with lots of comments to say what they are
> doing, expectations etc.
>
> a. publisher-side gen-col "a" replicating to subscriber-side NOT
> gen-col "a" (ok, value gets replicated)
> b. publisher-side gen-col "a" replicating to subscriber-side gen-col
> (ok, but ignored)
> c. publisher-side NOT gen-col "b" replicating to subscriber-side
> gen-col "b" (error?)
>
Added the tests

> ~~
>
> 5.
> +$result = $node_subscriber->safe_psql('postgres', "SELECT a, b FROM tab3");
> +is( $result, qq(1|2
> +2|4
> +3|6), 'generated columns initial sync with include_generated_column = true');
>
> Should this say "ORDER BY..." so it will not fail if the row order
> happens to be something unanticipated?
>
Fixed

> ==
>
> 99.
> Also, see the attached file with numerous other nitpicks:
> - plural param- and var-names
> - typos in comments
> - missing spaces
> - SQL keyword should be UPPERCASE
> - etc.
>
> Please apply any/all of these if you agree with them.
Fixed

Patch 7-0002 contains all the changes. Please refer [1]
[1]: 
https://www.postgresql.org/message-id/CANhcyEUz0FcyR3T76b%2BNhtmvWO7o96O_oEwsLZNZksEoPmVzXw%40mail.gmail.com

Thanks and Regards,
Shlok Kyal

Re: Pgoutput not capturing the generated columns

[Shlok Kyal]

On Tue, 4 Jun 2024 at 10:21, Peter Smith  wrote:
>
> Hi,
>
> Here are some review comments for patch v5-0002.
>
> ==
> GENERAL
>
> G1.
> IIUC now you are unconditionally allowing all generated columns to be copied.
>
> I think this is assuming that the table sync code (in the next patch
> 0003?) is going to explicitly name all the columns it wants to copy
> (so if it wants to get generated cols then it will name the generated
> cols, and if is doesn't want generated cols then it won't name them).
>
> Maybe that is OK for the logical replication tablesync case, but I am
> not sure if it will be desirable to *always* copy generated columns in
> other user scenarios.
>
> e.g. I was wondering if there should be a new COPY command option
> introduced here -- INCLUDE_GENERATED_COLUMNS (with default false) so
> then the current HEAD behaviour is unaffected unless that option is
> enabled.
>
> ~~~
>
> G2.
> The current COPY command documentation [1] says "If no column list is
> specified, all columns of the table except generated columns will be
> copied."
>
> But this 0002 patch has changed that documented behaviour, and so the
> documentation needs to be changed as well, right?
>
> ==
> Commit Message
>
> 1.
> Currently COPY command do not copy generated column. With this commit
> added support for COPY for generated column.
>
> ~
>
> The grammar/cardinality is not good here. Try some tool (Grammarly or
> chatGPT, etc) to help correct it.
>
> ==
> src/backend/commands/copy.c
>
> ==
> src/test/regress/expected/generated.out
>
> ==
> src/test/regress/sql/generated.sql
>
> 2.
> I think these COPY test cases require some explicit comments to
> describe what they are doing, and what are the expected results.
>
> Currently, I have doubts about some of this test input/output
>
> e.g.1. Why is the 'b' column sometimes specified as 1? It needs some
> explanation. Are you expecting this generated col value to be
> ignored/overwritten or what?
>
> COPY gtest1 (a, b) FROM stdin DELIMITER ' ';
> 5 1
> 6 1
> \.
>
> e.g.2. what is the reason for this new 'missing data for column "b"'
> error? Or is it some introduced quirk because "b" now cannot be
> generated since there is no value for "a"? I don't know if the
> expected *.out here is OK or not, so some test comments may help to
> clarify it.
>
> ==
> [1] https://www.postgresql.org/docs/devel/sql-copy.html
>
Hi Peter,

I have removed the changes in the COPY command. I came up with an
approach which requires changes only in tablesync code. We can COPY
generated columns during tablesync using syntax 'COPY (SELECT
column_name from table) TO STDOUT.'

I have attached the patch for the same.
v7-0001 : Not Modified
v7-0002: Support replication of generated columns during initial sync.

Thanks and Regards,
Shlok Kyal


v7-0002-Support-replication-of-generated-column-during-in.patch
Description: Binary data


v7-0001-Enable-support-for-include_generated_columns-opti.patch
Description: Binary data

Re: strange context message in spi.c?

[Pavel Stehule]

Hi

čt 13. 6. 2024 v 20:56 odesílatel Daniel Gustafsson 
napsal:

> > On 13 Jun 2024, at 19:21, Pavel Stehule  wrote:
>
> > Is the message "SQL expression ..." for RAW_PLPGSQL_EXPR correct?
>
> That indeed seems incorrect.
>
> > Should there  be a "PL/pgSQL expression" instead?
>
> I think that would make more sense.
>

here is the patch

Regards

Pavel


>
> --
> Daniel Gustafsson
>
>
From 3ad34d511f3b8f1df451cd86178da771692d4926 Mon Sep 17 00:00:00 2001
From: "ok...@github.com" 
Date: Sat, 15 Jun 2024 07:56:47 +0200
Subject: [PATCH] fix mesleading info about expression context

---
 src/backend/executor/spi.c   | 2 +-
 src/pl/plpgsql/src/expected/plpgsql_record.out   | 8 
 src/pl/plpgsql/src/expected/plpgsql_varprops.out | 2 +-
 src/test/regress/expected/plpgsql.out| 6 +++---
 4 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index e516c0a67c..a84e444a9a 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -2976,7 +2976,7 @@ _SPI_error_callback(void *arg)
 		switch (carg->mode)
 		{
 			case RAW_PARSE_PLPGSQL_EXPR:
-errcontext("SQL expression \"%s\"", query);
+errcontext("PL/pgSQL expression \"%s\"", query);
 break;
 			case RAW_PARSE_PLPGSQL_ASSIGN1:
 			case RAW_PARSE_PLPGSQL_ASSIGN2:
diff --git a/src/pl/plpgsql/src/expected/plpgsql_record.out b/src/pl/plpgsql/src/expected/plpgsql_record.out
index a9b5b778ef..6974c8f4a4 100644
--- a/src/pl/plpgsql/src/expected/plpgsql_record.out
+++ b/src/pl/plpgsql/src/expected/plpgsql_record.out
@@ -272,7 +272,7 @@ NOTICE:  r1.q1 = 
 NOTICE:  r1.q2 = 
 NOTICE:  r1 = 
 ERROR:  record "r1" has no field "nosuchfield"
-CONTEXT:  SQL expression "r1.nosuchfield"
+CONTEXT:  PL/pgSQL expression "r1.nosuchfield"
 PL/pgSQL function inline_code_block line 7 at RAISE
 -- records, not so much
 do $$
@@ -286,7 +286,7 @@ end$$;
 NOTICE:  r1 = 
 ERROR:  record "r1" is not assigned yet
 DETAIL:  The tuple structure of a not-yet-assigned record is indeterminate.
-CONTEXT:  SQL expression "r1.f1"
+CONTEXT:  PL/pgSQL expression "r1.f1"
 PL/pgSQL function inline_code_block line 5 at RAISE
 -- but OK if you assign first
 do $$
@@ -304,7 +304,7 @@ NOTICE:  r1.f1 = 1
 NOTICE:  r1.f2 = 2
 NOTICE:  r1 = (1,2)
 ERROR:  record "r1" has no field "nosuchfield"
-CONTEXT:  SQL expression "r1.nosuchfield"
+CONTEXT:  PL/pgSQL expression "r1.nosuchfield"
 PL/pgSQL function inline_code_block line 9 at RAISE
 -- check %type with block-qualified variable names
 do $$
@@ -598,7 +598,7 @@ create function getf3(x mutable) returns int language plpgsql as
 $$ begin return x.f3; end $$;
 select getf3(null::mutable);  -- doesn't work yet
 ERROR:  record "x" has no field "f3"
-CONTEXT:  SQL expression "x.f3"
+CONTEXT:  PL/pgSQL expression "x.f3"
 PL/pgSQL function getf3(mutable) line 1 at RETURN
 alter table mutable add column f3 int;
 select getf3(null::mutable);  -- now it works
diff --git a/src/pl/plpgsql/src/expected/plpgsql_varprops.out b/src/pl/plpgsql/src/expected/plpgsql_varprops.out
index 25115a02bd..958d7bca9a 100644
--- a/src/pl/plpgsql/src/expected/plpgsql_varprops.out
+++ b/src/pl/plpgsql/src/expected/plpgsql_varprops.out
@@ -76,7 +76,7 @@ begin
   raise notice 'x = %', x;
 end$$;
 ERROR:  division by zero
-CONTEXT:  SQL expression "1/0"
+CONTEXT:  PL/pgSQL expression "1/0"
 PL/pgSQL function inline_code_block line 2 during statement block local variable initialization
 do $$
 declare x bigint[] := array[1,3,5];
diff --git a/src/test/regress/expected/plpgsql.out b/src/test/regress/expected/plpgsql.out
index 074af8f33a..0a6945581b 100644
--- a/src/test/regress/expected/plpgsql.out
+++ b/src/test/regress/expected/plpgsql.out
@@ -2388,7 +2388,7 @@ begin
 end $$ language plpgsql;
 select namedparmcursor_test7();
 ERROR:  division by zero
-CONTEXT:  SQL expression "42/0 AS p1, 77 AS p2"
+CONTEXT:  PL/pgSQL expression "42/0 AS p1, 77 AS p2"
 PL/pgSQL function namedparmcursor_test7() line 6 at OPEN
 -- check that line comments work correctly within the argument list
 -- (this used to require a special hack in the code; it no longer does,
@@ -4563,11 +4563,11 @@ end
 $$;
 select fail();
 ERROR:  division by zero
-CONTEXT:  SQL expression "1/0"
+CONTEXT:  PL/pgSQL expression "1/0"
 PL/pgSQL function fail() line 3 at RETURN
 select fail();
 ERROR:  division by zero
-CONTEXT:  SQL expression "1/0"
+CONTEXT:  PL/pgSQL expression "1/0"
 PL/pgSQL function fail() line 3 at RETURN
 drop function fail();
 -- Test handling of string literals.
-- 
2.45.2