On Thu, Mar 4, 2021 at 7:16 AM Greg Nancarrow <gregn4...@gmail.com> wrote: > > On Thu, Mar 4, 2021 at 1:07 AM Dilip Kumar <dilipbal...@gmail.com> wrote: > > > > On Wed, Mar 3, 2021 at 5:50 PM Greg Nancarrow <gregn4...@gmail.com> wrote: > > > > > > Asserts are normally only enabled in a debug-build, so for a > > > release-build that Assert has no effect. > > > The Assert is being used as a sanity-check that the function is only > > > currently getting called for INSERT, because that's all it currently > > > supports. > > > > I agree that assert is only for debug build, but once we add and > > assert that means we are sure that it should only be called for insert > > and if it is called for anything else then it is a programming error > > from the caller's side. So after the assert, adding if check for the > > same condition doesn't look like a good idea. That means we think > > that the code can hit assert in the debug mode so we need an extra > > protection in the release mode. > > > > The if-check isn't there for "extra protection". > It's to help with future changes; inside that if-block is code only > applicable to INSERT (and to UPDATE - sorry, before I said DELETE), as > the code-comment indicates, whereas the rest of the function is > generic to all command types. I don't see any problem with having this > if-block here, to help in this way, when other command types are > added. >
I think for Update/Delete, we might not do parallel-safety checks by calling target_rel_max_parallel_hazard_recurse especially because partitions are handled differently for Updates and Deletes (see inheritance_planner()). I think what Dilip is telling doesn't sound unreasonable to me. So, even, if we want to extend it later by making some checks specific to Inserts/Updates, we can do it at that time. The comments you have at that place are sufficient to tell that in the future we can use those checks for Updates as well. They will need some adjustment if we remove that check but the intent is clear. -- With Regards, Amit Kapila.
