Re: Add regular expression testing for user name mapping in the peer authentication TAP test

2022-10-14 Thread Drouvot, Bertrand
Hi, On 10/15/22 5:11 AM, Michael Paquier wrote: On Fri, Oct 14, 2022 at 06:31:15PM +0200, Drouvot, Bertrand wrote: while working on [1], I thought it could also be useful to add regular expression testing for user name mapping in the peer authentication TAP test. Good idea now that we have a

Re: Eliminating SPI from RI triggers - take 2

2022-10-14 Thread Amit Langote
On Wed, Oct 12, 2022 at 2:27 AM Robert Haas wrote: > > On Thu, Sep 29, 2022 at 12:47 AM Amit Langote wrote: > > [ patches ] > > While looking over this thread I came across this code: Thanks for looking. > /* For data reading, executor always omits detached partitions */ > if

Re: CREATE COLLATION must be specified

2022-10-14 Thread Shay Rojansky
> > CREATE COLLATION some_collation (LC_COLLATE = 'en-u-ks-primary', > > LC_CTYPE = 'en-u-ks-primary', > > PROVIDER = icu, > > DETERMINISTIC = False > > ); > > > > This works on PG14, but on PG15 it errors with 'parameter "locale" must > > be specified'. > > > > I wanted to make

Re: thinko in basic_archive.c

2022-10-14 Thread Bharath Rupireddy
On Sat, Oct 15, 2022 at 12:03 AM Nathan Bossart wrote: > > On Fri, Oct 14, 2022 at 02:15:19PM +0530, Bharath Rupireddy wrote: > > Given that temp file name includes WAL file name, epoch to > > milliseconds scale and MyProcPid, can there be name collisions after a > > server crash or even when

Re: Add regular expression testing for user name mapping in the peer authentication TAP test

2022-10-14 Thread Michael Paquier
On Fri, Oct 14, 2022 at 06:31:15PM +0200, Drouvot, Bertrand wrote: > while working on [1], I thought it could also be useful to add regular > expression testing for user name mapping in the peer authentication TAP > test. Good idea now that we have a bit more coverage in the authentication tests.

Re: New "single-call SRF" APIs are very confusingly named

2022-10-14 Thread Michael Paquier
On Fri, Oct 14, 2022 at 05:09:46PM -0400, Melanie Plageman wrote: > To summarize, I am in support of renaming SetSingleFuncCall() -> > InitMaterializedSRF() and SRF_SINGLE_USE_EXPECTED -> > MAT_SRF_USE_EXPECTED_TUPLE_DESC (or just DESC) as suggested elsewhere in > this thread. And I think we

Re: make_ctags: use -I option to ignore pg_node_attr macro

2022-10-14 Thread Tatsuo Ishii
> On Thu, 13 Oct 2022 15:35:09 +0900 (JST) > Tatsuo Ishii wrote: > >> > OK, that sounds good then. I would make a feature request to have a >> > switch that supresses creation of these links, then. >> >> Ok, I have added "-n" option to make_ctags so that it skips to create >> the links. >> >>

Re: predefined role(s) for VACUUM and ANALYZE

2022-10-14 Thread Corey Huinker
> > Sounds good. Here's a new patch set with aclitem's typalign fixed. > Patch applies. Passes make check and make check-world. Test coverage seems adequate. Coding is very clear and very much in the style of the existing code. Any quibbles I have with the coding style are ones I have with the

Re: Refactor to introduce pg_strcoll().

2022-10-14 Thread Jeff Davis
On Thu, 2022-10-13 at 10:57 +0200, Peter Eisentraut wrote: > It's a bit confusing that arguments must be NUL-terminated, but the > length is still specified.  Maybe another sentence to explain that > would > be helpful. Added a comment. It was a little frustrating to get a perfectly clean API,

Re: Avoid memory leaks during base backups

2022-10-14 Thread Cary Huang
The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: not tested Documentation:not tested Hello I applied your v5 patch on the current master and run

