(2019/04/19 13:00), Amit Langote wrote:
On 2019/04/18 22:10, Etsuro Fujita wrote:
On 2019/04/18 14:06, Amit Langote wrote:
On 2019/04/17 21:49, Etsuro Fujita wrote:
So what I'm thinking is to throw an error for cases like this.
(Though, I
think we should keep to allow rows to be moved from local partitions to a
foreign-table subplan targetrel that has been updated already.)
Hmm, how would you distinguish (totally inside postgred_fdw I presume) the
two cases?
One thing I came up with to do that is this:
@@ -1917,6 +1937,23 @@ postgresBeginForeignInsert(ModifyTableState *mtstate,
initStringInfo(&sql);
+ /*
+ * If the foreign table is an UPDATE subplan resultrel that hasn't
yet
+ * been updated, routing tuples to the table might yield incorrect
+ * results, because if routing tuples, routed tuples will be
mistakenly
+ * read from the table and updated twice when updating the table
--- it
+ * would be nice if we could handle this case; but for now, throw
an error
+ * for safety.
+ */
+ if (plan&& plan->operation == CMD_UPDATE&&
+ (resultRelInfo->ri_usesFdwDirectModify ||
+ resultRelInfo->ri_FdwState)&&
+ resultRelInfo> mtstate->resultRelInfo +
mtstate->mt_whichplan)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot route tuples into foreign
table to be updated \"%s\"",
+ RelationGetRelationName(rel))));
Oh, I see.
So IIUC, you're making postgresBeginForeignInsert() check two things:
1. Whether the foreign table it's about to begin inserting (moved) rows
into is a subplan result rel, checked by
(resultRelInfo->ri_usesFdwDirectModify || resultRelInfo->ri_FdwState)
2. Whether the foreign table it's about to begin inserting (moved) rows
into will be updated later, checked by (resultRelInfo>
mtstate->resultRelInfo + mtstate->mt_whichplan)
This still allows a foreign table to receive rows moved from the local
partitions if it has already finished the UPDATE operation.
Seems reasonable to me.
Great!
Forgot to say that since this is a separate issue from the original bug
report, maybe we can first finish fixing the latter. What to do you think?
Yeah, I think we can do that, but my favorite would be to fix the two
issues in a single shot, because they seem to me rather close to each
other. So I updated a new version in a single patch, which I'm attaching.
I agree that we can move to fix the other issue right away as the fix you
outlined above seems reasonable, but I wonder if it wouldn't be better to
commit the two fixes separately? The two fixes, although small, are
somewhat complicated and combining them in a single commit might be
confusing. Also, a combined commit might make it harder for the release
note author to list down the exact set of problems being fixed.
OK, I'll separate the patch into two.
Notes:
* I kept all the changes in the previous patch, because otherwise
postgres_fdw would fail to release resources for a foreign-insert
operation created by postgresBeginForeignInsert() for a tuple-routable
foreign table (ie, a foreign-table subplan resultrel that has been updated
already) during postgresEndForeignInsert().
Hmm are you saying that the cases for which we'll still allow tuple
routing (foreign table receiving moved-in rows has already been updated),
there will be two fmstates to be released -- the original fmstate
(UPDATE's) and aux_fmstate (INSERT's)?
Yeah, but I noticed that that explanation was not correct. (I think I
was really in hurry.) See the correction in [1].
* I revised a comment according to your previous comment, though I changed
a state name: s/sub_fmstate/aux_fmstate/
That seems fine to me.
Cool!
Some mostly cosmetic comments on the code changes:
* In the following comment:
+ /*
+ * If the foreign table is an UPDATE subplan resultrel that hasn't yet
+ * been updated, routing tuples to the table might yield incorrect
+ * results, because if routing tuples, routed tuples will be mistakenly
+ * read from the table and updated twice when updating the table --- it
+ * would be nice if we could handle this case; but for now, throw an
error
+ * for safety.
+ */
the part that start with "because if routing tuples..." reads a bit
unclear to me. How about writing this as:
/*
* If the foreign table we are about to insert routed rows into is
* also an UPDATE result rel and the UPDATE hasn't been performed yet,
* proceeding with the INSERT will result in the later UPDATE
* incorrectly modifying those routed rows, so prevent the INSERT ---
* it would be nice if we could handle this case, but for now, throw
* an error for safety.
*/
I think that's better than mine; will use that wording.
I see that in all the hunks involving some manipulation of aux_fmstate,
there's a comment explaining what it is, which seems a bit repetitive. I
can see more or less the same explanation in postgresExecForeignInsert(),
postgresBeginForeignInsert(), and postgresEndForeignInsert(). Maybe just
keep the description in postgresBeginForeignInsert as follows:
@@ -1983,7 +2020,19 @@ postgresBeginForeignInsert(ModifyTableState *mtstate,
retrieved_attrs != NIL,
retrieved_attrs);
- resultRelInfo->ri_FdwState = fmstate;
+ /*
+ * If the given resultRelInfo already has PgFdwModifyState set, it means
+ * the foreign table is an UPDATE subplan resultrel; in which case, store
+ * the resulting state into the aux_fmstate of the PgFdwModifyState.
+ */
and change the other sites to refer to postgresBeginForeingInsert for the
detailed explanation of what aux_fmstate is.
Good idea; will do.
Thanks for the comments!
Best regards,
Etsuro Fujita
[1] https://www.postgresql.org/message-id/5CB93D3F.6050903%40lab.ntt.co.jp
