On Tue, Jun 23, 2020 at 11:49 PM Andres Freund <andres@anarazel.de> wrote:
>
> Hi,
>
> On 2020-06-23 00:14:40 -0400, Tom Lane wrote:
> > Andres Freund <andres@anarazel.de> writes:
> > > I am only suggesting that where you save the old location, as currently
> > > done with LVRelStats olderrinfo, you instead use a more specific
> > > type. Not that you should pass that anywhere (except for
> > > update_vacuum_error_info).
> >
> > As things currently stand, I don't think we need another struct
> > type at all. ISTM we should hard-wire the handling of indname
> > as I suggested above. Then there are only two fields to be dealt
> > with, and we could just as well save them in simple local variables.
>
> That's fine with me too.
>
I have looked at both the patches (using separate variables (by
Justin) and using a struct (by Andres)) and found the second one bit
better.
>
> > If there's a clear future path to needing to save/restore more
> > fields, then maybe another struct type would be useful ... but
> > right now the struct type declaration itself would take more
> > lines of code than it would save.
>
> FWIW, I started to be annoyed by this code when I was addding
> prefetching support to vacuum, and wanted to change what's tracked
> where. The number of places that needed to be touched was
> disproportional.
>
>
> Here's a *draft* for how I thought this roughly could look like. I think
> it's nicer to not specify the exact saved state in multiple places, and
> I think it's much clearer to use a separate function to restore the
> state than to set a "fresh" state.
>
I think this is a good idea and makes code look better. I think it is
better to name new struct as LVSavedErrInfo instead of LVSavedPos.
> I've only applied a hacky fix for the way the indname is tracked, I
> thought that'd best be discussed separately.
>
I think it is better to use Tom's idea here to save and restore index
information in-place. I have used Justin's patch with some
improvements like adding Asserts and initializing with NULL for
indname while restoring to make things unambiguous.
I have improved some comments in the code and for now, kept as two
patches (a) one for improving the error info for index (mostly
Justin's patch based on Tom's idea) and (b) the other to generally
improve the code in this area (mostly Andres's patch).
I have done some testing with both the patches and would like to do
more unless there are objections with these.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
