Re: Implementing SQL ASSERTION

From: Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>
To: Joe Wildish <joe-postgresql(dot)org(at)elusive(dot)cx>, David Fetter <david(at)fetter(dot)org>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Implementing SQL ASSERTION
Date: 2018-09-24 14:06:02
Message-ID: 590cb6d4-39b7-1f4c-56c9-6ebc7dc27b68@2ndquadrant.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On 29/04/2018 20:18, Joe Wildish wrote:
> On 28 Mar 2018, at 16:13, David Fetter <david(at)fetter(dot)org> wrote:
>>
>> Sorry to bother you again, but this now doesn't compile atop master.
>
> Attached is a rebased patch for the prototype.

I took a look at this.

This has been lying around for a few months, so it will need to be
rebased again. I applied this patch on top of
68e7e973d22274a089ce95200b3782f514f6d2f8, which was the HEAD around the
time this patch was created, and it applies cleanly there.

Please check you patch for whitespace errors:

warning: squelched 13 whitespace errors
warning: 18 lines add whitespace errors.

Also, reduce the amount of useless whitespace changes in the patch.

There are some compiler warnings:

constraint.c: In function 'CreateAssertion':
constraint.c:1211:2: error: ISO C90 forbids mixed declarations and code
[-Werror=declaration-after-statement]

constraint.c: In function 'oppositeDmlOp':
constraint.c:458:1: error: control reaches end of non-void function
[-Werror=return-type]

The version check in psql's describeAssertions() needs to be updated.
Also, you should use formatPGVersionNumber() to cope with two-part and
one-part version numbers.

All this new code in constraint.c that checks the assertion expression
needs more comments and documentation.

Stuff like this isn't going to work:

static int
funcMaskForFuncOid(Oid funcOid)
{
char *name = get_func_name(funcOid);

if (name == NULL)
return OTHER_FUNC;
else if (strncmp(name, "min", strlen("min")) == 0)
return MIN_AGG_FUNC;
else if (strncmp(name, "max", strlen("max")) == 0)
return MAX_AGG_FUNC;

You can assume from the name of a function what it's going to do.
Solving this properly might be hard.

The regression test crashes for me around

frame #4: 0x000000010d3a4cdc postgres`castNodeImpl(type=T_SubLink,
ptr=0x00007ff27006d230) at nodes.h:582
frame #5: 0x000000010d3a61c6
postgres`visitSubLink(node=0x00007ff270034040, info=0x00007ffee2a23930)
at constraint.c:843

This ought to be reproducible for you if you build with assertions.

My feeling is that if we want to move forward on this topic, we need to
solve the concurrency question first. All these optimizations for when
we don't need to check the assertion are cool, but they are just
optimizations that we can apply later on, once we have solved the
critical problems.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Joe Conway 2018-09-24 14:09:17 Re: doc - add missing documentation for "acldefault"
Previous Message Tom Lane 2018-09-24 14:01:46 Re: doc - add missing documentation for "acldefault"