Re: New docs chapter on Transaction Management and related changes

2022-10-14 Thread Robert Treat
On Fri, Oct 14, 2022 at 3:51 PM Bruce Momjian wrote: > Attached is the merged patch from all the great comments I received. I > have also rebuilt the docs with the updated patch: > > https://momjian.us/tmp/pgsql/ > + RELEASE SAVEPOINT also subcommits and destroys + all savepoints

Re: archive modules

2022-10-14 Thread Nathan Bossart
On Fri, Oct 14, 2022 at 11:51:30AM -0700, Nathan Bossart wrote: > On Fri, Oct 14, 2022 at 12:10:18PM +0530, Bharath Rupireddy wrote: >> 2) I think we have a problem - set archive_mode and archive_library >> and start the server, then set archive_command, reload the conf, see >> [3] - the archiver

[PATCH] comment fixes for delayChkptFlags

2022-10-14 Thread David Christensen
Enclosed is a trivial fix for a typo and misnamed field I noted when doing some code review. Best, David 0001-fix-comment-typos-for-delayChkptFlags.patch Description: Binary data

Re: New "single-call SRF" APIs are very confusingly named

2022-10-14 Thread Tom Lane
Melanie Plageman writes: > So, while I agree that the "Single" in SetSingleFuncCall() could be > confusing given the name of ExprSingleResult, I feel like actually all > of the names are somewhat wrong. Maybe, but ExprSingleResult et al. have been there for decades and are certainly embedded in

Re: New "single-call SRF" APIs are very confusingly named

2022-10-14 Thread Melanie Plageman
On Thu, Oct 13, 2022 at 3:48 PM Andres Freund wrote: > I unfortunately just noticed this now, just after we released... > > In > > commit 9e98583898c347e007958c8a09911be2ea4acfb9 > Author: Michael Paquier > Date: 2022-03-07 10:26:29 +0900 > > Create routine able to set single-call SRFs for

Re: Add index scan progress to pg_stat_progress_vacuum

2022-10-14 Thread Imseih (AWS), Sami
Thank you for the feedback! >While it seems to be a good idea to have the atomic counter for the >number of indexes completed, I think we should not use the global >variable referencing the counter as follow: >+static pg_atomic_uint32 *index_vacuum_completed = NULL; >: >

Re: New docs chapter on Transaction Management and related changes

2022-10-14 Thread Bruce Momjian
On Fri, Oct 14, 2022 at 12:22:35PM +0100, Simon Riggs wrote: > > > + > > > +The parent xid of each subxid is recorded in the > > > +pg_subtrans directory. No entry is made for > > > +top-level xids since they do not have a parent, nor is an entry made > > > +for read-only

Re: New docs chapter on Transaction Management and related changes

2022-10-14 Thread Bruce Momjian
On Fri, Oct 14, 2022 at 10:46:15AM +0200, Álvaro Herrera wrote: > +1 for this new chapter. This latest patch looks pretty good. I think > that introducing the concept of "sub-commit" as in Simon's follow-up > patch clarifies things, though the word itself looks very odd. Maybe > it's okay. The

Re: New docs chapter on Transaction Management and related changes

2022-10-14 Thread Bruce Momjian
On Fri, Oct 14, 2022 at 01:05:14PM +0100, Simon Riggs wrote: > On Fri, 14 Oct 2022 at 08:55, Simon Riggs > wrote: > > > In related changes, I've also done some major rewording of the RELEASE > > SAVEPOINT command, since it was incorrectly described as having "no > > other user visible

Re: New docs chapter on Transaction Management and related changes

2022-10-14 Thread Bruce Momjian
On Fri, Oct 14, 2022 at 08:55:05AM +0100, Simon Riggs wrote: > On Thu, 13 Oct 2022 at 23:06, Bruce Momjian wrote: > > > > Thanks, updated patch attached. You can see the output at: > > Thank you for your work to tighten and cleanup this patch, much appreciated. > > I had two minor typos, plus

