Обсуждение: Fix inconsistencies for v12 (pass 2)

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

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

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

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

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