On Wed, Mar 11, 2015 at 5:10 PM, Etsuro Fujita <fujita.ets...@lab.ntt.co.jp> wrote:
> On 2015/03/11 17:37, Ashutosh Bapat wrote: > >> Now I can reproduce the problem. >> >> Sanity >> -------- >> Patch compiles cleanly and make check passes. The tests in file_fdw and >> postgres_fdw contrib modules pass. >> >> The patch works as expected in the test case reported. >> > > Thanks for the testing! > > I have only one doubt. >> In EvalPlanQualFetchRowMarks(). tuple->t_self is assigned from >> td->t_ctid. CTID or even t_self may be valid for foreign tables based on >> postgres_fdw but may not be valid for other FDWs. So, this assignment >> might put some garbage in t_self, rather we should set it to invalid as >> done prior to the patch. I might have missed some previous thread, we >> decided to go this route, so ignore the comment, in that case. >> > > Good point. As the following code and comment I added to ForeignNext, I > think that FDW authors should initialize the tup->t_data->t_ctid of each > scan tuple with its own TID. If the authors do that, the t_self is > guaranteed to be assigned the right TID from the whole-row Var (ie, > td->t_ctid) in EvalPlanQualFetchRowMarks. > > /* We assume that t_ctid is initialized with its own TID */ > tup->t_self = tup->t_data->t_ctid; > > IMHO, I'm not sure it's worth complicating the code as you mentioned. (I > don't know whether there are any discussions about this before.) > > Note that file_fdw needs no treatment. In that case, in ForeignNext, the > tup->t_data->t_ctid of each scan tuple is initialized with (0,0) (if > necessary), and then the t_self will be correctly assigned (0,0) throguh > the whole-row Var in EvalPlanQualFetchRowMarks. So, no inconsistency! > > I will leave this issue for the committer to judge. Changed the status to "ready for committer". > Apart from this, I do not have any comments here. >> > > Thanks again. > > Best regards, > Etsuro Fujita > -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
