On Wed, Mar 24, 2021 at 8:09 PM Tom Lane <t...@sss.pgh.pa.us> wrote:
>
> Amul Sul <sula...@gmail.com> writes:
> > On Tue, Mar 23, 2021 at 8:59 PM Tom Lane <t...@sss.pgh.pa.us> wrote:
> >> On closer inspection, I believe the true culprit is c6b92041d,
> >> which did this:
> >> - heap_sync(state->rs_new_rel);
> >> + smgrimmedsync(state->rs_new_rel->rd_smgr, MAIN_FORKNUM);
> >> heap_sync was careful about opening rd_smgr, the new code not so much.
>
> > So we also need to make sure of the
> > RelationOpenSmgr() call before smgrimmedsync() as proposed previously.
>
> I wonder if we should try to get rid of this sort of bug by banning
> direct references to rd_smgr? That is, write the above and all
> similar code like
>
> smgrimmedsync(RelationGetSmgr(state->rs_new_rel), MAIN_FORKNUM);
>
> where we provide something like
>
> static inline struct SMgrRelationData *
> RelationGetSmgr(Relation rel)
> {
> if (unlikely(rel->rd_smgr == NULL))
> RelationOpenSmgr(rel);
> return rel->rd_smgr;
> }
>
> and then we could get rid of most or all other RelationOpenSmgr calls.
>
+1
> This might create more code bloat than it's really worth, but
> it'd be a simple and mechanically-checkable scheme.
I think that will be fine, one-time pain. If you want I will do those changes.
A quick question: Can't it be a macro instead of an inline function
like other macros we have in rel.h?
Regards,
Amul
