Re: SQL:2011 Application Time Update & Delete - Mailing list pgsql-hackers

From Peter Eisentraut
Subject Re: SQL:2011 Application Time Update & Delete
Date
Msg-id 1ace7bc1-9dd4-42c9-a473-517cef37cce9@eisentraut.org
Whole thread Raw
In response to Re: SQL:2011 Application Time Update & Delete  (Paul A Jungwirth <pj@illuminatedcomputing.com>)
Responses Re: SQL:2011 Application Time Update & Delete
List pgsql-hackers
I have looked at the patch

v59-0004-Add-range_minus_multi-and-multirange_minus_multi.patch

This seems sound in principle.

Perhaps you could restate why you chose a set-returning function rather 
than (what I suppose would be the other options) returning multirange or 
an array of ranges.  (I don't necessarily disagree, but it would be good 
to be clear for everyone.)  The point about allowing user-defined types 
makes sense (but for example, I see types like multipolygon and 
multipoint in postgis, so maybe those could also work?).

That said, I think there is a problem in your implementation.  Note that 
the added regression test cases for range return multiple rows but the 
ones for multirange all return a single row with a set {....} value.  I 
think the problem is that your multirange_minus_multi() calls 
multirange_minus_internal() which already returns a set, and you are 
packing that set result into a single row.

A few other minor details:

* src/backend/utils/adt/rangetypes.c

+#include "utils/array.h"

seems to be unused.

+   typedef struct
+   {
+       RangeType  *rs[2];
+       int         n;
+   }           range_minus_multi_fctx;

This could be written just as  a struct, like

struct range_minus_multi_fctx
{
...
};

Wrapping it in a typedef doesn't achieve any additional useful
abstraction.

The code comment before range_minus_multi_internal() could first
explain briefly what the function does before going into the details
of the arguments.  Because we can't assume that someone will have read
the descriptions of the higher-level functions first.

* src/include/catalog/pg_proc.dat

The prorows values for the two new functions should be the same?

(I suppose they are correct now seeing your implementation of 
multirange_minus_multi(), but I'm not sure that was intended, as 
discussed above.)



pgsql-hackers by date:

Previous
From: Xuneng Zhou
Date:
Subject: Re: Rename sync_error_count to tbl_sync_error_count in subscription statistics
Next
From: jian he
Date:
Subject: Re: [patch] ALTER COLUMN SET EXPRESSION [GENERATED|STORED]