On Tue, Jan 19, 2021 at 9:19 AM Greg Nancarrow <gregn4422@gmail.com> wrote:
>
> On Tue, Jan 19, 2021 at 2:03 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > > >
> > > > You have not raised a WARNING for the second case.
> > >
> > > The same checks in current Postgres code also don't raise a WARNING
> > > for that case, so I'm just being consistent with existing Postgres
> > > code (which itself isn't consistent for those two cases).
> > >
> >
> > Search for the string "too few entries in indexprs list" and you will
> > find a lot of places in code raising ERROR for the same condition.
> >
>
> Yes, but raising an ERROR stops processing (not just logs an error
> message). Raising a WARNING logs a warning message and continues
> processing. It's a big difference.
> So, for the added parallel-safety-checking code, it was suggested by
> Vignesh (and agreed by me) that, for these rare and highly unlikely
> conditions, it would be best not to just copy the error-handling code
> verbatim from other cases in the Postgres code (as I had originally
> done) and just stop processing dead with an error, but to instead
> return PARALLEL_UNSAFE, so that processing continues as it would for
> current non-parallel processing, which would most likely error-out
> anyway along the current error-handling checks and paths when those
> bad attributes/fields are referenced.
> I will add some Asserts() and don't mind adding a WARNING message for
> the 2nd case.
> If you really feel strongly about this, I can just restore the
> original code, which will stop dead with an ERROR in the middle of
> parallel-safety checking should one of these rare conditions ever
> occur.
>
I am expecting that either we raise a WARNING and return
parallel_unsafe for all such checks (shouldn't reach cases) in the
patch or simply raise an ERROR as we do in other parts of the patch. I
personally prefer the latter alternative but I am fine with the former
one as well.
--
With Regards,
Amit Kapila.
