Re: Add minor version to v3 protocol to allow changes without breaking backwards compatibility

Hi Mikko,

First, for everyone else: this patch provides a more-compact binary output
format for arrays. When the array contains no NULL elements and has a
fixed-length element type, the new format saves four bytes per array element.
We could do more. We could add support for arrays containing NULLs by way of
a nulls bitmap. We could reduce the per-array overhead, currently 20 bytes
for a 1-D array, in addition to the per-element overhead. Does anyone care
about these broader cases? If so, speak now.

On Thu, Dec 01, 2011 at 04:42:43PM +0200, Mikko Tiihonen wrote:
> On 11/30/2011 06:52 PM, Merlin Moncure wrote:
>> On Mon, Nov 28, 2011 at 9:18 AM, Mikko Tiihonen
>> <mikko(dot)tiihonen(at)nitorcreations(dot)com> wrote:
>>> Hi,
>>>
>>> As discussed few days ago it would be beneficial if we could change the v3
>>> backend<->client protocol without breaking backwards compatibility.
>>>
>>> Here is a working patch that exports a supported_binary_minor constant and a
>>> binary_minor session variable and a that can be used by clients to enable
>>> newer features.
>>>
>>> I also added an example usage where the array encoding for constant size
>>> elements is optimized if binary_minor version is new enough.
>>>
>>> I have coded the client side support for binary_minor for jdbc driver and
>>> tested that it worked. But lets first discuss if this is an acceptable path
>>> forward.

>> I think all references to
>> 'protocol' should be removed; Binary wire formats != protocol: the
>> protocol could bump to v4 but the wire formats (by happenstance) could
>> all still continue to work -- therefore the version isn't minor (it's
>> not dependent on protocol version but only on itself). Therefore,
>> don't much like the name 'supported_binary_minor'. How about
>> binary_format_version?

> I was thinking that it would be possible use the new minor version to
> introduce also new protocol messages such as streaming of large data
> into database without knowing it's size beforehand.

I agree with Merlin; the frontend/backend protocol is logically distinct from
the binary send/recv formats of data types. For one key point, the latter is
not exclusively core-defined; third-party extensions change their send/recv
formats on a different schedule. They can add myext.binary_format_version
GUCs of their own to cope in a similar way.

Client interfaces that do not interpret individual result column data, such as
libpq, require no update for send/recv format changes. In contrast, all
client interfaces would need changes for a new protocol message. Most clients
doing so may as well then advertise their support unconditionally. In
contrast, send/recv format is something individual _users_ of the same client
library may set differently. It's reasonable to have an application that
still reads its data in send/recv format v0 even after upgrading to a version
of libpq that talks frontend/backend protocol v4.

I do think this is a great way to handle send/recv format changes.

> There needs to be decision on official policy about breaking backwards
> compatibility of postgresql clients. Is it:
>
> A) Every x.y postgres release is free to break any parts of the
> * protocol
> * text encoding
> * binary protocol
> as long as it is documented
> -> all client libraries should be coded so that they refuse to
> support version x.y+1 or newer (they might have a option to
> override this but there are no guarantees that it would work)
> Good: no random bugs when using old client library
> Bad: initial complaints from users before they understand that
> this is the best option for everyone

The ability to use old client libraries with new servers is more valuable than
the combined benefits of all currently-contemplated protocol improvements.

> D) My proposed compromise where there is one minor_version setting
> and code in server to support all different minor versions.
> The client requests the minor version so that all releases
> default to backwards compatible version.

This is workable.

> When there combinations starts to create too much maintenance
> overhead a new clean v4 version of protocol is specified.
> Good: Keeps full backwards compatibility
> Good: Allows introducing changes at any time
> Bad: Accumulates conditional code to server and clients until
> new version of protocol is released

We would retain support for protocol V3 for years following the first release
to support protocol V4, so think of the conditional code as lasting forever.

The patch works as advertised. After "set binary_minor = 1", the output of
this command shrinks from 102 MiB to 63 MiB:

\copy (select array[0,1,2,3,4,5,6,7,8,9] from generate_series(1,1000000)) to mynums with binary

> --- a/src/backend/utils/misc/guc.c
> +++ b/src/backend/utils/misc/guc.c
> @@ -477,6 +477,7 @@ static int segment_size;
> static int wal_block_size;
> static int wal_segment_size;
> static bool integer_datetimes;
> +static int supported_binary_minor;
> static int effective_io_concurrency;
>
> /* should be static, but commands/variable.c needs to get at this */
> @@ -1976,6 +1977,19 @@ static struct config_int ConfigureNamesInt[] =
> },
>
> {
> + {"supported_binary_minor", PGC_INTERNAL, PRESET_OPTIONS,
> + gettext_noop("Maximum supported binary minor version."),
> + NULL,
> + GUC_REPORT | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE
> + },
> + &supported_binary_minor,
> + SUPPORTED_BINARY_MINOR_VERSION,
> + SUPPORTED_BINARY_MINOR_VERSION,
> + SUPPORTED_BINARY_MINOR_VERSION,
> + NULL, NULL, NULL
> + },

We don't need this GUC; clients can do:

select max_val from pg_settings where name = 'binary_minor'

> --- a/src/backend/utils/adt/arrayfuncs.c
> +++ b/src/backend/utils/adt/arrayfuncs.c

> @@ -1535,6 +1537,10 @@ array_send(PG_FUNCTION_ARGS)
> typbyval = my_extra->typbyval;
> typalign = my_extra->typalign;
>
> + flags = ARR_HASNULL(v) ? 1 : 0;
> + if (binary_minor >= 1 && flags == 0 && typlen > 0)
> + flags |= 2;

Let's use symbolic constants for these flags.

Submit patches in context diff format (filterdiff --format=context). Also,
binary_minor.patch is actually two git commits stacked on each other and
modifying the same file twice. Please flatten each patch. You could also
merge fixed_length_array_protocol.patch and binary_minor.patch into one patch.
Neither would be applied in isolation, and they're no easier to understand
when separated.

Add a description of the new GUC to the SGML documentation. Reference that
GUC from the arrays data type page.

Our documentation does not generally describe the send/recv format for data
types, and array_send() is no exception. Considering that, I do wonder how
much software out there is using the current array_send(). Alas, no way to
know. This will be odd; we're adding a GUC that switches between two
undocumented behaviors.

Thanks,
nm