Thread: [PATCH v1] eliminate duplicate code in table.c
There are some duplicate code in table.c, add a static inline function to eliminate the duplicates. -- Regards Junwang Zhao
Attachment
On Thu, Jul 21, 2022 at 1:56 PM Junwang Zhao <zhjwpku@gmail.com> wrote: > > There are some duplicate code in table.c, add a static inline function > to eliminate the duplicates. > Can we name function as validate_object_type, or check_object_type? Otherwise, the patch looks fine to me. Let's see if others have something to say. -- With Regards, Amit Kapila.
Hi hackers, > > There are some duplicate code in table.c, add a static inline function > > to eliminate the duplicates. > > > > Can we name function as validate_object_type, or check_object_type? > > Otherwise, the patch looks fine to me. Let's see if others have > something to say. LGTM -- Best regards, Aleksander Alekseev
Attachment
On Thu, Jul 21, 2022 at 5:09 PM Aleksander Alekseev <aleksander@timescale.com> wrote: > > > > There are some duplicate code in table.c, add a static inline function > > > to eliminate the duplicates. > > > > > > > Can we name function as validate_object_type, or check_object_type? > > > > Otherwise, the patch looks fine to me. Let's see if others have > > something to say. > > LGTM > @@ -161,10 +121,32 @@ table_openrv_extended(const RangeVar *relation, LOCKMODE lockmode, * * Note that it is often sensible to hold a lock beyond relation_close; * in that case, the lock is released automatically at xact end. - * ---------------- + * ---------------- */ void table_close(Relation relation, LOCKMODE lockmode) I don't think this change should be part of this patch. Do you see a reason for doing this? -- With Regards, Amit Kapila.
Hi Amit, > I don't think this change should be part of this patch. Do you see a > reason for doing this? My bad. I thought this was done by pgindent. -- Best regards, Aleksander Alekseev
Attachment
On 2022-Jul-21, Junwang Zhao wrote: > There are some duplicate code in table.c, add a static inline function > to eliminate the duplicates. Hmm, but see commit 2ed532ee8c47 about this kind of check. Perhaps we should change these error messages to conform to the same message style. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "I love the Postgres community. It's all about doing things _properly_. :-)" (David Garamond)
Hi Alvaro, > Hmm, but see commit 2ed532ee8c47 about this kind of check. Perhaps we > should change these error messages to conform to the same message style. Good point! Done. -- Best regards, Aleksander Alekseev
Attachment
On Thu, Jul 21, 2022 at 6:12 PM Aleksander Alekseev <aleksander@timescale.com> wrote: > > Hi Alvaro, > > > Hmm, but see commit 2ed532ee8c47 about this kind of check. Perhaps we > > should change these error messages to conform to the same message style. > > Good point! Done. > Yeah, that's better. On again thinking about the function name, I wonder if validate_relation_type() suits here as there is no generic object being passed? -- With Regards, Amit Kapila.
Hi Amit, > Yeah, that's better. On again thinking about the function name, I > wonder if validate_relation_type() suits here as there is no generic > object being passed? Yep, validate_relation_type() sounds better. -- Best regards, Aleksander Alekseev
Attachment
Hi Amit, > Yep, validate_relation_type() sounds better. Or maybe validate_relation_kind() after all? -- Best regards, Aleksander Alekseev
yeah, IMHO validate_relation_kind() is better ;) On Thu, Jul 21, 2022 at 10:21 PM Aleksander Alekseev <aleksander@timescale.com> wrote: > > Hi Amit, > > > Yep, validate_relation_type() sounds better. > > Or maybe validate_relation_kind() after all? > > -- > Best regards, > Aleksander Alekseev -- Regards Junwang Zhao
btw, there are some typos in Patch v5, %s/ralation/relation/g On Thu, Jul 21, 2022 at 10:05 PM Aleksander Alekseev <aleksander@timescale.com> wrote: > > Hi Amit, > > > Yeah, that's better. On again thinking about the function name, I > > wonder if validate_relation_type() suits here as there is no generic > > object being passed? > > Yep, validate_relation_type() sounds better. > > -- > Best regards, > Aleksander Alekseev -- Regards Junwang Zhao
Hi Junwang, > btw, there are some typos in Patch v5, %s/ralation/relation/g D'oh! > yeah, IMHO validate_relation_kind() is better ;) Cool. Here is the corrected patch. Thanks! -- Best regards, Aleksander Alekseev
Attachment
LGTM On Thu, Jul 21, 2022 at 11:52 PM Aleksander Alekseev <aleksander@timescale.com> wrote: > > Hi Junwang, > > > btw, there are some typos in Patch v5, %s/ralation/relation/g > > D'oh! > > > yeah, IMHO validate_relation_kind() is better ;) > > Cool. Here is the corrected patch. Thanks! > > -- > Best regards, > Aleksander Alekseev -- Regards Junwang Zhao
Hi.
+ errmsg("cannot operate on relation \"%s\"",
Other callers of errdetail_relkind_not_supported() describing
operations concretely. In that sense we I think should say "cannot
open relation \"%s\"" here.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
On Fri, Jul 22, 2022 at 7:39 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
>
> + errmsg("cannot operate on relation \"%s\"",
>
> Other callers of errdetail_relkind_not_supported() describing
> operations concretely. In that sense we I think should say "cannot
> open relation \"%s\"" here.
>
Sounds reasonable to me. This will give more precise information to the user.
--
With Regards,
Amit Kapila.
Here is the patch v7. Thanks!
On Fri, Jul 22, 2022 at 1:15 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Jul 22, 2022 at 7:39 AM Kyotaro Horiguchi
> <horikyota.ntt@gmail.com> wrote:
> >
> > + errmsg("cannot operate on relation \"%s\"",
> >
> > Other callers of errdetail_relkind_not_supported() describing
> > operations concretely. In that sense we I think should say "cannot
> > open relation \"%s\"" here.
> >
>
> Sounds reasonable to me. This will give more precise information to the user.
>
> --
> With Regards,
> Amit Kapila.
--
Regards
Junwang Zhao
Attachment
On Fri, Jul 22, 2022 at 1:37 PM Junwang Zhao <zhjwpku@gmail.com> wrote: > > Here is the patch v7. Thanks! > LGTM. I'll push this sometime early next week unless there are more suggestions/comments. -- With Regards, Amit Kapila.