Horton

Thread

Commits

  1. Fix typos and inconsistencies in code comments

  1. Fix inconsistencies for v12 (pass 2)

    Alexander Law <exclusion@gmail.com> — 2019-06-12T14:34:06Z

    Hello Amit,

Can you also review the following fixes?:
2.1. bt_binsrch_insert -> _bt_binsrch_insert (an internal inconsistency)
2.2. EWOULDBOCK -> EWOULDBLOCK (a typo)
2.3. FORGET_RELATION_FSYNC & FORGET_DATABASE_FSYNC ->
SYNC_FORGET_REQUEST (orphaned after 3eb77eba)
2.4. GetNewObjectIdWithIndex -> GetNewOidWithIndex (an internal
inconsistency)
2.5. get_opclass_family_and_input_type ->
get_opclass_opfamily_and_input_type (an internal inconsistency)
2.6. HAVE_BUILTIN_CLZ -> HAVE__BUILTIN_CLZ (missing underscore)
2.7. HAVE_BUILTIN_CTZ -> HAVE__BUILTIN_CTZ (missing underscore)
2.8. MultiInsertInfoNextFreeSlot -> CopyMultiInsertInfoNextFreeSlot (an
internal inconsistency)
2.9. targetIsArray -> targetIsSubscripting (an internal inconsistency)
2.10. tss_htup -> remove (orphaned after 2e3da03e)

I can't see another inconsistencies for v12 for now, but there are some
that appeared before.
If this work can be performed more effectively or should be
postponed/canceled, please let me know.

Best regards,
Alexander

  2. Re: Fix inconsistencies for v12 (pass 2)

    Michael Paquier <michael@paquier.xyz> — 2019-06-13T08:10:07Z

    On Wed, Jun 12, 2019 at 05:34:06PM +0300, Alexander Lakhin wrote:
> I can't see another inconsistencies for v12 for now, but there are some
> that appeared before.
> If this work can be performed more effectively or should be
> postponed/canceled, please let me know.

Note sure that it is much productive to have one patch with basically 
one-liners in each one...  Anyway..

All your suggestions are right.  I do have one doubt for the
suggestion in execnodes.h:
@@ -1571,7 +1571,6 @@ typedef struct TidScanState
    int         tss_NumTids;
    int         tss_TidPtr;
    ItemPointerData *tss_TidList;
-   HeapTupleData tss_htup;
} TidScanState;
The last trace of tss_htup has been removed as of 2e3da03, and I see
no mention of it in the related thread.  Andres, is that intentional
for table AMs to keep a trace of a currently-fetched tuple for a TID
scan or something that can be removed?  The field is still
documented, so the patch is incomplete if we finish by removing the
field.  And my take is that we should keep it.
--
Michael

  3. Re: Fix inconsistencies for v12 (pass 2)

    Alexander Law <exclusion@gmail.com> — 2019-06-13T08:28:42Z

    Hello Michael,
13.06.2019 11:10, Michael Paquier wrote:
> On Wed, Jun 12, 2019 at 05:34:06PM +0300, Alexander Lakhin wrote:
>> I can't see another inconsistencies for v12 for now, but there are some
>> that appeared before.
>> If this work can be performed more effectively or should be
>> postponed/canceled, please let me know.
> Note sure that it is much productive to have one patch with basically 
> one-liners in each one...  Anyway..
As the proposed fixes are independent, I decided to separate them. I
will make a single patch on next iteration.
> All your suggestions are right.  I do have one doubt for the
> suggestion in execnodes.h:
> @@ -1571,7 +1571,6 @@ typedef struct TidScanState
>     int         tss_NumTids;
>     int         tss_TidPtr;
>     ItemPointerData *tss_TidList;
> -   HeapTupleData tss_htup;
> } TidScanState;
> The last trace of tss_htup has been removed as of 2e3da03, and I see
> no mention of it in the related thread.  Andres, is that intentional
> for table AMs to keep a trace of a currently-fetched tuple for a TID
> scan or something that can be removed?  The field is still
> documented, so the patch is incomplete if we finish by removing the
> field.  And my take is that we should keep it.
Yes, you're right. I've completed the patch for a possible elimination
of the field.

Best regards,
Alexander

  4. Re: Fix inconsistencies for v12 (pass 2)

    Michael Paquier <michael@paquier.xyz> — 2019-06-14T00:42:31Z

    On Thu, Jun 13, 2019 at 11:28:42AM +0300, Alexander Lakhin wrote:
> Yes, you're right. I've completed the patch for a possible elimination
> of the field.

For now I have discarded this one, and committed the rest as the
inconsistencies stand out.  Good catches by the way.

Your patch was actually incorrect in checkpointer.c.  3eb77eb has
refactored the fsync queue and has removed FORGET_DATABASE_FSYNC, but
it has been replaced by SYNC_FILTER_REQUEST as equivalent in the
shared queue to forget database-level stuff.
--
Michael

  5. Re: Fix inconsistencies for v12 (pass 2)

    Alexander Law <exclusion@gmail.com> — 2019-06-14T04:16:15Z

    Hello,
13.06.2019 11:10, Michael Paquier wrote:
> The last trace of tss_htup has been removed as of 2e3da03, and I see
> no mention of it in the related thread.  Andres, is that intentional
> for table AMs to keep a trace of a currently-fetched tuple for a TID
> scan or something that can be removed?  The field is still
> documented, so the patch is incomplete if we finish by removing the
> field.  And my take is that we should keep it.
Andres, I've found another unused structure field "was_xmin" in the
was_running structure, having the following comment:
* Outdated: This struct isn't used for its original purpose anymore, but
* can't be removed / changed in a minor version, because it's stored
* on-disk.
This comment lives here since 955a684e, May 13 2017. Shouldn't the
outdated structure be removed in v12?

Best regards,
Alexander