Hi Timur. Thanks for the patches you provided. My replies are inline below.
On Sat, Aug 16, 2025 at 3:45 AM Timur Magomedov <t.magome...@postgrespro.ru> wrote: > > On Thu, 2025-08-14 at 11:23 +1000, Peter Smith wrote: > > Here are the latest v18* patches. > > > > Hi Peter! > I've reworked my recent patch [1] so it is now based on v18 and is > divided into several simpler patches. Here they are plus one additional > patch. > > 0001-Fixed-comment-and-guard-name-in-vci_pg_copy.h.patch > Looks like vci_pg_copy.h was renamed from vci_numeric.h but file name > comment and define guard name were not updated. Fixed it. In v19 I intend to merge vci_pg_copy.h into postgresql_copy.h, so this is moot. > > 0002-Removed-vci_set_merge_and_copy_trans_funcs.patch > Found that vci_set_merge_and_copy_trans_funcs() is not used anywhere, > removed it alogn with the code that was only called inside it. > trans_funcs_table[] now only contains single transfn_oid field, others > (unused) are removed. > > 0003-Replaced-linear-search-by-switch-case.patch > Replaced linear search inside trans_funcs_table array to more optimal > switch-case. Thanks, these 0002 and 0003 changes will be in v19 patches which I'm hoping to post tomorrow or the day after. > > 0004-Removed-worker-name-check-in-lock.c.patch > This is one I'm not sure about. > Found that changes in Postgres core lock.c file check for "backend=" > substring in background worker name. There is also a comment in > vci_ros_daemon.c mentioning bgw_name checks of LockAquire(). Names > don't match however. So as far as I understand the check for "backend=" > in name is always false since no code in VCI sets bgw_name to something > similar. > This is either forgotten feature that can be easily fixed by removing > bgw_name checks, either some bug, either my misunderstanding. > For the first case, here is a patch that removes bgw_name checks in > lock.c. It makes core patch a bit smaller and not touching lock.c at > all (Yay!). > There is code in the function vci_LaunchROSControlWorker() which does a sprintf to assign the worker.bgw_name, but I also do not see how LockAcquire can have a bgw_name containing string “backend=”. Frankly, I expected the patch 0001 code should say more like strstr(…, “vci:”) because otherwise it does not seem VCI specific. Indeed, if it was checking “vci:” then the comment in storage/vci_ros_daemon.c would make sense to me. So, I agree that the Acquire/ReleaseLock code seems like it might be unreachable, OTOH, I am reluctant to remove it without understanding more about what was the intended purpose in the first place. Checking… ====== Kind Regards, Peter Smith. Fujitsu Australia
