Re: Partitioning option for COPY

Emmanuel Cecchet wrote:
> Hi all,

Hi!,

> partitioning option for COPY

Here's the review:

== Submission ==
The patch is contextual, applies cleanly to current HEAD, compiles fine.
The docs build cleanly.

== Docs ==
They're reasonably clear, although they still mention ERROR_LOGGING,
which was taken out of this patch. They could use some wordsmithing, but
I didn't go into details, as there were more severe issues with the patch.

One thing that made me cautious was the mention that triggers modifying
tuples will make random errors appear. As is demonstrated later,
triggers are a big issue.

== Regression tests ==
They ran fine, there's one additional regression test that exercises the
new option.

== Style/nitpicks ==
Minor gripes include:
o instead of using an ad-hoc data structure for the LRU cache list, I'd
suggest an OidList from pg_list.h.
o some mentions of "method" in comments should be changed to "function"
o trailing whitespace in the patch (it's not the end of the world, of
course)

== Issues ==
Attached are 3 files that demonstrate problems the patch has.
o test1.sql always segfaults for me, poking around with gdb suggests
it's a case of an uninitialised cache list (another reason to use the
builtin one).
o test2.sql demonstrates, that indices on child tables are not being
updated, probably because after resultRelInfo in
check_tuple_constraints() gets created is never has ri_NumIndices set,
and so the code that was supposed to take care of indices is never
called. Looks like a copy-paste error.
o test3.sql demonstrates, that some triggers that I would expect to be
fired are in fact not fired. I guess it's the same reason as mentioned:
ri_TrigDesc never gets set, so the code that calls triggers is dead.

I stopped there, because unfortunately, apart from all that there's one
fundamental problem with this patch, namely "we probably don't want it".

As it stands it's more of a proof of concept than a really usable
solution, it feels like built from spare (copied from around copy.c)
parts. IMHO it's much too narrow for a general partitioning solution,
even if the design it's based upon would be accepted. It's assuming a
lot of things about the presence of child tables (with proper
constraints), the absence of triggers, and so on.

Granted, it solves a particular problem (bulk loading into a partitioned
table, with not extra features like triggers and with standard
inheritance/exclusive check constraints setup), but that's not good
enough in my opinion, even if all other issues would be addressed.

Now I'm not a real Postgres user, it's been a while since I worked in a
PG shop (or a DB shop for that matter), but from what I understand from
following this community for a while, a patch like that doesn't have a
lot of chances to be committed. That said, my puny experience with real
PG installations and their needs must be taken into account here.

I'll mark this patch as "Waiting on Author", but I have little doubt
that even after fixing those probably trivial segfaults etc. the patch
would be promptly rejected by a committer. I suggest withdrawing it from
this commitfest and trying to work out a more complete design first that
would address the needs of a bigger variety of users, or joining some of
the already underway efforts to bring full-featured partitioning into
Postgres.

Best,
Jan