On Fri, Mar 20, 2020 at 12:21 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> On Fri, Mar 20, 2020 at 11:24:25AM +0530, Amit Kapila wrote:
> > On Fri, Mar 20, 2020 at 5:59 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
> > That makes sense. I have a few more comments:
> >
> > 1.
> > + VACUUM_ERRCB_PHASE_INDEX_CLEANUP,
> > +} errcb_phase;
> >
> > Why do you need a comma after the last element in the above enum?
>
> It's not needed but a common convention to avoid needing a two-line patch in
> order to add a line at the end, like:
>
> - foo
> + foo,
> + bar
>
I don't think this is required and we don't have this at other places,
so I removed it. Apart from that, I made a few additional changes
(a) moved the typedef to a different palace as it was looking odd
in-between other struct defines, (b) renamed the enum ErrCbPhase as
that suits more to nearby other trypedefs (c) added/edited comments at
few places, (d) ran pgindent.
See, how the attached looks? I have written a commit message as well,
see if I have missed anyone is from the credit list?
>
> > 8. Can we think of some easy way to add tests for this patch?
>
> Is it possible to make an corrupted index which errors during scan during
> regress tests ?
>
I don't think so.
For now, let's focus on the main patch. Once that is committed, we
can look into the other code rearrangement/cleanup patches.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
