Re: Partitioning option for COPY

Jan,

> A couple of nitpicks first:
>
> o) the route_tuple_to_child recurses to child tables of child tables,
> which is undocumented and requires a check_stack_depth() call if it's
> really desirable
>
The recursive call is as deep as the inheritance hierarchy. I am not
sure what we are supposed to do if check_stack_depth() fails.
> o) the error messages given when a trigger modifies the tuple should be
> one sentence, I suggest dropping the "Aborting insert" part
>
Where are those rules about error messages specified?
> o) there are two places with "Close the relation but keep the lock"
> comments. Why is in necessary to keep the locks? I confess I don't know
> why *wouldn't* it be necessary, but maybe the comment could explain
> that? Or is it just my lack of understanding and it should be obvious
> that the lock needs to be kept?
>
As we did write to the table, we must maintain the lock on it until the
operation or transaction is complete.
> o) the result of relation_open is explicitly cast to Relation, the
> result of try_relation_open is not (a minor gripe)
>
The first cast was unnecessary, I removed it.
> And a couple of more important things:
>
> o) the code added in trigger.c (ExecARInsertTriggersNow) is copy/pasted
> from just above, I guess there was a reason why you needed that code,
> but I also suspect that's a string indication that something's wrong
> with the abstractions in your patch. Again I don't really know how else
> you could achieve what you want. It just looks fishy if you need to
> modify trigger.c to add an option to COPY.
>
As I explained to Tom, if the after row trigger is called asynchronously
I get a relcache leak on the child table at the end of the copy
operation. If the trigger is called synchronously (like a before row
trigger) it works fine. Also calling the after row trigger synchronously
allows me to detect any potential problem between the actions of the
trigger and the routing decision. I am open to any suggestion for a more
elegant solution.
> o) publicizing ExecRelCheck might also indicate a problem, but I guess
> that can be defended, as the patch is basically based on using that
> function for each incoming tuple
>
The only exposed method for checking constraints (ExecConstraints) goes
directly into an error (ereport) if the constraint checking fails.
Another option would be to add a new parameter to ExecConstraint to tell
it whether to generate an ereport or not but that would impact all
callers of that method.
> o) the LRU OID cache is a separate optimisation that could be
> separated from the patch. I didn't do any performance tests, and I trust
> that a cache like that helps with some workloads, but I think we could
> do a better effort that a simplistic cache like that. Also, I'm not 100%
> sure it's OK to just stick it into CacheMemoryContext... Maybe it could
> go into the COPY statement context? You said you don't want to start
> with a cold cache always, but OTOH if you're loading into different
> tables in the same backend, the cache will actually hurt...
>
> [thinks of something really bad... types up a quick test...]
>
> Oh, actually, the cache is outright *wrong*, as the attached test6.sql
> shows. Ugh, let's just forget about that LRU cache for now.
>
Point taken, I have removed the cache from the GUC variables and it is
now only used for the duration of the COPY operation.
> o) the patch could use some more docs, especially about descending into
> child tables.
>
Do you mean an overall comment explaining the design? Otherwise there is
a comment for every single 'if' and block of code in the patch. Be more
specific if you have a special location where you think comments are
missing or too vague.
> o) my main concern is still valid: the design was never agreed upon.
> The approach of using inheritance info for automatic partitioning is, at
> least IMHO, too restricted. Regular INSERTs won't get routed to child
> tables. Data from writable CTEs won't get routed. People wanting to do
> partitioning on something else that constraints are stuffed.
>
> I strongly suspect the patch will get rejected on the grounds of lack of
> community agreement on partitioning, but I'd hate to see your work
> wasted. It's not too late to open a discussion on how automatic
> partitioning could work (or start working out a common proposal with the
> people discussing in the "Syntax for partitioning" thread).
>
> Marking as Waiting on Author, although I'd really like to see a solid
> design being agreed upon, and then the code.
>
I already commented on that part in another message and this is not
related to that patch but to the politics of implementing partitioning
in Postgres. Now if the rejection of the patch is based on political
stances rather than technical once, I can understand that too.

Please find the new patch attached.
Emmanuel

--
Emmanuel Cecchet
Aster Data
Web: http://www.asterdata.com