Re: Question about pull_up_sublinks_qual_recurse

2022-10-14 Thread Tom Lane
Andy Fan writes: > After some more self review, I find my proposal has the following side > effects. Yeah, I do not think this works at all. The mechanism as it stands right now is that we can insert pulled-up semijoins above j->larg if they only need variables from relations in j->larg, and

Re: Tracking last scan time

2022-10-14 Thread Dave Page
On Fri, 14 Oct 2022 at 19:16, Andres Freund wrote: > Hi, > > On 2022-10-13 14:38:06 +0100, Dave Page wrote: > > Thanks for that. It looks good to me, bar one comment (repeated 3 times > in > > the sql and expected files): > > > > fetch timestamps from before the next test > > > > "from " should

Re: archive modules

2022-10-14 Thread Nathan Bossart
On Fri, Oct 14, 2022 at 12:10:18PM +0530, Bharath Rupireddy wrote: > +ereport(ERROR, > +(errcode(ERRCODE_INVALID_PARAMETER_VALUE), > + errmsg("both archive_command and archive_library > specified"), > + errdetail("Only one of

Re: thinko in basic_archive.c

2022-10-14 Thread Nathan Bossart
On Fri, Oct 14, 2022 at 02:15:19PM +0530, Bharath Rupireddy wrote: > Given that temp file name includes WAL file name, epoch to > milliseconds scale and MyProcPid, can there be name collisions after a > server crash or even when multiple servers with different pids are > archiving/copying the same

Re: hash_xlog_split_allocate_page: failed to acquire cleanup lock

2022-10-14 Thread Andres Freund
Hi, On 2022-10-14 10:40:11 +0530, Amit Kapila wrote: > On Fri, Oct 14, 2022 at 2:25 AM Andres Freund wrote: > > > > > > > > Attached are two patches. The first patch is what Robert has proposed > > > with some changes in comments to emphasize the fact that cleanup lock > > > on the new bucket is

Re: Tracking last scan time

2022-10-14 Thread Andres Freund
Hi, On 2022-10-13 14:38:06 +0100, Dave Page wrote: > Thanks for that. It looks good to me, bar one comment (repeated 3 times in > the sql and expected files): > > fetch timestamps from before the next test > > "from " should be removed. I was trying to say something with that from, but clearly

Re: Bug: pg_regress makefile does not always copy refint.so

2022-10-14 Thread Alvaro Herrera
On 2022-Oct-14, Donghang Lin wrote: > Hi hackers, > > When I was building pg_regress, it didn’t always copy a rebuilt version of > refint.so to the folder. > > Steps to reproduce: > OS: centos7 > PSQL version: 14.5 > > 1. configure and build postgres > 2. edit file src/include/utils/rel.h so

Re: [PATCH] Reset single-row processing mode at end of pipeline commands queue

2022-10-14 Thread Alvaro Herrera
Hello Denis, On 2022-Oct-07, Denis Laxalde wrote: > I'm trying to make single-row mode and pipeline mode work together in > Psycopg using libpq. I think there is something wrong with respect to the > single-row mode flag, not being correctly reset, in some situations. > > The minimal case I'm

Re: dynamic result sets support in extended query protocol

2022-10-14 Thread Pavel Stehule
Hi pá 14. 10. 2022 v 9:12 odesílatel Peter Eisentraut < peter.eisentr...@enterprisedb.com> napsal: > On 01.02.22 15:40, Peter Eisentraut wrote: > > On 12.01.22 11:20, Julien Rouhaud wrote: > >> Since you mentioned that this patch depends on the SHOW_ALL_RESULTS > >> psql patch > >> which is

Add regular expression testing for user name mapping in the peer authentication TAP test

2022-10-14 Thread Drouvot, Bertrand
Hi hackers, while working on [1], I thought it could also be useful to add regular expression testing for user name mapping in the peer authentication TAP test. This kind of test already exists in kerberos/t/001_auth.pl but the proposed one in the peer authentication testing would probably

Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher

2022-10-14 Thread Önder Kalacı
Hi, Thanks for the review! Here are some comments on v17 patch. > > 1. > -LogicalRepRelMapEntry * > +LogicalRepPartMapEntry * > logicalrep_partition_open(LogicalRepRelMapEntry *root, > Relation partrel, > AttrMap *map) > { > > Is there any

Re: Error for WITH options on partitioned tables

2022-10-14 Thread Karina Litskevich
Hi, Simon! The new error message looks better. But I believe it is better to use "parameters" instead of "options" as it is called "storage parameters" in the documentation. I also believe it is better to report error in partitioned_table_reloptions() (thanks to Japin Li for mentioning this

Re: Fix error message for MERGE foreign tables

2022-10-14 Thread Tom Lane
Richard Guo writes: > Or maybe we can make it even earlier, when we expand an RTE for a > partitioned table and add result tables to leaf_result_relids. I'm not really on board with injecting command-type-specific logic into completely unrelated places just so that we can throw an error a bit

Re: [PATCH] Improve tab completion for ALTER TABLE on identity columns

2022-10-14 Thread Dagfinn Ilmari Mannsåker
Dagfinn Ilmari Mannsåker writes: > Hi Hackers, > > I noticed that psql has no tab completion around identity columns in > ALTER TABLE, so here's some patches for that. Added to the next commit fest: https://commitfest.postgresql.org/40/3947/ - ilmari

Re: Move backup-related code to xlogbackup.c/.h

2022-10-14 Thread Bharath Rupireddy
On Fri, Oct 14, 2022 at 1:54 PM Alvaro Herrera wrote: > > On 2022-Oct-13, Bharath Rupireddy wrote: > > > The pg_backup_start_callback() can just go ahead and reset > > sessionBackupState. However, it leads us to the complete removal of > > pg_backup_start_callback() itself and use

Re: Fix error message for MERGE foreign tables

2022-10-14 Thread Richard Guo
On Fri, Oct 14, 2022 at 7:19 PM Richard Guo wrote: > > On Fri, Oct 14, 2022 at 5:24 PM Alvaro Herrera > wrote: > >> Actually, I hadn't realized that the originally submitted patch had the >> test in postgres_fdw only, but we really want it to catch any FDW, so it >> needs to be somewhere more

Re: Patch proposal: make use of regular expressions for the username in pg_hba.conf

2022-10-14 Thread Drouvot, Bertrand
Hi, On 10/14/22 7:30 AM, Michael Paquier wrote: On Wed, Oct 12, 2022 at 08:17:14AM +0200, Drouvot, Bertrand wrote: Indeed, ;-) So, I have spent the last two days looking at all that, studying the structure of the patch and the existing HEAD code, Thanks! The code could be split to tackle

Re: Patch proposal: make use of regular expressions for the username in pg_hba.conf

2022-10-14 Thread Drouvot, Bertrand
Hi, On 10/14/22 8:18 AM, Michael Paquier wrote: On Fri, Oct 14, 2022 at 02:30:25PM +0900, Michael Paquier wrote: First, as of HEAD, AuthToken is only used for elements in a list of role and database names in hba.conf before filling in each HbaLine, hence we limit its usage to the initial

Re: New docs chapter on Transaction Management and related changes

2022-10-14 Thread Simon Riggs
On Fri, 14 Oct 2022 at 08:55, Simon Riggs wrote: > In related changes, I've also done some major rewording of the RELEASE > SAVEPOINT command, since it was incorrectly described as having "no > other user visible behavior". A complex example is given to explain, > using the terminology

Re: New docs chapter on Transaction Management and related changes

2022-10-14 Thread Simon Riggs
On Fri, 14 Oct 2022 at 09:46, Alvaro Herrera wrote: > > +1 for this new chapter. This latest patch looks pretty good. I think > that introducing the concept of "sub-commit" as in Simon's follow-up > patch clarifies things, though the word itself looks very odd. Maybe > it's okay. The addition

Re: Fix error message for MERGE foreign tables

2022-10-14 Thread Richard Guo
On Fri, Oct 14, 2022 at 5:24 PM Alvaro Herrera wrote: > Actually, I hadn't realized that the originally submitted patch had the > test in postgres_fdw only, but we really want it to catch any FDW, so it > needs to be somewhere more general. The best place I found to put this > test is in

Re: Improve description of XLOG_RUNNING_XACTS

2022-10-14 Thread Bharath Rupireddy
On Mon, Oct 3, 2022 at 1:46 PM Michael Paquier wrote: > > On Fri, Sep 16, 2022 at 10:55:53AM +0900, Kyotaro Horiguchi wrote: > > Putting an arbitrary upper-bound on the number of subxids to print > > might work? I'm not sure how we can determine the upper-bound, though. > > You could hardcode it

RE: Perform streaming logical transactions by background workers and parallel apply

2022-10-14 Thread houzj.f...@fujitsu.com
On Wed, Oct 12, 2022 at 18:11 PM Peter Smith wrote: > Here are some review comments for v36-0001. Thanks for your comments. > == > > 1. GENERAL > > Houzj wrote ([1] #3a): > The word 'streaming' is the same as the actual option name, so > personally I think it's fine. But if others also

Re: Fix error message for MERGE foreign tables

2022-10-14 Thread Alvaro Herrera
Actually, I hadn't realized that the originally submitted patch had the test in postgres_fdw only, but we really want it to catch any FDW, so it needs to be somewhere more general. The best place I found to put this test is in make_modifytable ... I searched for some earlier place in the planner

pub/sub - specifying optional parameters without values.

2022-10-14 Thread Peter Smith
Hi hackers. This post is about parameter default values. Specifically. it's about the CREATE PUBLICATION and CREATE SUBSCRIPTION syntax, although the same issue might apply to other commands I am unaware of... ~~~ Background: CREATE PUBLICATION syntax has a WITH clause: [ WITH (

Re: Fix error message for MERGE foreign tables

2022-10-14 Thread Alvaro Herrera
On 2022-Oct-14, Michael Paquier wrote: > On Fri, Oct 14, 2022 at 12:26:19PM +0800, Richard Guo wrote: > > Maybe something like below, so that we keep it consistent with the case > > of a foreign table being specified as a target. > > > > --- a/contrib/postgres_fdw/postgres_fdw.c > > +++

Re: New docs chapter on Transaction Management and related changes

2022-10-14 Thread Alvaro Herrera
+1 for this new chapter. This latest patch looks pretty good. I think that introducing the concept of "sub-commit" as in Simon's follow-up patch clarifies things, though the word itself looks very odd. Maybe it's okay. The addition of the savepoint example looks good also. On 2022-Oct-13,

Re: thinko in basic_archive.c

2022-10-14 Thread Bharath Rupireddy
On Fri, Oct 14, 2022 at 10:11 AM Nathan Bossart wrote: > > I intended for the temporary file name generated by basic_archive.c to I'm trying to understand this a bit: /* * Pick a sufficiently unique name for the temporary file so that a * collision is unlikely. This helps avoid

Re: Fix error message for MERGE foreign tables

2022-10-14 Thread Michael Paquier
On Fri, Oct 14, 2022 at 12:26:19PM +0800, Richard Guo wrote: > Maybe something like below, so that we keep it consistent with the case > of a foreign table being specified as a target. > > --- a/contrib/postgres_fdw/postgres_fdw.c > +++ b/contrib/postgres_fdw/postgres_fdw.c > @@ -1872,6 +1872,13

Re: Move backup-related code to xlogbackup.c/.h

2022-10-14 Thread Michael Paquier
On Fri, Oct 14, 2022 at 10:24:41AM +0200, Alvaro Herrera wrote: > However, what's most problematic about this patch is that it introduces > a pretty serious bug, yet that bug goes unnoticed if you just run the > builtin test suites. I only noticed because I added an elog(ERROR, > "oops") in the

Re: Move backup-related code to xlogbackup.c/.h

2022-10-14 Thread Alvaro Herrera
On 2022-Oct-13, Bharath Rupireddy wrote: > The pg_backup_start_callback() can just go ahead and reset > sessionBackupState. However, it leads us to the complete removal of > pg_backup_start_callback() itself and use do_pg_abort_backup() > consistently across, saving 20 LOC attached as v5-0001.

Re: Question about pull_up_sublinks_qual_recurse

2022-10-14 Thread Andy Fan
> > Later we tried to pull up the EXISTS sublink to t1 OR t2 *separately*, > since > this subselect referenced to t1 *AND* t2, so we CAN'T pull up the > sublink. I > am thinking why we have to pull up it t1 OR t2 rather than JoinExpr(t1, > t2), > I think the latter one is better. > After some

Re: New docs chapter on Transaction Management and related changes

2022-10-14 Thread Simon Riggs
On Thu, 13 Oct 2022 at 23:06, Bruce Momjian wrote: > > Thanks, updated patch attached. You can see the output at: Thank you for your work to tighten and cleanup this patch, much appreciated. I had two minor typos, plus a slight rewording to avoid using the word "global" in the last section,

refactor ownercheck and aclcheck functions

2022-10-14 Thread Peter Eisentraut
These patches take the dozens of mostly-duplicate pg_foo_ownercheck() and pg_foo_aclcheck() functions and replace (most of) them by common functions that are driven by the ObjectProperty table. All the required information is already in that table. This is similar to the consolidation of the

Re: PG upgrade 14->15 fails - database contains our own extension

2022-10-14 Thread David Turoň
Hi, I really appreciate your help and very quick response. And WOW, write patch for this in few hours ...that's amazing! > Looking closer, I don't see how b55f2b692 could have changed pg_dump's > opinion of the order to sort these three casts in; that sort ordering > logic is old enough to

Re: [PoC] Improve dead tuple storage for lazy vacuum

2022-10-14 Thread Masahiko Sawada
Hi, On Mon, Oct 10, 2022 at 2:16 PM John Naylor wrote: > > The following is not quite a full review, but has plenty to think about. > There is too much to cover at once, and I have to start somewhere... > > My main concerns are that internal APIs: > > 1. are difficult to follow > 2. lead to

Re: dynamic result sets support in extended query protocol

2022-10-14 Thread Peter Eisentraut
On 01.02.22 15:40, Peter Eisentraut wrote: On 12.01.22 11:20, Julien Rouhaud wrote: Since you mentioned that this patch depends on the SHOW_ALL_RESULTS psql patch which is still being worked on, I'm not expecting much activity here until the prerequirements are done.  It also seems better to

Re: archive modules

2022-10-14 Thread Bharath Rupireddy
On Fri, Oct 14, 2022 at 6:00 AM Michael Paquier wrote: > > On Thu, Oct 13, 2022 at 02:53:38PM -0400, Tom Lane wrote: > > Yeah, it really does not work to use GUC hooks to enforce multi-variable > > constraints. We've learned that the hard way (more than once, if memory > > serves). > > 414c2fd

Bug: pg_regress makefile does not always copy refint.so

2022-10-14 Thread Donghang Lin
Hi hackers, When I was building pg_regress, it didn’t always copy a rebuilt version of refint.so to the folder. Steps to reproduce: OS: centos7 PSQL version: 14.5 1. configure and build postgres 2. edit file src/include/utils/rel.h so that contrib/spi will rebuild 3. cd

Re: Patch proposal: make use of regular expressions for the username in pg_hba.conf

2022-10-14 Thread Michael Paquier
On Fri, Oct 14, 2022 at 02:30:25PM +0900, Michael Paquier wrote: > First, as of HEAD, AuthToken is only used for elements in a list of > role and database names in hba.conf before filling in each HbaLine, > hence we limit its usage to the initial parsing. The patch assigns an > optional regex_t