From: | torikoshia <torikoshia(at)oss(dot)nttdata(dot)com> |
---|---|
To: | Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> |
Cc: | jian he <jian(dot)universality(at)gmail(dot)com>, Álvaro Herrera <alvherre(at)kurilemu(dot)de>, Kirill Reshke <reshkekirill(at)gmail(dot)com>, vignesh C <vignesh21(at)gmail(dot)com>, David Rowley <dgrowleyml(at)gmail(dot)com>, Melih Mutlu <m(dot)melihmutlu(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: speedup COPY TO for partitioned table. |
Date: | 2025-10-09 05:59:30 |
Message-ID: | cd5ddd1ea4abc141a435a57eeb92a3a7@oss.nttdata.com |
Views: | Whole Thread | Raw Message | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
<jian(dot)universality(at)gmail(dot)com> wrote:
>>
>> On Fri, Oct 3, 2025 at 8:31 AM torikoshia <torikoshia(at)oss(dot)nttdata(dot)com>
>> wrote:
>> >
>> > It’s been about two months since this discussion, and there don’t seem
>> > to be any further comments.
>> > How about updating the patch accordingly?
>> > If there are no new remarks, I’d like to mark the patch as Ready for
>> > Committer.
>> >
>> > >> + List *children = NIL;
>> > >> ...
>> > >> + {
>> > >> + children = find_all_inheritors(RelationGetRelid(rel),
>> > >>
>> > >> Since 'children' is only used inside the else if block, I think we
>> > >> don't
>> > >> need the separate "List *children = NIL;" declaration.
>> > >> Instead, it could just be "List *children =
>> > >> find_all_inheritors(...)".
>> > >>
>> > > you are right.
>> > > ""List *children = find_all_inheritors(...)"." should be ok.
>> >
>>
>> hi.
>>
>> please check the attached v15.
Thanks for updating the patch!
Here are some minor comments.
> #include "access/tableam.h"
> +#include "access/table.h"
As in partbounds.c, I think table.h should come before tableam.h in the
include order.
> + char relkind = get_rel_relkind(childreloid);
> +
> + if (relkind == RELKIND_FOREIGN_TABLE)
> + {
> + char *relation_name;
> +
> + relation_name = get_rel_name(childreloid);
Similar to how relkind is declared, it might be simpler to combine the
declaration and assignment here.
On 2025-10-09 10:13, Masahiko Sawada wrote:
Thanks for your review!
> RelationGetRelationName(rel)),
> + errhint("Try the COPY (SELECT ...) TO
> variant."));
>
> I think we don't need "the" in the error message.
I agree. However, I noticed that some existing messages use “the” in
similar contexts. For example:
if (rel->rd_rel->relkind == RELKIND_VIEW)
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg("cannot copy from view \"%s\"",
RelationGetRelationName(rel)),
errhint("Try the COPY (SELECT ...) TO
variant.")));
If we want to fix it, I think we should update all similar messages
together for consistency.
--
Regards,
--
Atsushi Torikoshi
Seconded from NTT DATA Japan Corporation to SRA OSS K.K.
From | Date | Subject | |
---|---|---|---|
Next Message | Peter Smith | 2025-10-09 06:01:47 | Re: duplicate logging in pg_createsubscriber |
Previous Message | Chao Li | 2025-10-09 05:57:20 | Re: What is the build strategy between make and meson? |