Обсуждение: [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

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

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

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

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

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

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

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.