On Tue, Feb 9, 2021 at 12:02 PM Peter Smith <smithpb2...@gmail.com> wrote: > > Here are my feedback comments for the V29 patch. >
Thanks. > > 3. > Previously the tablesync origin name format was encapsulated in a > common function. IMO it was cleaner/safer how it was before, instead > of the same "pg_%u_%u" cut/paste and scattered in many places. > (same comment applies multiple places, in this file and in tablesync.c) > > 4. > Calls like replorigin_drop_by_name(originname, true, false); make it > unnecessarily hard to read code when the boolean params are neither > named as variables nor commented. I noticed on another thread [et0205] > there was an idea that having no name/comments is fine because anyway > it is not difficult to figure out when using a "modern IDE", but since > my review tools are only "vi" and "meld" I beg to differ with that > justification. > (same comment applies multiple places, in this file and in tablesync.c) > It would be a bit convenient for you but for most others, I think it would be noise. Personally, I find the code more readable without such name comments, it just breaks the flow of code unless you want to study in detail the value of each param. > [et0205] > https://www.postgresql.org/message-id/c1d9833f-eeeb-40d5-89ba-87674e1b7ba3%40www.fastmail.com > > ===== > > FILE: tablesync.c > > 5. > Previously there was a function tablesync_replorigin_drop which was > encapsulating the tablesync origin name formatting. I thought that was > better than the V29 code which now has the same formatting scattered > over many places. > (same comment applies for worker_internal.h) > Isn't this the same as what you want to say in point-3? > > 7. > Maybe consider to just assign GetSystemIdentifier() to a static > instead of calling that function for every slot? > static uint64 sysid = GetSystemIdentifier(); > IIUC the sysid value is never going to change for a process, right? > That's right but I am not sure if there is much value in saving one call here by introducing extra variable. I'll fix other comments raised by you. -- With Regards, Amit Kapila.
