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
Attachments:
bt_binsrch_insert.patchtext/x-patch; name=bt_binsrch_insert.patchDownload
diff --git a/src/backend/access/nbtree/nbtsearch.c b/src/backend/access/nbtree/nbtsearch.c
index 1f809c24a1..c655dadb96 100644
--- a/src/backend/access/nbtree/nbtsearch.c
+++ b/src/backend/access/nbtree/nbtsearch.c
@@ -424,7 +424,7 @@ _bt_binsrch(Relation rel,
/*
*
- * bt_binsrch_insert() -- Cacheable, incremental leaf page binary search.
+ * _bt_binsrch_insert() -- Cacheable, incremental leaf page binary search.
*
* Like _bt_binsrch(), but with support for caching the binary search
* bounds. Only used during insertion, and only on the leaf page that it
EWOULDBOCK.patchtext/x-patch; name=EWOULDBOCK.patchDownload
diff --git a/src/backend/libpq/be-secure-gssapi.c b/src/backend/libpq/be-secure-gssapi.c
index 1673b10315..ba8c0cd0f0 100644
--- a/src/backend/libpq/be-secure-gssapi.c
+++ b/src/backend/libpq/be-secure-gssapi.c
@@ -400,7 +400,7 @@ read_or_wait(Port *port, ssize_t len)
{
/*
* If we got back less than zero, indicating an error, and that
- * wasn't just a EWOULDBOCK/EAGAIN, then give up.
+ * wasn't just a EWOULDBLOCK/EAGAIN, then give up.
*/
if (ret < 0 && !(errno == EWOULDBLOCK || errno == EAGAIN))
return -1;
FORGET_RELATION_FSYNC.patchtext/x-patch; name=FORGET_RELATION_FSYNC.patchDownload
diff --git a/src/backend/postmaster/checkpointer.c b/src/backend/postmaster/checkpointer.c
index 13f152b473..6ceff63ae1 100644
--- a/src/backend/postmaster/checkpointer.c
+++ b/src/backend/postmaster/checkpointer.c
@@ -1213,8 +1213,8 @@ CompactCheckpointerRequestQueue(void)
* backwards from the end of the queue and check whether a request is
* *preceded* by an earlier, identical request, in the hopes of doing less
* copying. But that might change the semantics, if there's an
- * intervening FORGET_RELATION_FSYNC or FORGET_DATABASE_FSYNC request, so
- * we do it this way. It would be possible to be even smarter if we made
+ * intervening SYNC_FORGET_REQUEST, so we do it this way.
+ * It would be possible to be even smarter if we made
* the code below understand the specific semantics of such requests (it
* could blow away preceding entries that would end up being canceled
* anyhow), but it's not clear that the extra complexity would buy us
get_opclass_family_and_input_type.patchtext/x-patch; name=get_opclass_family_and_input_type.patchDownload
diff --git a/src/backend/utils/cache/lsyscache.c b/src/backend/utils/cache/lsyscache.c
index b4f2d0f35a..c13c08a97b 100644
--- a/src/backend/utils/cache/lsyscache.c
+++ b/src/backend/utils/cache/lsyscache.c
@@ -1055,7 +1055,7 @@ get_opclass_input_type(Oid opclass)
}
/*
- * get_opclass_family_and_input_type
+ * get_opclass_opfamily_and_input_type
*
* Returns the OID of the operator family the opclass belongs to,
* the OID of the datatype the opclass indexes
GetNewObjectIdWithIndex.patchtext/x-patch; name=GetNewObjectIdWithIndex.patchDownload
diff --git a/src/backend/catalog/catalog.c b/src/backend/catalog/catalog.c
index 11936a6571..a065419cdb 100644
--- a/src/backend/catalog/catalog.c
+++ b/src/backend/catalog/catalog.c
@@ -383,7 +383,7 @@ GetNewOidWithIndex(Relation relation, Oid indexId, AttrNumber oidcolumn)
* is also an unused OID within pg_class. If the result is to be used only
* as a relfilenode for an existing relation, pass NULL for pg_class.
*
- * As with GetNewObjectIdWithIndex(), there is some theoretical risk of a race
+ * As with GetNewOidWithIndex(), there is some theoretical risk of a race
* condition, but it doesn't seem worth worrying about.
*
* Note: we don't support using this in bootstrap mode. All relations
HAVE_BUILTIN_CLZ.patchtext/x-patch; name=HAVE_BUILTIN_CLZ.patchDownload
diff --git a/src/port/pg_bitutils.c b/src/port/pg_bitutils.c
index 60fb55af53..589c2f1615 100644
--- a/src/port/pg_bitutils.c
+++ b/src/port/pg_bitutils.c
@@ -28,7 +28,7 @@
* left-most the 7th bit. The 0th entry of the array should not be used.
*
* Note: this is not used by the functions in pg_bitutils.h when
- * HAVE_BUILTIN_CLZ is defined, but we provide it anyway, so that
+ * HAVE__BUILTIN_CLZ is defined, but we provide it anyway, so that
* extensions possibly compiled with a different compiler can use it.
*/
const uint8 pg_leftmost_one_pos[256] = {
HAVE_BUILTIN_CTZ.patchtext/x-patch; name=HAVE_BUILTIN_CTZ.patchDownload
diff --git a/src/port/pg_bitutils.c b/src/port/pg_bitutils.c
index 60fb55af53..8d9f5be32a 100644
--- a/src/port/pg_bitutils.c
+++ b/src/port/pg_bitutils.c
@@ -56,7 +56,7 @@ const uint8 pg_leftmost_one_pos[256] = {
* left-most the 7th bit. The 0th entry of the array should not be used.
*
* Note: this is not used by the functions in pg_bitutils.h when
- * HAVE_BUILTIN_CTZ is defined, but we provide it anyway, so that
+ * HAVE__BUILTIN_CTZ is defined, but we provide it anyway, so that
* extensions possibly compiled with a different compiler can use it.
*/
const uint8 pg_rightmost_one_pos[256] = {
MultiInsertInfoNextFreeSlot.patchtext/x-patch; name=MultiInsertInfoNextFreeSlot.patchDownload
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 84c54fbc70..ac86f3d5be 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -2626,7 +2626,7 @@ CopyMultiInsertInfoNextFreeSlot(CopyMultiInsertInfo *miinfo,
/*
* Record the previously reserved TupleTableSlot that was reserved by
- * MultiInsertInfoNextFreeSlot as being consumed.
+ * CopyMultiInsertInfoNextFreeSlot as being consumed.
*/
static inline void
CopyMultiInsertInfoStore(CopyMultiInsertInfo *miinfo, ResultRelInfo *rri,
targetIsArray.patchtext/x-patch; name=targetIsArray.patchDownload
diff --git a/src/backend/parser/parse_target.c b/src/backend/parser/parse_target.c
index ef2f5b45d8..ba470366e1 100644
--- a/src/backend/parser/parse_target.c
+++ b/src/backend/parser/parse_target.c
@@ -38,7 +38,7 @@ static void markTargetListOrigin(ParseState *pstate, TargetEntry *tle,
static Node *transformAssignmentIndirection(ParseState *pstate,
Node *basenode,
const char *targetName,
- bool targetIsArray,
+ bool targetIsSubscripting,
Oid targetTypeId,
int32 targetTypMod,
Oid targetCollation,
tss_htup.patchtext/x-patch; name=tss_htup.patchDownload
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 99b9fa414f..ab985b60c6 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -1571,7 +1571,6 @@ typedef struct TidScanState
int tss_NumTids;
int tss_TidPtr;
ItemPointerData *tss_TidList;
- HeapTupleData tss_htup;
} TidScanState;
/* ----------------
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
Attachments:
tss_htup.patchtext/x-patch; name=tss_htup.patchDownload
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 99b9fa414f..03166754c0 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -1560,7 +1560,6 @@ typedef struct BitmapHeapScanState
* NumTids number of tids in this scan
* TidPtr index of currently fetched tid
* TidList evaluated item pointers (array of size NumTids)
- * htup currently-fetched tuple, if any
* ----------------
*/
typedef struct TidScanState
@@ -1571,7 +1570,6 @@ typedef struct TidScanState
int tss_NumTids;
int tss_TidPtr;
ItemPointerData *tss_TidList;
- HeapTupleData tss_htup;
} TidScanState;
/* ----------------
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