Re: Fix inconsistencies for v12 (pass 2)
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
Re: Fix inconsistencies for v12 (pass 2)
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 signature.asc Description: PGP signature
Re: Fix inconsistencies for v12 (pass 2)
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 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; /*
Re: Fix inconsistencies for v12 (pass 2)
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 signature.asc Description: PGP signature
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
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
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;
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
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
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
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] = {
diff --git a/src/port/pg_bitutils.c b/src/port/pg_bitutils.c
index 60fb55af53..8d9f5be32a 100644
---
Re: Fix inconsistencies for v12
On Mon, Jun 3, 2019 at 10:56 PM Tom Lane wrote: > > Amit Langote writes: > > On 2019/05/30 18:51, Amit Kapila wrote: > >> I think it will be better to include postgres_fdw in the comment in > >> some way so that if someone wants a concrete example, there is > >> something to refer to. > > > Maybe a good idea, but this will be the first time to mention postgres_fdw > > in the core source code. If you think that's OK, how about the attached? > > This wording seems fine to me. > > Now that we've beat that item into the ground ... there were a bunch of > other tweaks suggested in Alexander's initial email. Amit (K), were you > going to review/commit those? > Pushed most of the changes except for two (point no. 10 and point no. 20) about which it is better if someone else can also comment. I have provided suggestions about those in my review email [1]. See, if you have any comments on those. [1] - https://www.postgresql.org/message-id/CAA4eK1J9_gdV22dRg-KaH_tnA1bXOUgLWCoJQikmPVyRbMHboA%40mail.gmail.com -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: Fix inconsistencies for v12
Hello Amit, 07.06.2019 7:30, Amit Kapila wrote: > Leaving the changes related to the above two points, I have combined > all the changes and fixed the things as per my review in the attached > patch. Alexander, see if you can verify once the combined patch. I > am planning to commit the attached by tomorrow and then we can deal > with the remaining two. However, in the meantime, if Andres shared > his views on the above two points, then we can include the changes > corresponding to them as well. Amit, I agree with all of your changes. All I could is to move a dot: .. (see contrib/postgres_fdw/postgres_fdw.c: postgresBeginDirectModify() as one example). Best regards, Alexander
Re: Fix inconsistencies for v12
On Tue, Jun 4, 2019 at 7:36 AM Amit Kapila wrote:
>
> Hi Andres,
>
> I have added you here as some of these are related to commits done by
> you. So need your opinion on the same. I have mentioned where your
> feedback will be helpful.
>
> > 10. HeapTupleSatisfiesSnapshot -> HeapTupleSatisfiesVisibility (an
> > internal inconsistency)
> >
>
> * This is an interface to HeapTupleSatisfiesVacuum that's callable via
> - * HeapTupleSatisfiesSnapshot, so it can be used through a Snapshot.
> + * HeapTupleSatisfiesVisibility, so it can be used through a Snapshot.
>
> I think now we don't need to write the second half of the comment ("so
> it can be used through a Snapshot"). It makes more sense with
> previous style API.
>
> Another related point:
> * HeapTupleSatisfiesNonVacuumable
> *
> * True if tuple might be visible to some
> transaction; false if it's
> * surely dead to everyone, ie, vacuumable.
> *
> * See SNAPSHOT_TOAST's definition for the intended behaviour.
>
> Here, I think instead of SNAPSHOT_TOAST, we should mention
> SNAPSHOT_NON_VACUUMABLE.
>
> Andres, do you have any comments on the proposed changes?
>
>
> > 20. RelationGetOidIndex ? just to remove the paragraph (orphaned after
> > 578b2297)
>
> - * This is exported separately because there are cases where we want to use
> - * an index that will not be recognized by RelationGetOidIndex: TOAST tables
> - * have indexes that are usable, but have multiple columns and are on
> - * ordinary columns rather than a true OID column. This code will work
> - * anyway, so long as the OID is the index's first column. The caller must
> - * pass in the actual heap attnum of the OID column, however.
> - *
>
> Instead of removing the entire paragraph, how about changing it like
> "This also handles the special cases where TOAST tables have indexes
> that are usable, but have multiple columns and are on ordinary columns
> rather than a true OID column. This code will work anyway, so long as
> the OID is the index's first column. The caller must
> pass in the actual heap attnum of the OID column, however."
>
> Andres, any suggestions?
>
Leaving the changes related to the above two points, I have combined
all the changes and fixed the things as per my review in the attached
patch. Alexander, see if you can verify once the combined patch. I
am planning to commit the attached by tomorrow and then we can deal
with the remaining two. However, in the meantime, if Andres shared
his views on the above two points, then we can include the changes
corresponding to them as well.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
0001-Fix-assorted-inconsistencies.patch
Description: Binary data
Re: Fix inconsistencies for v12
Hi Andres,
I have added you here as some of these are related to commits done by
you. So need your opinion on the same. I have mentioned where your
feedback will be helpful.
On Sun, May 26, 2019 at 2:20 AM Alexander Lakhin wrote:
>
> 6. ExecGetResultSlot - remove (not used since introduction in 1a0586de)
>
Yeah, I also think this is not required. Andres, this API is not
defined. Is it intended for some purpose?
> 8. heap_parallelscan_initialize -> remove the sentence (changed in c2fe139c)
>
The same check has been moved to table_block_parallelscan_initialize.
So, I think instead of removing the sentence you need to change the
function name in the comment.
> 10. HeapTupleSatisfiesSnapshot -> HeapTupleSatisfiesVisibility (an
> internal inconsistency)
>
* This is an interface to HeapTupleSatisfiesVacuum that's callable via
- * HeapTupleSatisfiesSnapshot, so it can be used through a Snapshot.
+ * HeapTupleSatisfiesVisibility, so it can be used through a Snapshot.
I think now we don't need to write the second half of the comment ("so
it can be used through a Snapshot"). It makes more sense with
previous style API.
Another related point:
* HeapTupleSatisfiesNonVacuumable
*
* True if tuple might be visible to some
transaction; false if it's
* surely dead to everyone, ie, vacuumable.
*
* See SNAPSHOT_TOAST's definition for the intended behaviour.
Here, I think instead of SNAPSHOT_TOAST, we should mention
SNAPSHOT_NON_VACUUMABLE.
Andres, do you have any comments on the proposed changes?
> 14. latestRemovedxids -> latestRemovedXids (an inconsistent case)
* Conjecture: if hitemid is dead then it had xids before the xids
* marked on LP_NORMAL items. So we just ignore this item and move
* onto the next, for the purposes of calculating
- * latestRemovedxids.
+ * latestRemovedXids.
I think it should be latestRemovedXid.
> 16. NextSampletuple -> NextSampleTuple (an inconsistent case)
I think this doesn't make much difference, but we can fix it so that
NextSampleTuple's occurrence can be found during grep.
> 20. RelationGetOidIndex ? just to remove the paragraph (orphaned after
> 578b2297)
- * This is exported separately because there are cases where we want to use
- * an index that will not be recognized by RelationGetOidIndex: TOAST tables
- * have indexes that are usable, but have multiple columns and are on
- * ordinary columns rather than a true OID column. This code will work
- * anyway, so long as the OID is the index's first column. The caller must
- * pass in the actual heap attnum of the OID column, however.
- *
Instead of removing the entire paragraph, how about changing it like
"This also handles the special cases where TOAST tables have indexes
that are usable, but have multiple columns and are on ordinary columns
rather than a true OID column. This code will work anyway, so long as
the OID is the index's first column. The caller must
pass in the actual heap attnum of the OID column, however."
Andres, any suggestions?
> 27. XactTopTransactionId -> XactTopFullTransactionId (an internal
> inconsistency)
>
- * XactTopTransactionId stores the XID of our toplevel transaction, which
+ * XactTopFullTransactionId stores the XID of our toplevel transaction, which
* will be the same as TopTransactionState.transactionId in an ordinary
I think in the above sentence, now we need to use
TopTransactionState.fullTransactionId.
Note that I agree with your changes for the points where I have not
responded anything.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
Re: Fix inconsistencies for v12
On Mon, Jun 3, 2019 at 10:56 PM Tom Lane wrote: > > Amit Langote writes: > > On 2019/05/30 18:51, Amit Kapila wrote: > >> I think it will be better to include postgres_fdw in the comment in > >> some way so that if someone wants a concrete example, there is > >> something to refer to. > > > Maybe a good idea, but this will be the first time to mention postgres_fdw > > in the core source code. If you think that's OK, how about the attached? > > This wording seems fine to me. > > Now that we've beat that item into the ground ... there were a bunch of > other tweaks suggested in Alexander's initial email. Amit (K), were you > going to review/commit those? > Yes, I am already reviewing those. I will post my comments today. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: Fix inconsistencies for v12
Amit Langote writes: > On 2019/05/30 18:51, Amit Kapila wrote: >> I think it will be better to include postgres_fdw in the comment in >> some way so that if someone wants a concrete example, there is >> something to refer to. > Maybe a good idea, but this will be the first time to mention postgres_fdw > in the core source code. If you think that's OK, how about the attached? This wording seems fine to me. Now that we've beat that item into the ground ... there were a bunch of other tweaks suggested in Alexander's initial email. Amit (K), were you going to review/commit those? regards, tom lane
Re: Fix inconsistencies for v12
On 2019/05/30 18:51, Amit Kapila wrote: > On Wed, May 29, 2019 at 6:12 AM Amit Langote wrote: >> On 2019/05/28 20:26, Amit Kapila wrote: >>> Can we ensure some way that only FDW's rely on it not any other part >>> of the code? >> >> Hmm, I can't think of any way of doing than other than manual inspection. >> We are sure that no piece of core code relies on it in the ExecInitNode() >> code path. Apparently FDWs may, as we've found out here. Now that I've >> looked around, maybe other loadable modules may too, by way of (only?) >> Custom nodes. I don't see any other way to hook into ExecInitNode(), so >> maybe that's it. >> >> So, maybe reword a bit as: >> >> diff --git a/src/backend/executor/nodeModifyTable.c >> b/src/backend/executor/nodeModifyTable.c >> index a3c0e91543..95545c9472 100644 >> --- a/src/backend/executor/nodeModifyTable.c >> +++ b/src/backend/executor/nodeModifyTable.c >> @@ -2316,7 +2316,7 @@ ExecInitModifyTable(ModifyTable *node, EState >> *estate, int eflags) >> * verify that the proposed target relations are valid and open their >> * indexes for insertion of new index entries. Note we *must* set >> * estate->es_result_relation_info correctly while we initialize each >> - * sub-plan; ExecContextForcesOids depends on that! >> + * sub-plan; external modules such as FDWs may depend on that. >> > > I think it will be better to include postgres_fdw in the comment in > some way so that if someone wants a concrete example, there is > something to refer to. Maybe a good idea, but this will be the first time to mention postgres_fdw in the core source code. If you think that's OK, how about the attached? Thanks, Amit diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c index a3c0e91543..da6da06115 100644 --- a/src/backend/executor/nodeModifyTable.c +++ b/src/backend/executor/nodeModifyTable.c @@ -2316,7 +2316,9 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags) * verify that the proposed target relations are valid and open their * indexes for insertion of new index entries. Note we *must* set * estate->es_result_relation_info correctly while we initialize each -* sub-plan; ExecContextForcesOids depends on that! +* sub-plan; external modules such as FDWs may depend on that (see +* contrib/postgres_fdw/postgres_fdw.c: postgresBeginDirectModify() +* as one example.) */ saved_resultRelInfo = estate->es_result_relation_info;
Re: Fix inconsistencies for v12
On Wed, May 29, 2019 at 6:12 AM Amit Langote wrote: > > On 2019/05/28 20:26, Amit Kapila wrote: > > On Tue, May 28, 2019 at 12:29 PM Amit Langote wrote: > >> Seeing that the crash occurs due to postgres_fdw relying on > >> es_result_relation_info being set when initializing a "direct > >> modification" plan on foreign tables managed by it, we could change the > >> comment to say that instead. Note that allowing "direct modification" of > >> foreign tables is a core feature, so there's no postgres_fdw-specific > >> behavior here; there may be other FDWs that support "direct modification" > >> plans and so likewise rely on es_result_relation_info being set. > > > > > > Can we ensure some way that only FDW's rely on it not any other part > > of the code? > > Hmm, I can't think of any way of doing than other than manual inspection. > We are sure that no piece of core code relies on it in the ExecInitNode() > code path. Apparently FDWs may, as we've found out here. Now that I've > looked around, maybe other loadable modules may too, by way of (only?) > Custom nodes. I don't see any other way to hook into ExecInitNode(), so > maybe that's it. > > So, maybe reword a bit as: > > diff --git a/src/backend/executor/nodeModifyTable.c > b/src/backend/executor/nodeModifyTable.c > index a3c0e91543..95545c9472 100644 > --- a/src/backend/executor/nodeModifyTable.c > +++ b/src/backend/executor/nodeModifyTable.c > @@ -2316,7 +2316,7 @@ ExecInitModifyTable(ModifyTable *node, EState > *estate, int eflags) > * verify that the proposed target relations are valid and open their > * indexes for insertion of new index entries. Note we *must* set > * estate->es_result_relation_info correctly while we initialize each > - * sub-plan; ExecContextForcesOids depends on that! > + * sub-plan; external modules such as FDWs may depend on that. > I think it will be better to include postgres_fdw in the comment in some way so that if someone wants a concrete example, there is something to refer to. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: Fix inconsistencies for v12
On Tue, May 28, 2019 at 10:30 AM Alexander Lakhin wrote: > > 28.05.2019 2:05, Amit Kapila wrote: > > On Mon, May 27, 2019 at 3:59 AM Tom Lane wrote: > >> Amit Kapila writes: > >>> On Sun, May 26, 2019 at 2:20 AM Alexander Lakhin > >>> wrote: > 5. ExecContextForcesOids - not changed, but may be should be removed > (orphaned after 578b2297) > >>> Yes, we should remove the use of ExecContextForcesOids. > >> Unless grep is failing me, ExecContextForcesOids is in fact gone. > >> All that's left is one obsolete mention in a comment, which should > >> certainly be cleaned up. > >> .. > > */ > I got a coredump with `make installcheck-world` (on postgres_fdw test): > Thanks for noticing this. I have run the tests in parallel mode with something like make -s check-world -j4 PROVE_FLAGS='-j4'. It didn't stop at failure, so I missed to notice it. However, now looking carefully (by redirecting the output to a log file), I could see this. > > (On a side note, I agree with your remarks regarding 2 and 3; please > look at a better patch for 3 attached.) > The new patch looks good to me. However, instead of committing just this one alone, I will review others as well and see which all can be combined and pushed together. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: Fix inconsistencies for v12
On 2019/05/28 20:26, Amit Kapila wrote: > On Tue, May 28, 2019 at 12:29 PM Amit Langote wrote: >> Seeing that the crash occurs due to postgres_fdw relying on >> es_result_relation_info being set when initializing a "direct >> modification" plan on foreign tables managed by it, we could change the >> comment to say that instead. Note that allowing "direct modification" of >> foreign tables is a core feature, so there's no postgres_fdw-specific >> behavior here; there may be other FDWs that support "direct modification" >> plans and so likewise rely on es_result_relation_info being set. > > > Can we ensure some way that only FDW's rely on it not any other part > of the code? Hmm, I can't think of any way of doing than other than manual inspection. We are sure that no piece of core code relies on it in the ExecInitNode() code path. Apparently FDWs may, as we've found out here. Now that I've looked around, maybe other loadable modules may too, by way of (only?) Custom nodes. I don't see any other way to hook into ExecInitNode(), so maybe that's it. So, maybe reword a bit as: diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c index a3c0e91543..95545c9472 100644 --- a/src/backend/executor/nodeModifyTable.c +++ b/src/backend/executor/nodeModifyTable.c @@ -2316,7 +2316,7 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags) * verify that the proposed target relations are valid and open their * indexes for insertion of new index entries. Note we *must* set * estate->es_result_relation_info correctly while we initialize each - * sub-plan; ExecContextForcesOids depends on that! + * sub-plan; external modules such as FDWs may depend on that. Thanks, Amit
Re: Fix inconsistencies for v12
On Tue, May 28, 2019 at 12:29 PM Amit Langote wrote: > > On 2019/05/28 14:00, Alexander Lakhin wrote: > > 28.05.2019 2:05, Amit Kapila wrote: > >> ... If we read the comment atop ExecContextForcesOids > >> [1] before it was removed, it seems to indicate that the > >> initialization of es_result_relation_info for each subplan is for its > >> usage in ExecContextForcesOids. I have run the regression tests with > >> the attached patch (which removes changing es_result_relation_info in > >> ExecInitModifyTable) and all the tests passed. Do you have any > >> thoughts on this matter? > > > > I got a coredump with `make installcheck-world` (on postgres_fdw test): > > Core was generated by `postgres: law contrib_regression [local] > > UPDATE '. > > Program terminated with signal SIGSEGV, Segmentation fault. > > #0 0x7ff1410ece98 in postgresBeginDirectModify > > (node=0x560a563fab30, eflags=0) at postgres_fdw.c:2363 > > 2363rtindex = > > estate->es_result_relation_info->ri_RangeTableIndex; > > (gdb) bt > > #0 0x7ff1410ece98 in postgresBeginDirectModify > > (node=0x560a563fab30, eflags=0) at postgres_fdw.c:2363 > > #1 0x560a55979e62 in ExecInitForeignScan > > (node=node@entry=0x560a56254dc0, estate=estate@entry=0x560a563f9ae8, > > eflags=eflags@entry=0) at nodeForeignscan.c:227 > > #2 0x560a5594e123 in ExecInitNode (node=node@entry=0x560a56254dc0, > > estate=estate@entry=0x560a563f9ae8, > > eflags=eflags@entry=0) at execProcnode.c:277 > > ... > > So It seems that this is not a dead code. > > > ... So it seems that > > this comment at least diverged from the initial author's intent. > > With this in mind, I am inclined to just remove it. > > Seeing that the crash occurs due to postgres_fdw relying on > es_result_relation_info being set when initializing a "direct > modification" plan on foreign tables managed by it, we could change the > comment to say that instead. Note that allowing "direct modification" of > foreign tables is a core feature, so there's no postgres_fdw-specific > behavior here; there may be other FDWs that support "direct modification" > plans and so likewise rely on es_result_relation_info being set. > Can we ensure some way that only FDW's rely on it not any other part of the code? -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: Fix inconsistencies for v12
On 2019/05/28 14:00, Alexander Lakhin wrote: > 28.05.2019 2:05, Amit Kapila wrote: >> ... If we read the comment atop ExecContextForcesOids >> [1] before it was removed, it seems to indicate that the >> initialization of es_result_relation_info for each subplan is for its >> usage in ExecContextForcesOids. I have run the regression tests with >> the attached patch (which removes changing es_result_relation_info in >> ExecInitModifyTable) and all the tests passed. Do you have any >> thoughts on this matter? > > I got a coredump with `make installcheck-world` (on postgres_fdw test): > Core was generated by `postgres: law contrib_regression [local] > UPDATE '. > Program terminated with signal SIGSEGV, Segmentation fault. > #0 0x7ff1410ece98 in postgresBeginDirectModify > (node=0x560a563fab30, eflags=0) at postgres_fdw.c:2363 > 2363 rtindex = > estate->es_result_relation_info->ri_RangeTableIndex; > (gdb) bt > #0 0x7ff1410ece98 in postgresBeginDirectModify > (node=0x560a563fab30, eflags=0) at postgres_fdw.c:2363 > #1 0x560a55979e62 in ExecInitForeignScan > (node=node@entry=0x560a56254dc0, estate=estate@entry=0x560a563f9ae8, > eflags=eflags@entry=0) at nodeForeignscan.c:227 > #2 0x560a5594e123 in ExecInitNode (node=node@entry=0x560a56254dc0, > estate=estate@entry=0x560a563f9ae8, > eflags=eflags@entry=0) at execProcnode.c:277 > ... > So It seems that this is not a dead code. > ... So it seems that > this comment at least diverged from the initial author's intent. > With this in mind, I am inclined to just remove it. Seeing that the crash occurs due to postgres_fdw relying on es_result_relation_info being set when initializing a "direct modification" plan on foreign tables managed by it, we could change the comment to say that instead. Note that allowing "direct modification" of foreign tables is a core feature, so there's no postgres_fdw-specific behavior here; there may be other FDWs that support "direct modification" plans and so likewise rely on es_result_relation_info being set. How about: diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c index a3c0e91543..95545c9472 100644 --- a/src/backend/executor/nodeModifyTable.c +++ b/src/backend/executor/nodeModifyTable.c @@ -2316,7 +2316,7 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags) * verify that the proposed target relations are valid and open their * indexes for insertion of new index entries. Note we *must* set * estate->es_result_relation_info correctly while we initialize each - * sub-plan; ExecContextForcesOids depends on that! + * sub-plan; FDWs may depend on that. */ saved_resultRelInfo = estate->es_result_relation_info; Thanks, Amit
Re: Fix inconsistencies for v12
28.05.2019 2:05, Amit Kapila wrote:
> On Mon, May 27, 2019 at 3:59 AM Tom Lane wrote:
>> Amit Kapila writes:
>>> On Sun, May 26, 2019 at 2:20 AM Alexander Lakhin
>>> wrote:
5. ExecContextForcesOids - not changed, but may be should be removed
(orphaned after 578b2297)
>>> Yes, we should remove the use of ExecContextForcesOids.
>> Unless grep is failing me, ExecContextForcesOids is in fact gone.
>> All that's left is one obsolete mention in a comment, which should
>> certainly be cleaned up.
>>
> That's right and I was talking about that usage. Initially, I thought
> we need to change the comment, but on again looking at the code, I
> think we can remove that comment and the related code, but I am not
> completely sure. If we read the comment atop ExecContextForcesOids
> [1] before it was removed, it seems to indicate that the
> initialization of es_result_relation_info for each subplan is for its
> usage in ExecContextForcesOids. I have run the regression tests with
> the attached patch (which removes changing es_result_relation_info in
> ExecInitModifyTable) and all the tests passed. Do you have any
> thoughts on this matter?
>
>
> [1]
> /*
> ..
> * We assume that if we are generating tuples for INSERT or UPDATE,
> * estate->es_result_relation_info is already set up to describe the target
> * relation. Note that in an UPDATE that spans an inheritance tree, some of
> * the target relations may have OIDs and some not. We have to make the
> * decisions on a per-relation basis as we initialize each of the subplans of
> * the ModifyTable node, so ModifyTable has to set es_result_relation_info
> * while initializing each subplan.
> ..
> */
I got a coredump with `make installcheck-world` (on postgres_fdw test):
Core was generated by `postgres: law contrib_regression [local]
UPDATE '.
Program terminated with signal SIGSEGV, Segmentation fault.
#0 0x7ff1410ece98 in postgresBeginDirectModify
(node=0x560a563fab30, eflags=0) at postgres_fdw.c:2363
2363 rtindex =
estate->es_result_relation_info->ri_RangeTableIndex;
(gdb) bt
#0 0x7ff1410ece98 in postgresBeginDirectModify
(node=0x560a563fab30, eflags=0) at postgres_fdw.c:2363
#1 0x560a55979e62 in ExecInitForeignScan
(node=node@entry=0x560a56254dc0, estate=estate@entry=0x560a563f9ae8,
eflags=eflags@entry=0) at nodeForeignscan.c:227
#2 0x560a5594e123 in ExecInitNode (node=node@entry=0x560a56254dc0,
estate=estate@entry=0x560a563f9ae8,
eflags=eflags@entry=0) at execProcnode.c:277
...
So It seems that this is not a dead code.
This comment initially appeared with c7a165ad in
nodeAppend.c:ExecInitAppend as following:
/*
* call ExecInitNode on each of the plans to be executed and
save the
* results into the array "initialized". Note we *must* set
* estate->es_result_relation_info correctly while we initialize
each
* sub-plan; ExecAssignResultTypeFromTL depends on that!
*/
for (i = appendstate->as_firstplan; i <=
appendstate->as_lastplan; i++)
{
appendstate->as_whichplan = i;
exec_append_initialize_next(node);
initNode = (Plan *) nth(i, appendplans);
initialized[i] = ExecInitNode(initNode, estate, (Plan *)
node);
}
/*
* initialize tuple type
*/
ExecAssignResultTypeFromTL((Plan *) node, >cstate);
appendstate->cstate.cs_ProjInfo = NULL;
and in ExecAssignResultTypeFromTL we see:
* This is pretty grotty: we need to ensure that result tuples have
* space for an OID iff they are going to be stored into a relation
* that has OIDs. We assume that estate->es_result_relation_info
* is already set up to describe the target relation.
So the initial comment stated that before calling
ExecAssignResultTypeFromTL we need to have correct
es_result_relation_infos (but we don't set them in that code).
Later in commit a376a467 we have the ExecContextForcesOids call inside
ExecAssignResultTypeFromTL appeared:
void
ExecAssignResultTypeFromTL(PlanState *planstate)
{
bool hasoid;
TupleDesc tupDesc;
if (ExecContextForcesOids(planstate, ))
{
/* context forces OID choice; hasoid is now set correctly */
}
And the comment was changed to:
Note we *must* set
* estate->es_result_relation_info correctly while we initialize
each
* sub-plan; ExecContextForcesOids depends on that!
although the code still calls ExecAssignResultTypeFromTL:
for (i = appendstate->as_firstplan; i <=
appendstate->as_lastplan; i++)
{
appendstate->as_whichplan = i;
exec_append_initialize_next(appendstate);
initNode = (Plan *) nth(i, node->appendplans);
appendplanstates[i] = ExecInitNode(initNode, estate);
}
/*
* initialize
Re: Fix inconsistencies for v12
On Mon, May 27, 2019 at 3:59 AM Tom Lane wrote: > > Amit Kapila writes: > > On Sun, May 26, 2019 at 2:20 AM Alexander Lakhin > > wrote: > >> 5. ExecContextForcesOids - not changed, but may be should be removed > >> (orphaned after 578b2297) > > > Yes, we should remove the use of ExecContextForcesOids. > > Unless grep is failing me, ExecContextForcesOids is in fact gone. > All that's left is one obsolete mention in a comment, which should > certainly be cleaned up. > That's right and I was talking about that usage. Initially, I thought we need to change the comment, but on again looking at the code, I think we can remove that comment and the related code, but I am not completely sure. If we read the comment atop ExecContextForcesOids [1] before it was removed, it seems to indicate that the initialization of es_result_relation_info for each subplan is for its usage in ExecContextForcesOids. I have run the regression tests with the attached patch (which removes changing es_result_relation_info in ExecInitModifyTable) and all the tests passed. Do you have any thoughts on this matter? [1] /* .. * We assume that if we are generating tuples for INSERT or UPDATE, * estate->es_result_relation_info is already set up to describe the target * relation. Note that in an UPDATE that spans an inheritance tree, some of * the target relations may have OIDs and some not. We have to make the * decisions on a per-relation basis as we initialize each of the subplans of * the ModifyTable node, so ModifyTable has to set es_result_relation_info * while initializing each subplan. .. */ -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com remove_dead_code_ExecContextForcesOids_1.patch Description: Binary data
Re: Fix inconsistencies for v12
Amit Kapila writes: > On Sun, May 26, 2019 at 2:20 AM Alexander Lakhin wrote: >> 5. ExecContextForcesOids - not changed, but may be should be removed >> (orphaned after 578b2297) > Yes, we should remove the use of ExecContextForcesOids. Unless grep is failing me, ExecContextForcesOids is in fact gone. All that's left is one obsolete mention in a comment, which should certainly be cleaned up. However, the full context of the mention is /* * call ExecInitNode on each of the plans to be executed and save the * results into the array "mt_plans". This is also a convenient place to * verify that the proposed target relations are valid and open their * indexes for insertion of new index entries. Note we *must* set * estate->es_result_relation_info correctly while we initialize each * sub-plan; ExecContextForcesOids depends on that! */ which makes one wonder if the code to twiddle estate->es_result_relation_info during subplan init is dead code. If so we probably ought to remove it, as it's surely confusing. If it's not dead, then this comment ought to be updated to explain the surviving reason(s), not simply deleted. regards, tom lane
Re: Fix inconsistencies for v12
On Sun, May 26, 2019 at 2:20 AM Alexander Lakhin 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
Fix inconsistencies for v12
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)
3. copy_relation_data -> remove (orphaned after d25f5191)
4. endblock -> endblk (an internal inconsistency)
5. ExecContextForcesOids - not changed, but may be should be removed
(orphaned after 578b2297)
6. ExecGetResultSlot - remove (not used since introduction in 1a0586de)
7. existedConstraints & partConstraint -> provenConstraint &
testConstraint (sync with implementation)
8. heap_parallelscan_initialize -> remove the sentence (changed in c2fe139c)
9. heap_rescan_set_params - remove (orphaned after c2fe139c)
10. HeapTupleSatisfiesSnapshot -> HeapTupleSatisfiesVisibility (an
internal inconsistency)
11. interpretOidsOption - remove (orphaned after 578b2297)
12. item_itemno -> iter_itemno (an internal inconsistency)
13. iterset_is_member -> intset_is_member (an internal inconsistency)
14. latestRemovedxids -> latestRemovedXids (an inconsistent case)
15. newrode -> newrnode (an internal inconsistency)
16. NextSampletuple -> NextSampleTuple (an inconsistent case)
17. oid_typioparam - remove? (orphaned after 578b2297)
18. recoveryTargetIsLatest - remove (orphaned after 2dedf4d9)
19. register_unlink -> register_unlink_segment (an internal inconsistency)
20. RelationGetOidIndex ? just to remove the paragraph (orphaned after
578b2297)
21. slot_getsomeattr -> checked in slot_getsomeattrs ? (an internal
inconsistency and questionable grammar)
22. spekToken -> specToken (an internal inconsistency)
23. SSLdone -> secure_done (sync with implementation)
24. stats_relation & keep_buf - remove (orphaned after 9a8ee1dc & 5db6df0c0)
25. SyncRequstHandler -> SyncRequestHandler (a typo)
26. table_needs_toast_table -> table_relation_needs_toast_table (an
internal inconsistency)
27. XactTopTransactionId -> XactTopFullTransactionId (an internal
inconsistency)
The separate patches for all the defects (except 5) are attached. In
case a single patch is preferable, I can produce it too.
Best regards,
Alexander
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index c9b8857d30..e4627d2026 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -5512,8 +5512,8 @@ ATPrepAddColumn(List **wqueue, Relation rel, bool recurse, bool recursing,
}
/*
- * Add a column to a table; this handles the AT_AddOids cases as well. The
- * return value is the address of the new column in the parent relation.
+ * Add a column to a table. The return value is the address of the
+ * new column in the parent relation.
*/
static ObjectAddress
ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
diff --git a/contrib/pgrowlocks/pgrowlocks.c b/contrib/pgrowlocks/pgrowlocks.c
index a2c44a916c..caf036f40b 100644
--- a/contrib/pgrowlocks/pgrowlocks.c
+++ b/contrib/pgrowlocks/pgrowlocks.c
@@ -165,7 +165,7 @@ pgrowlocks(PG_FUNCTION_ARGS)
infomask = tuple->t_data->t_infomask;
/*
- * A tuple is locked if HTSU returns BeingModified.
+ * A tuple is locked if HTSU returns TM_BeingModified.
*/
if (htsu == TM_BeingModified)
{
diff --git a/src/backend/access/heap/rewriteheap.c b/src/backend/access/heap/rewriteheap.c
index 131ec7b8d7..19617e6eaa 100644
--- a/src/backend/access/heap/rewriteheap.c
+++ b/src/backend/access/heap/rewriteheap.c
@@ -350,10 +350,9 @@ end_heap_rewrite(RewriteState state)
*
* It's obvious that we must do this when not WAL-logging. It's less
* obvious that we have to do it even if we did WAL-log the pages. The
- * 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.
*/
if (RelationNeedsWAL(state->rs_new_rel))
heap_sync(state->rs_new_rel);
diff --git a/src/backend/access/transam/xloginsert.c b/src/backend/access/transam/xloginsert.c
index d3c0a93a2e..3ec67d468b 100644
--- a/src/backend/access/transam/xloginsert.c
+++ b/src/backend/access/transam/xloginsert.c
@@ -1024,7 +1024,7 @@ log_newpage_buffer(Buffer buffer, bool page_std)
/*
* WAL-log a range of blocks in a relation.
*
- * An image of all pages with block numbers 'startblk' <= X < 'endblock' is
+ * An image of all pages with block numbers 'startblk' <= X < 'endblk' is
* written to the WAL. If the range is large, this is done in multiple WAL
* records.
*
diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h
index 88134bcc71..d056fd6151 100644
--- a/src/include/executor/executor.h
+++ b/src/include/executor/executor.h
@@ -514,7 +514,6 @@ extern ExprContext
