Hello,
On 2024-Aug-09, Kirill Reshke wrote:
> Hi all!
> This change looks good to me. However, i have an objection to these
> lines from v2:
>
> > /* Close the forks at smgr level */
> > - for (forknum = 0; forknum <= MAX_FORKNUM; forknum++)
> > - smgrsw[which].smgr_close(rels[i], forknum);
> > + smgrclose(rels[i]);
>
> Why do we do this?
I want to say that this does not appear to be to be an objection. I
think it was just a question. An objection implies that you consider a
change to be incorrect or detrimental, and therefore it should not be
made. In this case, by posing your question in this way:
> This seems to be an unrelated change given thread
> $subj. This is just a pure refactoring job, which deserves a separate
> patch. There is similar coding in
> smgrdestroy function:
[...]
> So, I feel like these two places should be either changed together or
> not be altered at all. And is it definitely a separate change.
... the patch was derailed and we ended up not doing anything at all,
and worse, we ended up with a separate thread doing something closely
related but in a way that appears unconnected to this thread. (Worse,
Steven forgot to open a commitfest entry for it, so the patch was lost
in the perpetual pgsql-hackers background noise). I think Masahiko
rejected this patch because it was incorrect without that other patch,
or something like that.
I've marked this[2] entry as returned with feedback now. I would
suggest to Steven to propose this patch again, but this time as a single
patch that does the complete change rather than half here and half
there, keeping Masahiko's closing comments[3] in mind. Once he has such
a patch, he can reopen this commitfest entry as Needs Review (moving it
to whatever the open commitfest is.)
Thanks
[1] https://postgr.es/m/CABBtG=d1Kkmi67VdM=jGaYkQ0+WGbhZpxwa3ms0s1DB_J_9Jww@mail.gmail.com
[2] https://commitfest.postgresql.org/patch/5149/
[3] https://postgr.es/m/CAD21AoBLePJO5NtDoZWRxprCOtauMFr6Uaj4V8FxWJ1rKeyZFw@mail.gmail.com
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
Thou shalt study thy libraries and strive not to reinvent them without
cause, that thy code may be short and readable and thy days pleasant
and productive. (7th Commandment for C Programmers)
