On Fri, Mar 20, 2020 at 12:21 PM Justin Pryzby <pry...@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 <pry...@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
v30-0001-Introduce-vacuum-errcontext-to-display-additiona.patch
Description: Binary data
