Re: bug in update tuple routing with foreign partitions
От | Etsuro Fujita |
---|---|
Тема | Re: bug in update tuple routing with foreign partitions |
Дата | |
Msg-id | 5CB95F0E.305@lab.ntt.co.jp обсуждение исходный текст |
Ответ на | Re: bug in update tuple routing with foreign partitions (Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>) |
Ответы |
Re: bug in update tuple routing with foreign partitions
(Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>)
Re: bug in update tuple routing with foreign partitions (Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>) |
Список | pgsql-hackers |
(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
В списке pgsql-hackers по дате отправления:
Следующее
От: Etsuro FujitaДата:
Сообщение: Re: bug in update tuple routing with foreign partitions