On Tue, Jan 19, 2021 at 9:19 AM Greg Nancarrow <gregn4...@gmail.com> wrote: > > On Tue, Jan 19, 2021 at 2:03 PM Amit Kapila <amit.kapil...@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.