On Sun, May 26, 2019 at 2:20 AM Alexander Lakhin <exclusion@gmail.com> wrote:
>
> Hello hackers,
>
> Please also consider fixing the following inconsistencies found in new
> v12 code:
>
> 1. AT_AddOids - remove (orphaned after 578b2297)
> 2. BeingModified ->TM_BeingModified (for consistency)
>
/*
- * A tuple is locked if HTSU returns BeingModified.
+ * A tuple is locked if HTSU returns TM_BeingModified.
*/
if (htsu == TM_BeingModified)
I think the existing comment is quite clear. I mean we can change it
if we want, but I don't see the dire need to do it.
> 3. copy_relation_data -> remove (orphaned after d25f5191)
- * reason is the same as in tablecmds.c's copy_relation_data(): we're
- * writing data that's not in shared buffers, and so a CHECKPOINT
- * occurring during the rewriteheap operation won't have fsync'd data we
- * wrote before the checkpoint.
+ * reason is that we're writing data that's not in shared buffers, and
+ * so a CHECKPOINT occurring during the rewriteheap operation won't
+ * have fsync'd data we wrote before the checkpoint.
It seems to me that the same thing is moved to storage.c's
RelationCopyStorage() in the commit mentioned by you. So, isn't it
better to change the comment accordingly rather than entirely removing
the reference to a similar comment in another place?
> 4. endblock -> endblk (an internal inconsistency)
> 5. ExecContextForcesOids - not changed, but may be should be removed
> (orphaned after 578b2297)
Yes, we should remove the use of ExecContextForcesOids. We are using
es_result_relation_info at other places as well, so I think we can
change the comment to indicate the same.
>
> The separate patches for all the defects (except 5) are attached. In
> case a single patch is preferable, I can produce it too.
>
It is okay, we can review the individual patches and then combine them
later. I couldn't get a chance to review all of them yet. Thanks
for working on this.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com