[PATCH v1] eliminate duplicate code in table.c

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.