Re: Documentation: warn about two_phase when altering a subscription
The following review has been posted through the commitfest application: make installcheck-world: not tested Implements feature: tested, passed Spec compliant: not tested Documentation:tested, passed Hello, I've reviewed your patch, it applies correctly and the documentation builds without any errors. As for the content of the patch itself, I think so far it's good but would make two modifications. I like how the patch was worded originally when referring to the subscription, stating these parameters were 'in' the subscription rather than 'by' it. So I'd propose changing > parameters specified by the subscription. When creating the slot, ensure to > parameters specified in the subscription. When creating the slot, ensure and secondly the section "ensure the slot properties failover and two_phase match their counterpart parameters of the subscription" sounds a bit clunky. So I'd also propose changing: > the slot properties failover and > two_phase > match their counterpart parameters of the subscription. to > the slot properties failover and > two_phase > match their counterpart parameters in the subscription. I feel this makes the description flow a bit better when reading. But other than that I think it's quite clear. kind regards, --- Tristen Raab Highgo Software Canada www.highgo.ca
Re: Add minimal C example and SQL registration example for custom table access methods.
The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: not tested Spec compliant: not tested Documentation:tested, passed Hello, I've reviewed your patch and it applies correctly and the documentation builds without any error. The built documentation also looks good with no formatting errors. It's always great to see more examples when reading through documentation so I think this patch is a good addition. thanks, --- Tristen Raab Highgo Software Canada www.highgo.ca
Re: Better help output for pgbench -I init_steps
The following review has been posted through the commitfest application: make installcheck-world: not tested Implements feature: tested, passed Spec compliant: not tested Documentation:not tested Hello, I've reviewed all 4 of your patches, each one applies and builds correctly. > I think I prefer variant 2. Currently, we only have 8 steps, so it might > be overkill to separate them out into a different option. +1 to this from Peter. Variant 2 is nicely formatted with lots of information which I feel better solves the problem this patch is trying to address. Both versions 3 and 4 are a bit too jumbled for my liking without adding anything significant, even the shortened version 4. If we were to go with variant 1 however, I think it would add more to have a link to the pgbench documentation that refers to the different init steps. Perhaps on a new line just under where it says "see pgbench documentation for a description of these steps". Overall good patch, I'm a firm believer that more information is always better than less. Tristen --- Software Engineer HighGo Software Inc. (Canada) tristen.r...@highgo.ca www.highgo.ca
Re: Allow specifying a dbname in pg_basebackup connection string
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've reviewed your patch and it applies and builds without error. When testing this patch I was slightly confused as to what its purpose was, after testing it I now understand. Initially, I thought this was a change to add database-level replication. I would suggest some clarifications to the documentation such as changing: "supplying a specific database name in the connection string won't cause PostgreSQL to behave any differently." to "supplying a specific database name in the connection string won't cause pg_basebackup to behave any differently." I believe this better illustrates that we are referring to the actual pg_basebackup utility and how this parameter is only used for proxies and bears no impact on what pg_basebackup is actually doing. It also would remove any confusion about database replication I had prior. There is also a small typo in the same documentation: "However, if you are connecting to PostgreSQL through a proxy, then it's possible that this proxy does use the supplied databasename to make certain decisions, such as to which cluster to route the connection." "databasename" is just missing a space. Other than that, everything looks good. Regards, Tristen
Re: Correct the documentation for work_mem
The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: not tested Documentation:tested, passed Hello, I've reviewed and built the documentation for the updated patch. As it stands right now I think the documentation for this section is quite clear. > I'm wondering about adding "and more than one of these operations may > be in progress simultaneously". Are you talking about concurrent > sessions running other queries which are using work_mem too? This appears to be referring to the "sort and hash" operations mentioned prior. > If so, > isn't that already covered by the final sentence in the quoted text > above? if not, what is running simultaneously? I believe the last sentence is referring to another session that is running its own sort and hash operations. So the first section you mention is describing how sort and hash operations can be in execution at the same time for a query, while the second refers to how sessions may overlap in their execution of sort and hash operations if I am understanding this correctly. I also agree that changing "sort or hash" to "sort and hash" is a better description. Tristen
Re: pg_waldump: add test for coverage
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've reviewed your latest v3 patches on Ubuntu 23.04. Both patches apply correctly and all the tests run and pass as they should. Execution time was normal for me, I didn't notice any significant latency when compared to other tests. The only other feedback I can provide would be to add test coverage to some of the other options that aren't currently covered (ie. --bkp-details, --end, --follow, --path, etc.) for completeness. Other than that, this looks like a great patch. Kind regards, Tristen
Re: [PATCH] Allow Postgres to pick an unused port to listen
The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: not tested Documentation:tested, passed Hello Yurii, I've retested your latest patch and tested building the documentation. I agree with the general sentiment that this is an interesting, albeit specific feature. Nevertheless, I would still like to see this integrated. My only concern, like many others have voiced, is in regard to the port number. When I was reviewing this patch I found it quite unintuitive to rummage through the postmaster.pid to find the correct port. I think either a specific pg_ctl command to return the port like Greg had initially mentioned or simply a separate file to store the port numbers would be ideal. The standalone file being the simpler option, this would free up postmaster.pid to allow any future alterations and still be able to reliably get the port number when using this wildcard. We can also build on this file later to allow for multiple ports to be listened on as previously suggested. Kind regards, Tristen
