pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedFix inconsistencies for v12 (pass 2)
5+ messages / 2 participants
[nested] [flat]
* Fix inconsistencies for v12 (pass 2)
@ 2019-06-12 14:34 Alexander Lakhin <exclusion@gmail.com>
2019-06-13 08:10 ` Re: Fix inconsistencies for v12 (pass 2) Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 5+ messages in thread
From: Alexander Lakhin @ 2019-06-12 14:34 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; pgsql-hackers
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:
[text/x-patch] bt_binsrch_insert.patch (571B, ../../e59e4d46-7c78-afb1-6edc-df77f6d8ccc9@gmail.com/2-bt_binsrch_insert.patch)
download | inline diff:
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
[text/x-patch] EWOULDBOCK.patch (544B, ../../e59e4d46-7c78-afb1-6edc-df77f6d8ccc9@gmail.com/3-EWOULDBOCK.patch)
download | inline diff:
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;
[text/x-patch] FORGET_RELATION_FSYNC.patch (976B, ../../e59e4d46-7c78-afb1-6edc-df77f6d8ccc9@gmail.com/4-FORGET_RELATION_FSYNC.patch)
download | inline diff:
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
[text/x-patch] get_opclass_family_and_input_type.patch (474B, ../../e59e4d46-7c78-afb1-6edc-df77f6d8ccc9@gmail.com/5-get_opclass_family_and_input_type.patch)
download | inline diff:
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
[text/x-patch] GetNewObjectIdWithIndex.patch (715B, ../../e59e4d46-7c78-afb1-6edc-df77f6d8ccc9@gmail.com/6-GetNewObjectIdWithIndex.patch)
download | inline diff:
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
[text/x-patch] HAVE_BUILTIN_CLZ.patch (570B, ../../e59e4d46-7c78-afb1-6edc-df77f6d8ccc9@gmail.com/7-HAVE_BUILTIN_CLZ.patch)
download | inline diff:
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] = {
[text/x-patch] HAVE_BUILTIN_CTZ.patch (612B, ../../e59e4d46-7c78-afb1-6edc-df77f6d8ccc9@gmail.com/8-HAVE_BUILTIN_CTZ.patch)
download | inline diff:
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] = {
[text/x-patch] MultiInsertInfoNextFreeSlot.patch (540B, ../../e59e4d46-7c78-afb1-6edc-df77f6d8ccc9@gmail.com/9-MultiInsertInfoNextFreeSlot.patch)
download | inline diff:
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,
[text/x-patch] targetIsArray.patch (582B, ../../e59e4d46-7c78-afb1-6edc-df77f6d8ccc9@gmail.com/10-targetIsArray.patch)
download | inline diff:
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,
[text/x-patch] tss_htup.patch (371B, ../../e59e4d46-7c78-afb1-6edc-df77f6d8ccc9@gmail.com/11-tss_htup.patch)
download | inline diff:
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;
/* ----------------
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Fix inconsistencies for v12 (pass 2)
2019-06-12 14:34 Fix inconsistencies for v12 (pass 2) Alexander Lakhin <exclusion@gmail.com>
@ 2019-06-13 08:10 ` Michael Paquier <michael@paquier.xyz>
2019-06-13 08:28 ` Re: Fix inconsistencies for v12 (pass 2) Alexander Lakhin <exclusion@gmail.com>
2019-06-14 04:16 ` Re: Fix inconsistencies for v12 (pass 2) Alexander Lakhin <exclusion@gmail.com>
0 siblings, 2 replies; 5+ messages in thread
From: Michael Paquier @ 2019-06-13 08:10 UTC (permalink / raw)
To: Alexander Lakhin <exclusion@gmail.com>; +Cc: Amit Kapila <amit.kapila16@gmail.com>; pgsql-hackers; Andres Freund <andres@anarazel.de>
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
Attachments:
[application/pgp-signature] signature.asc (832B, ../../20190613081007.GH1643@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Fix inconsistencies for v12 (pass 2)
2019-06-12 14:34 Fix inconsistencies for v12 (pass 2) Alexander Lakhin <exclusion@gmail.com>
2019-06-13 08:10 ` Re: Fix inconsistencies for v12 (pass 2) Michael Paquier <michael@paquier.xyz>
@ 2019-06-13 08:28 ` Alexander Lakhin <exclusion@gmail.com>
2019-06-14 00:42 ` Re: Fix inconsistencies for v12 (pass 2) Michael Paquier <michael@paquier.xyz>
1 sibling, 1 reply; 5+ messages in thread
From: Alexander Lakhin @ 2019-06-13 08:28 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Amit Kapila <amit.kapila16@gmail.com>; pgsql-hackers; Andres Freund <andres@anarazel.de>
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:
[text/x-patch] tss_htup.patch (686B, ../../dec6aae8-2d63-639f-4d50-20e229fb83e3@gmail.com/2-tss_htup.patch)
download | inline diff:
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;
/* ----------------
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Fix inconsistencies for v12 (pass 2)
2019-06-12 14:34 Fix inconsistencies for v12 (pass 2) Alexander Lakhin <exclusion@gmail.com>
2019-06-13 08:10 ` Re: Fix inconsistencies for v12 (pass 2) Michael Paquier <michael@paquier.xyz>
2019-06-13 08:28 ` Re: Fix inconsistencies for v12 (pass 2) Alexander Lakhin <exclusion@gmail.com>
@ 2019-06-14 00:42 ` Michael Paquier <michael@paquier.xyz>
0 siblings, 0 replies; 5+ messages in thread
From: Michael Paquier @ 2019-06-14 00:42 UTC (permalink / raw)
To: Alexander Lakhin <exclusion@gmail.com>; +Cc: Amit Kapila <amit.kapila16@gmail.com>; pgsql-hackers; Andres Freund <andres@anarazel.de>
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
Attachments:
[application/pgp-signature] signature.asc (832B, ../../20190614004231.GB1427@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Fix inconsistencies for v12 (pass 2)
2019-06-12 14:34 Fix inconsistencies for v12 (pass 2) Alexander Lakhin <exclusion@gmail.com>
2019-06-13 08:10 ` Re: Fix inconsistencies for v12 (pass 2) Michael Paquier <michael@paquier.xyz>
@ 2019-06-14 04:16 ` Alexander Lakhin <exclusion@gmail.com>
1 sibling, 0 replies; 5+ messages in thread
From: Alexander Lakhin @ 2019-06-14 04:16 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Michael Paquier <michael@paquier.xyz>; pgsql-hackers
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
^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2019-06-14 04:16 UTC | newest]
Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2019-06-12 14:34 Fix inconsistencies for v12 (pass 2) Alexander Lakhin <exclusion@gmail.com>
2019-06-13 08:10 ` Michael Paquier <michael@paquier.xyz>
2019-06-13 08:28 ` Alexander Lakhin <exclusion@gmail.com>
2019-06-14 00:42 ` Michael Paquier <michael@paquier.xyz>
2019-06-14 04:16 ` Alexander Lakhin <exclusion@gmail.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox