Re: Partitioning option for COPY

Emmanuel Cecchet wrote:
> 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.

I think that check_stack_depth() just throws an elog(ERROR) when the
stack depth is exceeded, so you can just add a check_stack_depth() call
somewhere at the beginning of route_tuple_to_child and that's it.

>> 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?

http://www.postgresql.org/docs/current/static/error-style-guide.html

Dropping "Aborting insert" is just a suggestion, it's possible the error
message will sound OK to a native English speaker.

>> 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.

OK, understood.

>> 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.

OK.

>> 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.

> 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.

OK, my competence ends here :-) Someone with a better knowledge of the
code should comment on that, I certainly don't have a better proposal.

>> 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.

OK, that looks better.

>> 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.

I was thinking more about SGML docs. They could mention that BEFORE
triggers are fired both for the parent table and for the child table,
while AFTER triggers will only be called on the target table. I'd add a
sentence or two explaining what happens if you have a three-level
inheritance hierarchy (that the tuple will be inserted in the bottommost
table of the hierarchy).

>> o) my main concern is still valid: the design was never agreed upon.

> 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.

Sure, sorry if it sounded harsh. As I said I have virtually no field
experience with PG, so I might have a wrong perspective. I also don't
feel particulary eligible to judge your approach of handling automatic
partitioning designwise.

Except for the really minor things like checking stack depth and adding
a few sentences to the SGML docs, I think it's time someone more
qualified looked at the patch. If you'd like to send a new version, I'll
wait for it and mark it as ready for committer review. Thanks for
persisting with the patch and sorry for nitpicking so much :-)

Cheers,
Jan