Feedback-ID: ia2694551:Fastmail
Date: Mon, 3 Feb 2025 17:40:18 +0100
From: =?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@alvh.no-ip.org>
To: Dmitry Koval <d.koval@postgrespro.ru>
Cc: Alexander Korotkov <aekorotkov@gmail.com>, 
	pgsql-hackers@lists.postgresql.org
Subject: Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
Message-ID: <202502031640.zem6orjmmxoz@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <ff30dc7e-ebca-42a2-9d49-a66c9a3c7ee5@postgrespro.ru>
Archived-At: <https://www.postgresql.org/message-id/202502031640.zem6orjmmxoz%40alvherre.pgsql>
Precedence: bulk

> --- a/src/backend/catalog/heap.c
> +++ b/src/backend/catalog/heap.c
> @@ -101,9 +101,6 @@ static ObjectAddress AddNewRelationType(const char *typeName,
>  										Oid new_row_type,
>  										Oid new_array_type);
>  static void RelationRemoveInheritance(Oid relid);
> -static Oid	StoreRelCheck(Relation rel, const char *ccname, Node *expr,
> -						  bool is_enforced, bool is_validated, bool is_local,
> -						  int16 inhcount, bool is_no_inherit, bool is_internal);
>  static void StoreConstraints(Relation rel, List *cooked_constraints,
>  							 bool is_internal);
>  static bool MergeWithExistingConstraint(Relation rel, const char *ccname, Node *expr,
> @@ -111,7 +108,6 @@ static bool MergeWithExistingConstraint(Relation rel, const char *ccname, Node *
>  										bool is_enforced,
>  										bool is_initially_valid,
>  										bool is_no_inherit);
> -static void SetRelationNumChecks(Relation rel, int numchecks);

I'm wary of taking some static functions and making them non-static
without further analysis, because we create a kind-of API that's
possible incoherent or at least incomplete and may cause trouble later.
Maybe we should make some effort to construct a real interface here.
For example, perhaps you don't need to expose both StoreRelCheck and
SetRelationNumChecks, but instead would be better served by exposing
StoreConstraints?  (Though I think we need some explicit check that that
function requires that the table is new and has no constraint, otherwise
SetRelationNumChecks stores the wrong thing.  No?)  Or maybe use
AddRelationNewConstraints() instead, which is an already exposed function.

What I'm saying is just that we should be more watchful of what we put
in our .h files.

Note: I didn't actually review the patch.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/