<[email protected]> wrote:
On Fri, Oct 3, 2025 at 8:31 AM torikoshia <[email protected]>
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.
