public inbox for [email protected]  

help / color / mirror / Atom feed

Optional message to user when terminating/cancelling backend
46+ messages / 14 participants
[nested] [flat]

* Optional message to user when terminating/cancelling backend
@ 2017-06-19 18:24  Daniel Gustafsson <[email protected]>
  0 siblings, 4 replies; 46+ messages in thread

From: Daniel Gustafsson @ 2017-06-19 18:24 UTC (permalink / raw)
  To: pgsql-hackers

When terminating, or cancelling, a backend it’s currently not possible to let
the signalled session know *why* it was dropped.  This has nagged me in the
past and now it happened to come up again, so I took a stab at this.  The
attached patch implements the ability to pass an optional text message to the
signalled session which is included in the error message:

  SELECT pg_terminate_backend(<pid> [, message]);
  SELECT pg_cancel_backend(<pid> [, message]);

Right now the message is simply appended on the error message, not sure if
errdetail or errhint would be better? Calling:

  select pg_terminate_backend(<pid>, 'server rebooting');

..leads to:

  FATAL:  terminating connection due to administrator command: "server rebooting"

Omitting the message invokes the command just like today.

The message is stored in a new shmem area which is checked when the session is
aborted.  To keep things simple a small buffer is kept per backend for the
message.  If deemed too costly, keeping a central buffer from which slabs are
allocated can be done (but seemed rather complicated for little gain compared
to the quite moderate memory spend.) 

cheers ./daniel



^ permalink  raw  reply  [nested|flat] 46+ messages in thread

* Re: Optional message to user when terminating/cancelling backend
@ 2017-06-19 18:40  Pavel Stehule <[email protected]>
  parent: Daniel Gustafsson <[email protected]>
  3 siblings, 1 reply; 46+ messages in thread

From: Pavel Stehule @ 2017-06-19 18:40 UTC (permalink / raw)
  To: Daniel Gustafsson <[email protected]>; +Cc: pgsql-hackers

2017-06-19 20:24 GMT+02:00 Daniel Gustafsson <[email protected]>:

> When terminating, or cancelling, a backend it’s currently not possible to
> let
> the signalled session know *why* it was dropped.  This has nagged me in the
> past and now it happened to come up again, so I took a stab at this.  The
> attached patch implements the ability to pass an optional text message to
> the
> signalled session which is included in the error message:
>
>   SELECT pg_terminate_backend(<pid> [, message]);
>   SELECT pg_cancel_backend(<pid> [, message]);
>
> Right now the message is simply appended on the error message, not sure if
> errdetail or errhint would be better? Calling:
>
>   select pg_terminate_backend(<pid>, 'server rebooting');
>
> ..leads to:
>
>   FATAL:  terminating connection due to administrator command: "server
> rebooting"
>
> Omitting the message invokes the command just like today.
>
> The message is stored in a new shmem area which is checked when the
> session is
> aborted.  To keep things simple a small buffer is kept per backend for the
> message.  If deemed too costly, keeping a central buffer from which slabs
> are
> allocated can be done (but seemed rather complicated for little gain
> compared
> to the quite moderate memory spend.)
>
> cheers ./daniel
>

+1

very good idea

Pavel


>
>
>
> --
> Sent via pgsql-hackers mailing list ([email protected])
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
>


^ permalink  raw  reply  [nested|flat] 46+ messages in thread

* Re: Optional message to user when terminating/cancelling backend
@ 2017-06-19 19:08  Satyanarayana Narlapuram <[email protected]>
  parent: Pavel Stehule <[email protected]>
  0 siblings, 1 reply; 46+ messages in thread

From: Satyanarayana Narlapuram @ 2017-06-19 19:08 UTC (permalink / raw)
  To: Pavel Stehule <[email protected]>; Daniel Gustafsson <[email protected]>; +Cc: pgsql-hackers

+1.

This really helps PostgreSQL Azure service as well. When we are doing the upgrades to the service, instead of abruptly terminating the sessions we can provide this message.

Thanks,
Satya

From: [email protected] [mailto:[email protected]] On Behalf Of Pavel Stehule
Sent: Monday, June 19, 2017 11:41 AM
To: Daniel Gustafsson <[email protected]>
Cc: PostgreSQL mailing lists <[email protected]>
Subject: Re: [HACKERS] Optional message to user when terminating/cancelling backend



2017-06-19 20:24 GMT+02:00 Daniel Gustafsson <[email protected]<mailto:[email protected]>>:
When terminating, or cancelling, a backend it’s currently not possible to let
the signalled session know *why* it was dropped.  This has nagged me in the
past and now it happened to come up again, so I took a stab at this.  The
attached patch implements the ability to pass an optional text message to the
signalled session which is included in the error message:

  SELECT pg_terminate_backend(<pid> [, message]);
  SELECT pg_cancel_backend(<pid> [, message]);

Right now the message is simply appended on the error message, not sure if
errdetail or errhint would be better? Calling:

  select pg_terminate_backend(<pid>, 'server rebooting');

..leads to:

  FATAL:  terminating connection due to administrator command: "server rebooting"

Omitting the message invokes the command just like today.

The message is stored in a new shmem area which is checked when the session is
aborted.  To keep things simple a small buffer is kept per backend for the
message.  If deemed too costly, keeping a central buffer from which slabs are
allocated can be done (but seemed rather complicated for little gain compared
to the quite moderate memory spend.)

cheers ./daniel

+1

very good idea

Pavel




--
Sent via pgsql-hackers mailing list ([email protected]<mailto:[email protected]>)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers<https://na01.safelinks.protection.outlook.com/?u...;



^ permalink  raw  reply  [nested|flat] 46+ messages in thread

* Re: Optional message to user when terminating/cancelling backend
@ 2017-06-20 18:54  Alvaro Herrera <[email protected]>
  parent: Satyanarayana Narlapuram <[email protected]>
  0 siblings, 2 replies; 46+ messages in thread

From: Alvaro Herrera @ 2017-06-20 18:54 UTC (permalink / raw)
  To: Satyanarayana Narlapuram <[email protected]>; +Cc: Pavel Stehule <[email protected]>; Daniel Gustafsson <[email protected]>; pgsql-hackers

Satyanarayana Narlapuram wrote:
> +1.
> 
> This really helps PostgreSQL Azure service as well. When we are doing
> the upgrades to the service, instead of abruptly terminating the
> sessions we can provide this message.

I think you mean "in addition to" rather than "instead of".

Unless you have a lot of users running psql manually, I don't see how
this is actually very useful or actionable.  What would the user do with
the information?  Hopefully your users already trust that you'd keep the
downtime to the minimum possible.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers



^ permalink  raw  reply  [nested|flat] 46+ messages in thread

* Re: Optional message to user when terminating/cancelling backend
@ 2017-06-20 19:43  David G. Johnston <[email protected]>
  parent: Alvaro Herrera <[email protected]>
  1 sibling, 1 reply; 46+ messages in thread

From: David G. Johnston @ 2017-06-20 19:43 UTC (permalink / raw)
  To: Alvaro Herrera <[email protected]>; +Cc: Satyanarayana Narlapuram <[email protected]>; Pavel Stehule <[email protected]>; Daniel Gustafsson <[email protected]>; pgsql-hackers

On Tue, Jun 20, 2017 at 11:54 AM, Alvaro Herrera
<[email protected]> wrote:
> Satyanarayana Narlapuram wrote:

> Unless you have a lot of users running psql manually, I don't see how
> this is actually very useful or actionable.  What would the user do with
> the information?  Hopefully your users already trust that you'd keep the
> downtime to the minimum possible.
>

Why wouldn't this be useful in application logs?  Spurious dropped
connections during application execution would be alarming.  Seeing a
message from the DBA when looking into those would be a painless and
quick way to alleviate stress.

pg_cancel_backend(<pid>, 'please try not to leave sessions in an "idle
in transaction" state...') would also seem like a useful message to
communicate; to user or application.  Sure, some of this can, and
maybe would also need to, be done out-of-band but this communication
channel seems worthy enough to at least evaluate the provided
implementation.

David J.


-- 
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers



^ permalink  raw  reply  [nested|flat] 46+ messages in thread

* Re: Optional message to user when terminating/cancelling backend
@ 2017-06-20 19:49  Satyanarayana Narlapuram <[email protected]>
  parent: David G. Johnston <[email protected]>
  0 siblings, 0 replies; 46+ messages in thread

From: Satyanarayana Narlapuram @ 2017-06-20 19:49 UTC (permalink / raw)
  To: David G. Johnston <[email protected]>; Alvaro Herrera <[email protected]>; +Cc: Pavel Stehule <[email protected]>; Daniel Gustafsson <[email protected]>; pgsql-hackers

Agree with David on the general usefulness of this channel. Not that Azure has this implementation or proposal today, but as a managed service this channel of communication is worth. For example, DBA / service can set a policy that if certain session exceeds the resource usage DBA can kill it and communicate the same. For example, too many locks, lot of IO activity, large open transactions etc. The messages will help application developer to tune their workloads appropriately. 

Thanks,
Satya

-----Original Message-----
From: David G. Johnston [mailto:[email protected]] 
Sent: Tuesday, June 20, 2017 12:44 PM
To: Alvaro Herrera <[email protected]>
Cc: Satyanarayana Narlapuram <[email protected]>; Pavel Stehule <[email protected]>; Daniel Gustafsson <[email protected]>; PostgreSQL mailing lists <[email protected]>
Subject: Re: [HACKERS] Optional message to user when terminating/cancelling backend

On Tue, Jun 20, 2017 at 11:54 AM, Alvaro Herrera <[email protected]> wrote:
> Satyanarayana Narlapuram wrote:

> Unless you have a lot of users running psql manually, I don't see how 
> this is actually very useful or actionable.  What would the user do 
> with the information?  Hopefully your users already trust that you'd 
> keep the downtime to the minimum possible.
>

Why wouldn't this be useful in application logs?  Spurious dropped connections during application execution would be alarming.  Seeing a message from the DBA when looking into those would be a painless and quick way to alleviate stress.

pg_cancel_backend(<pid>, 'please try not to leave sessions in an "idle in transaction" state...') would also seem like a useful message to communicate; to user or application.  Sure, some of this can, and maybe would also need to, be done out-of-band but this communication channel seems worthy enough to at least evaluate the provided implementation.

David J.

-- 
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


^ permalink  raw  reply  [nested|flat] 46+ messages in thread

* Re: Optional message to user when terminating/cancelling backend
@ 2017-06-20 20:01  Andres Freund <[email protected]>
  parent: Daniel Gustafsson <[email protected]>
  3 siblings, 0 replies; 46+ messages in thread

From: Andres Freund @ 2017-06-20 20:01 UTC (permalink / raw)
  To: Daniel Gustafsson <[email protected]>; +Cc: pgsql-hackers

Hi,

On 2017-06-19 20:24:43 +0200, Daniel Gustafsson wrote:
> When terminating, or cancelling, a backend it’s currently not possible to let
> the signalled session know *why* it was dropped.  This has nagged me in the
> past and now it happened to come up again, so I took a stab at this.  The
> attached patch implements the ability to pass an optional text message to the
> signalled session which is included in the error message:
> 
>   SELECT pg_terminate_backend(<pid> [, message]);
>   SELECT pg_cancel_backend(<pid> [, message]);
> 
> Right now the message is simply appended on the error message, not sure if
> errdetail or errhint would be better? Calling:
> 
>   select pg_terminate_backend(<pid>, 'server rebooting');
> 
> ..leads to:
> 
>   FATAL:  terminating connection due to administrator command: "server rebooting"
> 
> Omitting the message invokes the command just like today.

For extensions it'd also be useful if it'd be possible to overwrite the
error code.  E.g. for citus there's a distributed deadlock detector,
running out of process because there's no way to interrupt lock waits
locally, and we've to do some ugly hacking to generate proper error
messages and code from another session.

- Andres


-- 
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers



^ permalink  raw  reply  [nested|flat] 46+ messages in thread

* Re: Optional message to user when terminating/cancelling backend
@ 2017-06-21 03:06  Michael Paquier <[email protected]>
  parent: Daniel Gustafsson <[email protected]>
  3 siblings, 1 reply; 46+ messages in thread

From: Michael Paquier @ 2017-06-21 03:06 UTC (permalink / raw)
  To: Daniel Gustafsson <[email protected]>; +Cc: pgsql-hackers

On Tue, Jun 20, 2017 at 3:24 AM, Daniel Gustafsson <[email protected]> wrote:
> The message is stored in a new shmem area which is checked when the session is
> aborted.  To keep things simple a small buffer is kept per backend for the
> message.  If deemed too costly, keeping a central buffer from which slabs are
> allocated can be done (but seemed rather complicated for little gain compared
> to the quite moderate memory spend.)

I think that you are right to take the approach with a per-backend
slot. This will avoid complications related to entry removals and
locking issues. There would be scaling issues as well if things get
very signaled for a lot of backends.

+#define MAX_CANCEL_MSG 128
That looks enough.

+           LWLockAcquire(BackendCancelLock, LW_EXCLUSIVE);
+
+           strlcpy(slot->message, message, sizeof(slot->message));
+           slot->len = strlen(message);
Why not using one spin lock per slot and save it in BackendCancelShmemStruct?

+   pid_t       pid = PG_GETARG_INT32(0);
+   char       *msg = text_to_cstring(PG_GETARG_TEXT_PP(1));
+
+   PG_RETURN_BOOL(pg_terminate_backend_internal(pid, msg));
It would be more solid to add some error handling for messages that
are too long, or at least truncate the message if too long.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers



^ permalink  raw  reply  [nested|flat] 46+ messages in thread

* Re: Optional message to user when terminating/cancelling backend
@ 2017-06-21 13:57  Yugo Nagata <[email protected]>
  parent: Daniel Gustafsson <[email protected]>
  3 siblings, 0 replies; 46+ messages in thread

From: Yugo Nagata @ 2017-06-21 13:57 UTC (permalink / raw)
  To: Daniel Gustafsson <[email protected]>; +Cc: pgsql-hackers

Hi,

Here are some comments for the patch.

+Datum
+pg_cancel_backend(PG_FUNCTION_ARGS)
+{
+   PG_RETURN_BOOL(pg_cancel_backend_internal(PG_GETARG_INT32(0), NULL));
+}
+Datum
+pg_cancel_backend_msg(PG_FUNCTION_ARGS)
+{
+   pid_t       pid = PG_GETARG_INT32(0);
+   char       *msg = text_to_cstring(PG_GETARG_TEXT_PP(1));
+
+   PG_RETURN_BOOL(pg_cancel_backend_internal(pid, msg));
+}

It would be better to insert a blank line between these functions.

+/*
+ * Sets a cancellation message for the backend with the specified pid and
+ * returns the length of message actually created. If the returned length
+ * is equal to the length of the message parameter, truncation may have
+ * occurred. If the backend wasn't found and no message was set, -1 is
+ * returned.
+ */

It seems to me that this comment is incorrect.

 "If the returned length is not equal to the length of the message parameter,"

is right, isn't it?

In addition, the last statement would be 

 "If the backend wasn't found, -1 is returned. Otherwize, if no message was set,
  0 is returned."


+           strlcpy(slot->message, message, sizeof(slot->message));
+           slot->len = strlen(message);
+
+           LWLockRelease(BackendCancelLock);
+           return slot->len;

If SetBackendCancelMessage() has to return the length of message actually created,
slot->len should be strlen(slot->message) instead of strlen(message).
In the current code, when the return value and slot->len is always set
to the length of the passed message parameter.


Regards,

On Mon, 19 Jun 2017 20:24:43 +0200
Daniel Gustafsson <[email protected]> wrote:

> When terminating, or cancelling, a backend it’s currently not possible to let
> the signalled session know *why* it was dropped.  This has nagged me in the
> past and now it happened to come up again, so I took a stab at this.  The
> attached patch implements the ability to pass an optional text message to the
> signalled session which is included in the error message:
> 
>   SELECT pg_terminate_backend(<pid> [, message]);
>   SELECT pg_cancel_backend(<pid> [, message]);
> 
> Right now the message is simply appended on the error message, not sure if
> errdetail or errhint would be better? Calling:
> 
>   select pg_terminate_backend(<pid>, 'server rebooting');
> 
> ..leads to:
> 
>   FATAL:  terminating connection due to administrator command: "server rebooting"
> 
> Omitting the message invokes the command just like today.
> 
> The message is stored in a new shmem area which is checked when the session is
> aborted.  To keep things simple a small buffer is kept per backend for the
> message.  If deemed too costly, keeping a central buffer from which slabs are
> allocated can be done (but seemed rather complicated for little gain compared
> to the quite moderate memory spend.) 
> 
> cheers ./daniel
> 


-- 
Yugo Nagata <[email protected]>


-- 
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers



^ permalink  raw  reply  [nested|flat] 46+ messages in thread

* Re: Optional message to user when terminating/cancelling backend
@ 2017-06-21 14:30  Yugo Nagata <[email protected]>
  parent: Michael Paquier <[email protected]>
  0 siblings, 1 reply; 46+ messages in thread

From: Yugo Nagata @ 2017-06-21 14:30 UTC (permalink / raw)
  To: Michael Paquier <[email protected]>; +Cc: Daniel Gustafsson <[email protected]>; pgsql-hackers

On Wed, 21 Jun 2017 12:06:33 +0900
Michael Paquier <[email protected]> wrote:

> On Tue, Jun 20, 2017 at 3:24 AM, Daniel Gustafsson <[email protected]> wrote:
> > The message is stored in a new shmem area which is checked when the session is
> > aborted.  To keep things simple a small buffer is kept per backend for the
> > message.  If deemed too costly, keeping a central buffer from which slabs are
> > allocated can be done (but seemed rather complicated for little gain compared
> > to the quite moderate memory spend.)
> 
> I think that you are right to take the approach with a per-backend
> slot. This will avoid complications related to entry removals and
> locking issues. There would be scaling issues as well if things get
> very signaled for a lot of backends.
> 
> +#define MAX_CANCEL_MSG 128
> That looks enough.
> 
> +           LWLockAcquire(BackendCancelLock, LW_EXCLUSIVE);
> +
> +           strlcpy(slot->message, message, sizeof(slot->message));
> +           slot->len = strlen(message);
> Why not using one spin lock per slot and save it in BackendCancelShmemStruct?

+1

I found an example that a spin lock is used during strlcpy in WalReceiverMain().

> 
> +   pid_t       pid = PG_GETARG_INT32(0);
> +   char       *msg = text_to_cstring(PG_GETARG_TEXT_PP(1));
> +
> +   PG_RETURN_BOOL(pg_terminate_backend_internal(pid, msg));
> It would be more solid to add some error handling for messages that
> are too long, or at least truncate the message if too long.

I agree that error handling for too long messages is needed.
Although long messages are truncated in SetBackendCancelMessage(),
it is better to inform users that the message they can read was
truncated one. Or, maybe we should prohibit too long message
is passed in pg_teminate_backend()


> -- 
> Michael
> 
> 
> -- 
> Sent via pgsql-hackers mailing list ([email protected])
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers


-- 
Yugo Nagata <[email protected]>


-- 
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers



^ permalink  raw  reply  [nested|flat] 46+ messages in thread

* Re: Optional message to user when terminating/cancelling backend
@ 2017-06-21 14:42  Daniel Gustafsson <[email protected]>
  parent: Yugo Nagata <[email protected]>
  0 siblings, 1 reply; 46+ messages in thread

From: Daniel Gustafsson @ 2017-06-21 14:42 UTC (permalink / raw)
  To: Yugo Nagata <[email protected]>; +Cc: Michael Paquier <[email protected]>; pgsql-hackers

> On 21 Jun 2017, at 16:30, Yugo Nagata <[email protected]> wrote:
> 
> On Wed, 21 Jun 2017 12:06:33 +0900
> Michael Paquier <[email protected]> wrote:
> 
>> On Tue, Jun 20, 2017 at 3:24 AM, Daniel Gustafsson <[email protected]> wrote:
>>> The message is stored in a new shmem area which is checked when the session is
>>> aborted.  To keep things simple a small buffer is kept per backend for the
>>> message.  If deemed too costly, keeping a central buffer from which slabs are
>>> allocated can be done (but seemed rather complicated for little gain compared
>>> to the quite moderate memory spend.)
>> 
>> I think that you are right to take the approach with a per-backend
>> slot. This will avoid complications related to entry removals and
>> locking issues. There would be scaling issues as well if things get
>> very signaled for a lot of backends.
>> 
>> +#define MAX_CANCEL_MSG 128
>> That looks enough.
>> 
>> +           LWLockAcquire(BackendCancelLock, LW_EXCLUSIVE);
>> +
>> +           strlcpy(slot->message, message, sizeof(slot->message));
>> +           slot->len = strlen(message);
>> Why not using one spin lock per slot and save it in BackendCancelShmemStruct?
> 
> +1
> 
> I found an example that a spin lock is used during strlcpy in WalReceiverMain().

Yeah I agree as well, will fix.

>> +   pid_t       pid = PG_GETARG_INT32(0);
>> +   char       *msg = text_to_cstring(PG_GETARG_TEXT_PP(1));
>> +
>> +   PG_RETURN_BOOL(pg_terminate_backend_internal(pid, msg));
>> It would be more solid to add some error handling for messages that
>> are too long, or at least truncate the message if too long.
> 
> I agree that error handling for too long messages is needed.
> Although long messages are truncated in SetBackendCancelMessage(),
> it is better to inform users that the message they can read was
> truncated one. Or, maybe we should prohibit too long message
> is passed in pg_teminate_backend()

The message is truncated in SetBackendCancelMessage() for safety, but
pg_{cancel|terminate}_backend() could throw an error on too long message, or
warning truncation, to the caller as well.  Personally I think a warning is the
appropriate response, but I don’t really have a strong opinion.

cheers ./daniel

-- 
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers



^ permalink  raw  reply  [nested|flat] 46+ messages in thread

* Re: Optional message to user when terminating/cancelling backend
@ 2017-06-22 00:24  Michael Paquier <[email protected]>
  parent: Daniel Gustafsson <[email protected]>
  0 siblings, 1 reply; 46+ messages in thread

From: Michael Paquier @ 2017-06-22 00:24 UTC (permalink / raw)
  To: Daniel Gustafsson <[email protected]>; +Cc: Yugo Nagata <[email protected]>; pgsql-hackers

On Wed, Jun 21, 2017 at 11:42 PM, Daniel Gustafsson <[email protected]> wrote:
> The message is truncated in SetBackendCancelMessage() for safety, but
> pg_{cancel|terminate}_backend() could throw an error on too long message, or
> warning truncation, to the caller as well.  Personally I think a warning is the
> appropriate response, but I don’t really have a strong opinion.

And a NOTICE? That's what happens for relation name truncation. You
are right that having a check in SetBackendCancelMessage() makes the
most sense as bgworkers could just call the low level API. Isn't the
concept actually closer to just a backend message? This slot could be
used for other purposes than cancellation.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers



^ permalink  raw  reply  [nested|flat] 46+ messages in thread

* Re: Optional message to user when terminating/cancelling backend
@ 2017-06-22 08:24  Yugo Nagata <[email protected]>
  parent: Michael Paquier <[email protected]>
  0 siblings, 1 reply; 46+ messages in thread

From: Yugo Nagata @ 2017-06-22 08:24 UTC (permalink / raw)
  To: Michael Paquier <[email protected]>; +Cc: Daniel Gustafsson <[email protected]>; pgsql-hackers

On Thu, 22 Jun 2017 09:24:54 +0900
Michael Paquier <[email protected]> wrote:

> On Wed, Jun 21, 2017 at 11:42 PM, Daniel Gustafsson <[email protected]> wrote:
> > The message is truncated in SetBackendCancelMessage() for safety, but
> > pg_{cancel|terminate}_backend() could throw an error on too long message, or
> > warning truncation, to the caller as well.  Personally I think a warning is the
> > appropriate response, but I don’t really have a strong opinion.
> 
> And a NOTICE? That's what happens for relation name truncation. You
> are right that having a check in SetBackendCancelMessage() makes the
> most sense as bgworkers could just call the low level API. Isn't the
> concept actually closer to just a backend message? This slot could be
> used for other purposes than cancellation.

+1 for NOTICE. The message truncation seems to be a kind of helpful
information rather than a likely problem as long as pg_terminated_backend
exits successfully.

https://www.postgresql.org/docs/10/static/runtime-config-logging.html#runtime-config-severity-levels

> -- 
> Michael


-- 
Yugo Nagata <[email protected]>


-- 
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers



^ permalink  raw  reply  [nested|flat] 46+ messages in thread

* Re: Optional message to user when terminating/cancelling backend
@ 2017-06-22 13:48  Daniel Gustafsson <[email protected]>
  parent: Yugo Nagata <[email protected]>
  0 siblings, 2 replies; 46+ messages in thread

From: Daniel Gustafsson @ 2017-06-22 13:48 UTC (permalink / raw)
  To: Yugo Nagata <[email protected]>; Michael Paquier <[email protected]>; +Cc: pgsql-hackers

> On 22 Jun 2017, at 10:24, Yugo Nagata <[email protected]> wrote:
> 
> On Thu, 22 Jun 2017 09:24:54 +0900
> Michael Paquier <[email protected]> wrote:
> 
>> On Wed, Jun 21, 2017 at 11:42 PM, Daniel Gustafsson <[email protected]> wrote:
>>> The message is truncated in SetBackendCancelMessage() for safety, but
>>> pg_{cancel|terminate}_backend() could throw an error on too long message, or
>>> warning truncation, to the caller as well.  Personally I think a warning is the
>>> appropriate response, but I don’t really have a strong opinion.
>> 
>> And a NOTICE? That's what happens for relation name truncation. You
>> are right that having a check in SetBackendCancelMessage() makes the
>> most sense as bgworkers could just call the low level API. Isn't the
>> concept actually closer to just a backend message? This slot could be
>> used for other purposes than cancellation.
> 
> +1 for NOTICE. The message truncation seems to be a kind of helpful
> information rather than a likely problem as long as pg_terminated_backend
> exits successfully.
> 
> https://www.postgresql.org/docs/10/static/runtime-config-logging.html#runtime-config-severity-levels

Good point.  I’ve attached a new version which issues a NOTICE on truncation
and also addresses all other comments so far in this thread.  Thanks a lot for
the early patch reviews!

cheers ./daniel



^ permalink  raw  reply  [nested|flat] 46+ messages in thread

* Re: Optional message to user when terminating/cancelling backend
@ 2017-06-22 15:52  Dilip Kumar <[email protected]>
  parent: Daniel Gustafsson <[email protected]>
  1 sibling, 1 reply; 46+ messages in thread

From: Dilip Kumar @ 2017-06-22 15:52 UTC (permalink / raw)
  To: Daniel Gustafsson <[email protected]>; +Cc: Yugo Nagata <[email protected]>; Michael Paquier <[email protected]>; pgsql-hackers

On Thu, Jun 22, 2017 at 7:18 PM, Daniel Gustafsson <[email protected]> wrote:
> Good point.  I’ve attached a new version which issues a NOTICE on truncation
> and also addresses all other comments so far in this thread.  Thanks a lot for
> the early patch reviews!
>
> cheers ./daniel

I have done an initial review of the patch. I have some comments/suggestions.


+int
+GetCancelMessage(char **buffer, size_t buf_len)
+{
+ volatile BackendCancelShmemStruct *slot = MyCancelSlot;
+ int msg_length = 0;
+

Returned value of this function is never used, better to convert it to
just void.

-------

+bool
+HasCancelMessage(void)
+{
+ volatile BackendCancelShmemStruct *slot = MyCancelSlot;


+/*
+ * Return the configured cancellation message and its length. If the returned
+ * length is greater than the size of the passed buffer, truncation has been
+ * performed. The message is cleared on reading.
+ */
+int
+GetCancelMessage(char **buffer, size_t buf_len)

I think it will be good to merge these two function where
GetCancelMessage will first check whether it has the message or not
if it does then allocate the buffer of size slot->len and copy the
slot message to allocated buffer otherwise just return NULL.

So it will look like "char *GetCancelMessage()"

---------

+ SpinLockAcquire(&slot->mutex);
+ strlcpy(*buffer, (const char *) slot->message, buf_len);

strlcpy(*buffer, (const char *) slot->message, slot->len);

Isn't it better to copy only upto slot->len, seems like it will always
be <= buf_len, and if not then
we can do min(buf_len, slot->len)

----

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com


-- 
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers



^ permalink  raw  reply  [nested|flat] 46+ messages in thread

* Re: Optional message to user when terminating/cancelling backend
@ 2017-06-26 07:44  Daniel Gustafsson <[email protected]>
  parent: Dilip Kumar <[email protected]>
  0 siblings, 0 replies; 46+ messages in thread

From: Daniel Gustafsson @ 2017-06-26 07:44 UTC (permalink / raw)
  To: Dilip Kumar <[email protected]>; +Cc: Yugo Nagata <[email protected]>; Michael Paquier <[email protected]>; pgsql-hackers

> On 22 Jun 2017, at 17:52, Dilip Kumar <[email protected]> wrote:
> 
> On Thu, Jun 22, 2017 at 7:18 PM, Daniel Gustafsson <[email protected]> wrote:
>> Good point.  I’ve attached a new version which issues a NOTICE on truncation
>> and also addresses all other comments so far in this thread.  Thanks a lot for
>> the early patch reviews!
>> 
>> cheers ./daniel
> 
> I have done an initial review of the patch. I have some comments/suggestions.

Thanks!

> +int
> +GetCancelMessage(char **buffer, size_t buf_len)
> +{
> + volatile BackendCancelShmemStruct *slot = MyCancelSlot;
> + int msg_length = 0;
> +
> 
> Returned value of this function is never used, better to convert it to
> just void.

You’re probably right, I was thinking that someone might be interested in
knowing about truncation when extracting the message but I can’t really think
of a callsite which wouldn’t just pass in a large enough buffer in the first
place.

> +bool
> +HasCancelMessage(void)
> +{
> + volatile BackendCancelShmemStruct *slot = MyCancelSlot;
> 
> +/*
> + * Return the configured cancellation message and its length. If the returned
> + * length is greater than the size of the passed buffer, truncation has been
> + * performed. The message is cleared on reading.
> + */
> +int
> +GetCancelMessage(char **buffer, size_t buf_len)
> 
> I think it will be good to merge these two function where
> GetCancelMessage will first check whether it has the message or not
> if it does then allocate the buffer of size slot->len and copy the
> slot message to allocated buffer otherwise just return NULL.
> 
> So it will look like "char *GetCancelMessage()”

It doesn’t seem like a good idea to perform memory allocation inside a spinlock
in a signalled backend, that would probably at least require an LWLock wouldn’t
it?  It seems safer to leave memory management to the signalled backend but it
may be paranoia on my part.

> + SpinLockAcquire(&slot->mutex);
> + strlcpy(*buffer, (const char *) slot->message, buf_len);
> 
> strlcpy(*buffer, (const char *) slot->message, slot->len);
> 
> Isn't it better to copy only upto slot->len, seems like it will always
> be <= buf_len, and if not then
> we can do min(buf_len, slot->len)

strlcpy(3) is defined as taking the size of the passed buffer and not the
copied string.  Since we guarantee that slot->message is NUL terminated it
seems wise to stick to the API. Or did I misunderstand your comment?

cheers ./daniel

-- 
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers



^ permalink  raw  reply  [nested|flat] 46+ messages in thread

* Re: Optional message to user when terminating/cancelling backend
@ 2017-06-26 14:15  Joel Jacobson <[email protected]>
  parent: Alvaro Herrera <[email protected]>
  1 sibling, 1 reply; 46+ messages in thread

From: Joel Jacobson @ 2017-06-26 14:15 UTC (permalink / raw)
  To: Alvaro Herrera <[email protected]>; +Cc: Satyanarayana Narlapuram <[email protected]>; Pavel Stehule <[email protected]>; Daniel Gustafsson <[email protected]>; pgsql-hackers

+1

On Tue, Jun 20, 2017 at 8:54 PM, Alvaro Herrera
<[email protected]> wrote:
> Unless you have a lot of users running psql manually, I don't see how
> this is actually very useful or actionable.  What would the user do with
> the information?  Hopefully your users already trust that you'd keep the
> downtime to the minimum possible.

I think this feature would be useful for PgTerminator
(https://github.com/trustly/pgterminator)
a tool which automatically kills unprotected processes that could
potentially be the reason why
>X number of protected important processes have been waiting for >Y seconds.

When I'm guilty of locking this in the production DB and get killed by
PgTerminator,
it would be nice to know the reason, e.g. that it was PgTerminator
that killed me
and what process I was blocking.


-- 
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers



^ permalink  raw  reply  [nested|flat] 46+ messages in thread

* Re: Optional message to user when terminating/cancelling backend
@ 2017-06-26 14:39  Joshua D. Drake <[email protected]>
  parent: Joel Jacobson <[email protected]>
  0 siblings, 0 replies; 46+ messages in thread

From: Joshua D. Drake @ 2017-06-26 14:39 UTC (permalink / raw)
  To: Joel Jacobson <[email protected]>; Alvaro Herrera <[email protected]>; +Cc: Satyanarayana Narlapuram <[email protected]>; Pavel Stehule <[email protected]>; Daniel Gustafsson <[email protected]>; pgsql-hackers

On 06/26/2017 07:15 AM, Joel Jacobson wrote:
> +1
> 
> On Tue, Jun 20, 2017 at 8:54 PM, Alvaro Herrera
> <[email protected]> wrote:
>> Unless you have a lot of users running psql manually, I don't see how
>> this is actually very useful or actionable.  What would the user do with
>> the information?  Hopefully your users already trust that you'd keep the
>> downtime to the minimum possible.
> 
> I think this feature would be useful for PgTerminator
> (https://github.com/trustly/pgterminator)
> a tool which automatically kills unprotected processes that could
> potentially be the reason why
>> X number of protected important processes have been waiting for >Y seconds.
> 
> When I'm guilty of locking this in the production DB and get killed by
> PgTerminator,
> it would be nice to know the reason, e.g. that it was PgTerminator
> that killed me
> and what process I was blocking.

And not just the pid but literally "what".

jD


-- 
Command Prompt, Inc. || http://the.postgres.company/ || @cmdpromptinc

PostgreSQL Centered full stack support, consulting and development.
Advocate: @amplifypostgres || Learn: https://pgconf.us
*****     Unless otherwise stated, opinions are my own.   *****


-- 
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers



^ permalink  raw  reply  [nested|flat] 46+ messages in thread

* Re: Optional message to user when terminating/cancelling backend
@ 2017-09-02 00:21  Thomas Munro <[email protected]>
  parent: Daniel Gustafsson <[email protected]>
  1 sibling, 1 reply; 46+ messages in thread

From: Thomas Munro @ 2017-09-02 00:21 UTC (permalink / raw)
  To: Daniel Gustafsson <[email protected]>; +Cc: Yugo Nagata <[email protected]>; Michael Paquier <[email protected]>; pgsql-hackers

On Fri, Jun 23, 2017 at 1:48 AM, Daniel Gustafsson <[email protected]> wrote:
> Good point.  I’ve attached a new version which issues a NOTICE on truncation
> and also addresses all other comments so far in this thread.  Thanks a lot for
> the early patch reviews!

FYI this no longer builds because commit 81c5e46c490e just stole your OIDs:

make[3]: Entering directory
`/home/travis/build/postgresql-cfbot/postgresql/src/backend/catalog'
cd ../../../src/include/catalog && '/usr/bin/perl' ./duplicate_oids
772
972
make[3]: *** [postgres.bki] Error 1

-- 
Thomas Munro
http://www.enterprisedb.com


-- 
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers



^ permalink  raw  reply  [nested|flat] 46+ messages in thread

* Re: Optional message to user when terminating/cancelling backend
@ 2017-09-03 20:47  Daniel Gustafsson <[email protected]>
  parent: Thomas Munro <[email protected]>
  0 siblings, 1 reply; 46+ messages in thread

From: Daniel Gustafsson @ 2017-09-03 20:47 UTC (permalink / raw)
  To: Thomas Munro <[email protected]>; +Cc: Yugo Nagata <[email protected]>; Michael Paquier <[email protected]>; pgsql-hackers

> On 02 Sep 2017, at 02:21, Thomas Munro <[email protected]> wrote:
> 
> On Fri, Jun 23, 2017 at 1:48 AM, Daniel Gustafsson <[email protected]> wrote:
>> Good point.  I’ve attached a new version which issues a NOTICE on truncation
>> and also addresses all other comments so far in this thread.  Thanks a lot for
>> the early patch reviews!
> 
> FYI this no longer builds because commit 81c5e46c490e just stole your OIDs:
> 
> make[3]: Entering directory
> `/home/travis/build/postgresql-cfbot/postgresql/src/backend/catalog'
> cd ../../../src/include/catalog && '/usr/bin/perl' ./duplicate_oids
> 772
> 972
> make[3]: *** [postgres.bki] Error 1

Thanks, I hadn’t spotted that yet.  Attached is an updated patch using new OIDs
to make it compile again.

cheers ./daniel



^ permalink  raw  reply  [nested|flat] 46+ messages in thread

* Re: Optional message to user when terminating/cancelling backend
@ 2017-09-28 12:55  Yugo Nagata <[email protected]>
  parent: Daniel Gustafsson <[email protected]>
  0 siblings, 0 replies; 46+ messages in thread

From: Yugo Nagata @ 2017-09-28 12:55 UTC (permalink / raw)
  To: Daniel Gustafsson <[email protected]>; +Cc: Thomas Munro <[email protected]>; Michael Paquier <[email protected]>; pgsql-hackers

On Sun, 3 Sep 2017 22:47:10 +0200
Daniel Gustafsson <[email protected]> wrote:

I have reviewed your latest patch. 

I can apply this to the master branch and build this successfully,
and the behavior is as expected. 

However, here are some comments and suggestions.

> 135 +               char   *buffer = palloc0(MAX_CANCEL_MSG);
> 136 +
> 137 +               GetCancelMessage(&buffer, MAX_CANCEL_MSG);
> 138 +               ereport(ERROR,
> 139 +                       (errcode(ERRCODE_QUERY_CANCELED),
> 140 +                        errmsg("canceling statement due to user request: \"%s\"",
> 141 +                               buffer)));

The memory for buffer is allocated here, but I think we can do this
in GetCancelMessage. Since the size of allocated memory is fixed
to MAX_CANCEL_MSG, it isn't neccesary to pass this to the function.
In addition, how about returning the message as the return value?
That is, we can define GetCancelMessage as following;

  char * GetCancelMessage(void)

> 180 +       r = SetBackendCancelMessage(pid, msg);
> 181 +
> 182 +       if (r != -1 && r != strlen(msg))
> 183 +           ereport(NOTICE,
> 184 +                   (errmsg("message is too long and has been truncated")));
> 185 +   }

We can this error handling in SetBackendCancelMessage. I think this is better
because the truncation of message is done in this function. In addition, 
we should use pg_mbcliplen for this truncation as done in truncate_identifier. 
Else, multibyte character boundary is broken, and noises can slip in log
messages.

> 235 -   int         r = pg_signal_backend(PG_GETARG_INT32(0), SIGTERM);
> 236 +   int r = pg_signal_backend(pid, SIGTERM, msg);

This line includes an unnecessary indentation change.

> 411 + * returns the length of message actually created. If the returned length

"the length of message" might be "the length of the message"

> 413 + * If the backend wasn't found and no message was set, -1 is returned. If two

This comment is incorrect since this function returns 0 when no message was set.

> 440 +           strlcpy(slot->message, message, sizeof(slot->message));
> 441 +           slot->len = strlen(slot->message);
> 442 +           message_len = slot->len;
> 443 +           SpinLockRelease(&slot->mutex);
> 444 +
> 445 +           return message_len;

This can return slot->len directly and the variable message_len is
unnecessary. However, if we handle the "too long message" error
in this function as suggested above, this does not have to
return anything.

Regards,


-- 
Yugo Nagata <[email protected]>



> > On 02 Sep 2017, at 02:21, Thomas Munro <[email protected]> wrote:
> > 
> > On Fri, Jun 23, 2017 at 1:48 AM, Daniel Gustafsson <[email protected]> wrote:
> >> Good point.  I’ve attached a new version which issues a NOTICE on truncation
> >> and also addresses all other comments so far in this thread.  Thanks a lot for
> >> the early patch reviews!"
> > 
> > FYI this no longer builds because commit 81c5e46c490e just stole your OIDs:
> > 
> > make[3]: Entering directory
> > `/home/travis/build/postgresql-cfbot/postgresql/src/backend/catalog'
> > cd ../../../src/include/catalog && '/usr/bin/perl' ./duplicate_oids
> > 772
> > 972
> > make[3]: *** [postgres.bki] Error 1
> 
> Thanks, I hadn’t spotted that yet.  Attached is an updated patch using new OIDs
> to make it compile again.
> 
> cheers ./daniel
> 


-- 
Yugo Nagata <[email protected]>


-- 
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers



^ permalink  raw  reply  [nested|flat] 46+ messages in thread

* [PATCH v3 4/5] Fix bugs in the commit to change conversion function signature.
@ 2021-02-01 15:30  Heikki Linnakangas <[email protected]>
  0 siblings, 0 replies; 46+ messages in thread

From: Heikki Linnakangas @ 2021-02-01 15:30 UTC (permalink / raw)

Per John Naylor's report, and some bugs I found while testing.

TODO: to be squashed.
---
 src/backend/utils/mb/conv.c                      | 16 ++++++----------
 .../utf8_and_iso8859_1/utf8_and_iso8859_1.c      |  8 ++++++++
 src/backend/utils/mb/mbutils.c                   |  5 +++--
 3 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/src/backend/utils/mb/conv.c b/src/backend/utils/mb/conv.c
index b83358bc7a5..f3bda5753d0 100644
--- a/src/backend/utils/mb/conv.c
+++ b/src/backend/utils/mb/conv.c
@@ -497,7 +497,6 @@ UtfToLocal(const unsigned char *utf, int len,
 	int			l;
 	const pg_utf_to_local_combined *cp;
 	const unsigned char *start = utf;
-	const unsigned char *cur = utf;
 
 	if (!PG_VALID_ENCODING(encoding))
 		ereport(ERROR,
@@ -511,8 +510,6 @@ UtfToLocal(const unsigned char *utf, int len,
 		unsigned char b3 = 0;
 		unsigned char b4 = 0;
 
-		cur = iso;
-
 		/* "break" cases all represent errors */
 		if (*utf == '\0')
 			break;
@@ -648,10 +645,11 @@ UtfToLocal(const unsigned char *utf, int len,
 		}
 
 		/* failed to translate this character */
+		utf -= l;
 		if (noError)
 			break;
 		report_untranslatable_char(PG_UTF8, encoding,
-								   (const char *) (utf - l), len);
+								   (const char *) utf, len);
 	}
 
 	/* if we broke out of loop early, must be invalid input */
@@ -660,7 +658,7 @@ UtfToLocal(const unsigned char *utf, int len,
 
 	*iso = '\0';
 
-	return cur - start;
+	return utf - start;
 }
 
 /*
@@ -701,7 +699,6 @@ LocalToUtf(const unsigned char *iso, int len,
 	int			l;
 	const pg_local_to_utf_combined *cp;
 	const unsigned char *start = iso;
-	const unsigned char *cur = iso;
 
 	if (!PG_VALID_ENCODING(encoding))
 		ereport(ERROR,
@@ -715,8 +712,6 @@ LocalToUtf(const unsigned char *iso, int len,
 		unsigned char b3 = 0;
 		unsigned char b4 = 0;
 
-		cur = iso;
-
 		/* "break" cases all represent errors */
 		if (*iso == '\0')
 			break;
@@ -799,10 +794,11 @@ LocalToUtf(const unsigned char *iso, int len,
 		}
 
 		/* failed to translate this character */
+		iso -= l;
 		if (noError)
 			break;
 		report_untranslatable_char(encoding, PG_UTF8,
-								   (const char *) (iso - l), len);
+								   (const char *) iso, len);
 	}
 
 	/* if we broke out of loop early, must be invalid input */
@@ -811,5 +807,5 @@ LocalToUtf(const unsigned char *iso, int len,
 
 	*utf = '\0';
 
-	return cur - start;
+	return iso - start;
 }
diff --git a/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c b/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
index 8ac93604a1b..d0dc4cca378 100644
--- a/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
+++ b/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
@@ -104,10 +104,18 @@ utf8_to_iso8859_1(PG_FUNCTION_ARGS)
 			int			l = pg_utf_mblen(src);
 
 			if (l > len || !pg_utf8_islegal(src, l))
+			{
+				if (noError)
+					break;
 				report_invalid_encoding(PG_UTF8, (const char *) src, len);
+			}
 			if (l != 2)
+			{
+				if (noError)
+					break;
 				report_untranslatable_char(PG_UTF8, PG_LATIN1,
 										   (const char *) src, len);
+			}
 			c1 = src[1] & 0x3f;
 			c = ((c & 0x1f) << 6) | c1;
 			if (c >= 0x80 && c <= 0xff)
diff --git a/src/backend/utils/mb/mbutils.c b/src/backend/utils/mb/mbutils.c
index 3e106027d75..af1cafe523a 100644
--- a/src/backend/utils/mb/mbutils.c
+++ b/src/backend/utils/mb/mbutils.c
@@ -818,12 +818,13 @@ perform_default_encoding_conversion(const char *src, int len,
 		MemoryContextAllocHuge(CurrentMemoryContext,
 							   (Size) len * MAX_CONVERSION_GROWTH + 1);
 
-	FunctionCall5(flinfo,
+	FunctionCall6(flinfo,
 				  Int32GetDatum(src_encoding),
 				  Int32GetDatum(dest_encoding),
 				  CStringGetDatum(src),
 				  CStringGetDatum(result),
-				  Int32GetDatum(len));
+				  Int32GetDatum(len),
+				  BoolGetDatum(false));
 
 	/*
 	 * Release extra space if there might be a lot --- see comments in
-- 
2.29.2


--------------39E46EF15EBFAA37D53332E5
Content-Type: text/x-patch; charset=UTF-8;
 name="v3-0005-Do-COPY-FROM-encoding-conversion-verification-in-.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename*0="v3-0005-Do-COPY-FROM-encoding-conversion-verification-in-.pa";
 filename*1="tch"



^ permalink  raw  reply  [nested|flat] 46+ messages in thread

* [PATCH v3 4/5] Fix bugs in the commit to change conversion function signature.
@ 2021-02-01 15:30  Heikki Linnakangas <[email protected]>
  0 siblings, 0 replies; 46+ messages in thread

From: Heikki Linnakangas @ 2021-02-01 15:30 UTC (permalink / raw)

Per John Naylor's report, and some bugs I found while testing.

TODO: to be squashed.
---
 src/backend/utils/mb/conv.c                      | 16 ++++++----------
 .../utf8_and_iso8859_1/utf8_and_iso8859_1.c      |  8 ++++++++
 src/backend/utils/mb/mbutils.c                   |  5 +++--
 3 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/src/backend/utils/mb/conv.c b/src/backend/utils/mb/conv.c
index b83358bc7a5..f3bda5753d0 100644
--- a/src/backend/utils/mb/conv.c
+++ b/src/backend/utils/mb/conv.c
@@ -497,7 +497,6 @@ UtfToLocal(const unsigned char *utf, int len,
 	int			l;
 	const pg_utf_to_local_combined *cp;
 	const unsigned char *start = utf;
-	const unsigned char *cur = utf;
 
 	if (!PG_VALID_ENCODING(encoding))
 		ereport(ERROR,
@@ -511,8 +510,6 @@ UtfToLocal(const unsigned char *utf, int len,
 		unsigned char b3 = 0;
 		unsigned char b4 = 0;
 
-		cur = iso;
-
 		/* "break" cases all represent errors */
 		if (*utf == '\0')
 			break;
@@ -648,10 +645,11 @@ UtfToLocal(const unsigned char *utf, int len,
 		}
 
 		/* failed to translate this character */
+		utf -= l;
 		if (noError)
 			break;
 		report_untranslatable_char(PG_UTF8, encoding,
-								   (const char *) (utf - l), len);
+								   (const char *) utf, len);
 	}
 
 	/* if we broke out of loop early, must be invalid input */
@@ -660,7 +658,7 @@ UtfToLocal(const unsigned char *utf, int len,
 
 	*iso = '\0';
 
-	return cur - start;
+	return utf - start;
 }
 
 /*
@@ -701,7 +699,6 @@ LocalToUtf(const unsigned char *iso, int len,
 	int			l;
 	const pg_local_to_utf_combined *cp;
 	const unsigned char *start = iso;
-	const unsigned char *cur = iso;
 
 	if (!PG_VALID_ENCODING(encoding))
 		ereport(ERROR,
@@ -715,8 +712,6 @@ LocalToUtf(const unsigned char *iso, int len,
 		unsigned char b3 = 0;
 		unsigned char b4 = 0;
 
-		cur = iso;
-
 		/* "break" cases all represent errors */
 		if (*iso == '\0')
 			break;
@@ -799,10 +794,11 @@ LocalToUtf(const unsigned char *iso, int len,
 		}
 
 		/* failed to translate this character */
+		iso -= l;
 		if (noError)
 			break;
 		report_untranslatable_char(encoding, PG_UTF8,
-								   (const char *) (iso - l), len);
+								   (const char *) iso, len);
 	}
 
 	/* if we broke out of loop early, must be invalid input */
@@ -811,5 +807,5 @@ LocalToUtf(const unsigned char *iso, int len,
 
 	*utf = '\0';
 
-	return cur - start;
+	return iso - start;
 }
diff --git a/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c b/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
index 8ac93604a1b..d0dc4cca378 100644
--- a/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
+++ b/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
@@ -104,10 +104,18 @@ utf8_to_iso8859_1(PG_FUNCTION_ARGS)
 			int			l = pg_utf_mblen(src);
 
 			if (l > len || !pg_utf8_islegal(src, l))
+			{
+				if (noError)
+					break;
 				report_invalid_encoding(PG_UTF8, (const char *) src, len);
+			}
 			if (l != 2)
+			{
+				if (noError)
+					break;
 				report_untranslatable_char(PG_UTF8, PG_LATIN1,
 										   (const char *) src, len);
+			}
 			c1 = src[1] & 0x3f;
 			c = ((c & 0x1f) << 6) | c1;
 			if (c >= 0x80 && c <= 0xff)
diff --git a/src/backend/utils/mb/mbutils.c b/src/backend/utils/mb/mbutils.c
index 3e106027d75..af1cafe523a 100644
--- a/src/backend/utils/mb/mbutils.c
+++ b/src/backend/utils/mb/mbutils.c
@@ -818,12 +818,13 @@ perform_default_encoding_conversion(const char *src, int len,
 		MemoryContextAllocHuge(CurrentMemoryContext,
 							   (Size) len * MAX_CONVERSION_GROWTH + 1);
 
-	FunctionCall5(flinfo,
+	FunctionCall6(flinfo,
 				  Int32GetDatum(src_encoding),
 				  Int32GetDatum(dest_encoding),
 				  CStringGetDatum(src),
 				  CStringGetDatum(result),
-				  Int32GetDatum(len));
+				  Int32GetDatum(len),
+				  BoolGetDatum(false));
 
 	/*
 	 * Release extra space if there might be a lot --- see comments in
-- 
2.29.2


--------------39E46EF15EBFAA37D53332E5
Content-Type: text/x-patch; charset=UTF-8;
 name="v3-0005-Do-COPY-FROM-encoding-conversion-verification-in-.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename*0="v3-0005-Do-COPY-FROM-encoding-conversion-verification-in-.pa";
 filename*1="tch"



^ permalink  raw  reply  [nested|flat] 46+ messages in thread

* [PATCH v3 4/5] Fix bugs in the commit to change conversion function signature.
@ 2021-02-01 15:30  Heikki Linnakangas <[email protected]>
  0 siblings, 0 replies; 46+ messages in thread

From: Heikki Linnakangas @ 2021-02-01 15:30 UTC (permalink / raw)

Per John Naylor's report, and some bugs I found while testing.

TODO: to be squashed.
---
 src/backend/utils/mb/conv.c                      | 16 ++++++----------
 .../utf8_and_iso8859_1/utf8_and_iso8859_1.c      |  8 ++++++++
 src/backend/utils/mb/mbutils.c                   |  5 +++--
 3 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/src/backend/utils/mb/conv.c b/src/backend/utils/mb/conv.c
index b83358bc7a5..f3bda5753d0 100644
--- a/src/backend/utils/mb/conv.c
+++ b/src/backend/utils/mb/conv.c
@@ -497,7 +497,6 @@ UtfToLocal(const unsigned char *utf, int len,
 	int			l;
 	const pg_utf_to_local_combined *cp;
 	const unsigned char *start = utf;
-	const unsigned char *cur = utf;
 
 	if (!PG_VALID_ENCODING(encoding))
 		ereport(ERROR,
@@ -511,8 +510,6 @@ UtfToLocal(const unsigned char *utf, int len,
 		unsigned char b3 = 0;
 		unsigned char b4 = 0;
 
-		cur = iso;
-
 		/* "break" cases all represent errors */
 		if (*utf == '\0')
 			break;
@@ -648,10 +645,11 @@ UtfToLocal(const unsigned char *utf, int len,
 		}
 
 		/* failed to translate this character */
+		utf -= l;
 		if (noError)
 			break;
 		report_untranslatable_char(PG_UTF8, encoding,
-								   (const char *) (utf - l), len);
+								   (const char *) utf, len);
 	}
 
 	/* if we broke out of loop early, must be invalid input */
@@ -660,7 +658,7 @@ UtfToLocal(const unsigned char *utf, int len,
 
 	*iso = '\0';
 
-	return cur - start;
+	return utf - start;
 }
 
 /*
@@ -701,7 +699,6 @@ LocalToUtf(const unsigned char *iso, int len,
 	int			l;
 	const pg_local_to_utf_combined *cp;
 	const unsigned char *start = iso;
-	const unsigned char *cur = iso;
 
 	if (!PG_VALID_ENCODING(encoding))
 		ereport(ERROR,
@@ -715,8 +712,6 @@ LocalToUtf(const unsigned char *iso, int len,
 		unsigned char b3 = 0;
 		unsigned char b4 = 0;
 
-		cur = iso;
-
 		/* "break" cases all represent errors */
 		if (*iso == '\0')
 			break;
@@ -799,10 +794,11 @@ LocalToUtf(const unsigned char *iso, int len,
 		}
 
 		/* failed to translate this character */
+		iso -= l;
 		if (noError)
 			break;
 		report_untranslatable_char(encoding, PG_UTF8,
-								   (const char *) (iso - l), len);
+								   (const char *) iso, len);
 	}
 
 	/* if we broke out of loop early, must be invalid input */
@@ -811,5 +807,5 @@ LocalToUtf(const unsigned char *iso, int len,
 
 	*utf = '\0';
 
-	return cur - start;
+	return iso - start;
 }
diff --git a/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c b/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
index 8ac93604a1b..d0dc4cca378 100644
--- a/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
+++ b/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
@@ -104,10 +104,18 @@ utf8_to_iso8859_1(PG_FUNCTION_ARGS)
 			int			l = pg_utf_mblen(src);
 
 			if (l > len || !pg_utf8_islegal(src, l))
+			{
+				if (noError)
+					break;
 				report_invalid_encoding(PG_UTF8, (const char *) src, len);
+			}
 			if (l != 2)
+			{
+				if (noError)
+					break;
 				report_untranslatable_char(PG_UTF8, PG_LATIN1,
 										   (const char *) src, len);
+			}
 			c1 = src[1] & 0x3f;
 			c = ((c & 0x1f) << 6) | c1;
 			if (c >= 0x80 && c <= 0xff)
diff --git a/src/backend/utils/mb/mbutils.c b/src/backend/utils/mb/mbutils.c
index 3e106027d75..af1cafe523a 100644
--- a/src/backend/utils/mb/mbutils.c
+++ b/src/backend/utils/mb/mbutils.c
@@ -818,12 +818,13 @@ perform_default_encoding_conversion(const char *src, int len,
 		MemoryContextAllocHuge(CurrentMemoryContext,
 							   (Size) len * MAX_CONVERSION_GROWTH + 1);
 
-	FunctionCall5(flinfo,
+	FunctionCall6(flinfo,
 				  Int32GetDatum(src_encoding),
 				  Int32GetDatum(dest_encoding),
 				  CStringGetDatum(src),
 				  CStringGetDatum(result),
-				  Int32GetDatum(len));
+				  Int32GetDatum(len),
+				  BoolGetDatum(false));
 
 	/*
 	 * Release extra space if there might be a lot --- see comments in
-- 
2.29.2


--------------39E46EF15EBFAA37D53332E5
Content-Type: text/x-patch; charset=UTF-8;
 name="v3-0005-Do-COPY-FROM-encoding-conversion-verification-in-.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename*0="v3-0005-Do-COPY-FROM-encoding-conversion-verification-in-.pa";
 filename*1="tch"



^ permalink  raw  reply  [nested|flat] 46+ messages in thread

* [PATCH v3 4/5] Fix bugs in the commit to change conversion function signature.
@ 2021-02-01 15:30  Heikki Linnakangas <[email protected]>
  0 siblings, 0 replies; 46+ messages in thread

From: Heikki Linnakangas @ 2021-02-01 15:30 UTC (permalink / raw)

Per John Naylor's report, and some bugs I found while testing.

TODO: to be squashed.
---
 src/backend/utils/mb/conv.c                      | 16 ++++++----------
 .../utf8_and_iso8859_1/utf8_and_iso8859_1.c      |  8 ++++++++
 src/backend/utils/mb/mbutils.c                   |  5 +++--
 3 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/src/backend/utils/mb/conv.c b/src/backend/utils/mb/conv.c
index b83358bc7a5..f3bda5753d0 100644
--- a/src/backend/utils/mb/conv.c
+++ b/src/backend/utils/mb/conv.c
@@ -497,7 +497,6 @@ UtfToLocal(const unsigned char *utf, int len,
 	int			l;
 	const pg_utf_to_local_combined *cp;
 	const unsigned char *start = utf;
-	const unsigned char *cur = utf;
 
 	if (!PG_VALID_ENCODING(encoding))
 		ereport(ERROR,
@@ -511,8 +510,6 @@ UtfToLocal(const unsigned char *utf, int len,
 		unsigned char b3 = 0;
 		unsigned char b4 = 0;
 
-		cur = iso;
-
 		/* "break" cases all represent errors */
 		if (*utf == '\0')
 			break;
@@ -648,10 +645,11 @@ UtfToLocal(const unsigned char *utf, int len,
 		}
 
 		/* failed to translate this character */
+		utf -= l;
 		if (noError)
 			break;
 		report_untranslatable_char(PG_UTF8, encoding,
-								   (const char *) (utf - l), len);
+								   (const char *) utf, len);
 	}
 
 	/* if we broke out of loop early, must be invalid input */
@@ -660,7 +658,7 @@ UtfToLocal(const unsigned char *utf, int len,
 
 	*iso = '\0';
 
-	return cur - start;
+	return utf - start;
 }
 
 /*
@@ -701,7 +699,6 @@ LocalToUtf(const unsigned char *iso, int len,
 	int			l;
 	const pg_local_to_utf_combined *cp;
 	const unsigned char *start = iso;
-	const unsigned char *cur = iso;
 
 	if (!PG_VALID_ENCODING(encoding))
 		ereport(ERROR,
@@ -715,8 +712,6 @@ LocalToUtf(const unsigned char *iso, int len,
 		unsigned char b3 = 0;
 		unsigned char b4 = 0;
 
-		cur = iso;
-
 		/* "break" cases all represent errors */
 		if (*iso == '\0')
 			break;
@@ -799,10 +794,11 @@ LocalToUtf(const unsigned char *iso, int len,
 		}
 
 		/* failed to translate this character */
+		iso -= l;
 		if (noError)
 			break;
 		report_untranslatable_char(encoding, PG_UTF8,
-								   (const char *) (iso - l), len);
+								   (const char *) iso, len);
 	}
 
 	/* if we broke out of loop early, must be invalid input */
@@ -811,5 +807,5 @@ LocalToUtf(const unsigned char *iso, int len,
 
 	*utf = '\0';
 
-	return cur - start;
+	return iso - start;
 }
diff --git a/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c b/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
index 8ac93604a1b..d0dc4cca378 100644
--- a/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
+++ b/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
@@ -104,10 +104,18 @@ utf8_to_iso8859_1(PG_FUNCTION_ARGS)
 			int			l = pg_utf_mblen(src);
 
 			if (l > len || !pg_utf8_islegal(src, l))
+			{
+				if (noError)
+					break;
 				report_invalid_encoding(PG_UTF8, (const char *) src, len);
+			}
 			if (l != 2)
+			{
+				if (noError)
+					break;
 				report_untranslatable_char(PG_UTF8, PG_LATIN1,
 										   (const char *) src, len);
+			}
 			c1 = src[1] & 0x3f;
 			c = ((c & 0x1f) << 6) | c1;
 			if (c >= 0x80 && c <= 0xff)
diff --git a/src/backend/utils/mb/mbutils.c b/src/backend/utils/mb/mbutils.c
index 3e106027d75..af1cafe523a 100644
--- a/src/backend/utils/mb/mbutils.c
+++ b/src/backend/utils/mb/mbutils.c
@@ -818,12 +818,13 @@ perform_default_encoding_conversion(const char *src, int len,
 		MemoryContextAllocHuge(CurrentMemoryContext,
 							   (Size) len * MAX_CONVERSION_GROWTH + 1);
 
-	FunctionCall5(flinfo,
+	FunctionCall6(flinfo,
 				  Int32GetDatum(src_encoding),
 				  Int32GetDatum(dest_encoding),
 				  CStringGetDatum(src),
 				  CStringGetDatum(result),
-				  Int32GetDatum(len));
+				  Int32GetDatum(len),
+				  BoolGetDatum(false));
 
 	/*
 	 * Release extra space if there might be a lot --- see comments in
-- 
2.29.2


--------------39E46EF15EBFAA37D53332E5
Content-Type: text/x-patch; charset=UTF-8;
 name="v3-0005-Do-COPY-FROM-encoding-conversion-verification-in-.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename*0="v3-0005-Do-COPY-FROM-encoding-conversion-verification-in-.pa";
 filename*1="tch"



^ permalink  raw  reply  [nested|flat] 46+ messages in thread

* [PATCH v3 4/5] Fix bugs in the commit to change conversion function signature.
@ 2021-02-01 15:30  Heikki Linnakangas <[email protected]>
  0 siblings, 0 replies; 46+ messages in thread

From: Heikki Linnakangas @ 2021-02-01 15:30 UTC (permalink / raw)

Per John Naylor's report, and some bugs I found while testing.

TODO: to be squashed.
---
 src/backend/utils/mb/conv.c                      | 16 ++++++----------
 .../utf8_and_iso8859_1/utf8_and_iso8859_1.c      |  8 ++++++++
 src/backend/utils/mb/mbutils.c                   |  5 +++--
 3 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/src/backend/utils/mb/conv.c b/src/backend/utils/mb/conv.c
index b83358bc7a5..f3bda5753d0 100644
--- a/src/backend/utils/mb/conv.c
+++ b/src/backend/utils/mb/conv.c
@@ -497,7 +497,6 @@ UtfToLocal(const unsigned char *utf, int len,
 	int			l;
 	const pg_utf_to_local_combined *cp;
 	const unsigned char *start = utf;
-	const unsigned char *cur = utf;
 
 	if (!PG_VALID_ENCODING(encoding))
 		ereport(ERROR,
@@ -511,8 +510,6 @@ UtfToLocal(const unsigned char *utf, int len,
 		unsigned char b3 = 0;
 		unsigned char b4 = 0;
 
-		cur = iso;
-
 		/* "break" cases all represent errors */
 		if (*utf == '\0')
 			break;
@@ -648,10 +645,11 @@ UtfToLocal(const unsigned char *utf, int len,
 		}
 
 		/* failed to translate this character */
+		utf -= l;
 		if (noError)
 			break;
 		report_untranslatable_char(PG_UTF8, encoding,
-								   (const char *) (utf - l), len);
+								   (const char *) utf, len);
 	}
 
 	/* if we broke out of loop early, must be invalid input */
@@ -660,7 +658,7 @@ UtfToLocal(const unsigned char *utf, int len,
 
 	*iso = '\0';
 
-	return cur - start;
+	return utf - start;
 }
 
 /*
@@ -701,7 +699,6 @@ LocalToUtf(const unsigned char *iso, int len,
 	int			l;
 	const pg_local_to_utf_combined *cp;
 	const unsigned char *start = iso;
-	const unsigned char *cur = iso;
 
 	if (!PG_VALID_ENCODING(encoding))
 		ereport(ERROR,
@@ -715,8 +712,6 @@ LocalToUtf(const unsigned char *iso, int len,
 		unsigned char b3 = 0;
 		unsigned char b4 = 0;
 
-		cur = iso;
-
 		/* "break" cases all represent errors */
 		if (*iso == '\0')
 			break;
@@ -799,10 +794,11 @@ LocalToUtf(const unsigned char *iso, int len,
 		}
 
 		/* failed to translate this character */
+		iso -= l;
 		if (noError)
 			break;
 		report_untranslatable_char(encoding, PG_UTF8,
-								   (const char *) (iso - l), len);
+								   (const char *) iso, len);
 	}
 
 	/* if we broke out of loop early, must be invalid input */
@@ -811,5 +807,5 @@ LocalToUtf(const unsigned char *iso, int len,
 
 	*utf = '\0';
 
-	return cur - start;
+	return iso - start;
 }
diff --git a/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c b/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
index 8ac93604a1b..d0dc4cca378 100644
--- a/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
+++ b/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
@@ -104,10 +104,18 @@ utf8_to_iso8859_1(PG_FUNCTION_ARGS)
 			int			l = pg_utf_mblen(src);
 
 			if (l > len || !pg_utf8_islegal(src, l))
+			{
+				if (noError)
+					break;
 				report_invalid_encoding(PG_UTF8, (const char *) src, len);
+			}
 			if (l != 2)
+			{
+				if (noError)
+					break;
 				report_untranslatable_char(PG_UTF8, PG_LATIN1,
 										   (const char *) src, len);
+			}
 			c1 = src[1] & 0x3f;
 			c = ((c & 0x1f) << 6) | c1;
 			if (c >= 0x80 && c <= 0xff)
diff --git a/src/backend/utils/mb/mbutils.c b/src/backend/utils/mb/mbutils.c
index 3e106027d75..af1cafe523a 100644
--- a/src/backend/utils/mb/mbutils.c
+++ b/src/backend/utils/mb/mbutils.c
@@ -818,12 +818,13 @@ perform_default_encoding_conversion(const char *src, int len,
 		MemoryContextAllocHuge(CurrentMemoryContext,
 							   (Size) len * MAX_CONVERSION_GROWTH + 1);
 
-	FunctionCall5(flinfo,
+	FunctionCall6(flinfo,
 				  Int32GetDatum(src_encoding),
 				  Int32GetDatum(dest_encoding),
 				  CStringGetDatum(src),
 				  CStringGetDatum(result),
-				  Int32GetDatum(len));
+				  Int32GetDatum(len),
+				  BoolGetDatum(false));
 
 	/*
 	 * Release extra space if there might be a lot --- see comments in
-- 
2.29.2


--------------39E46EF15EBFAA37D53332E5
Content-Type: text/x-patch; charset=UTF-8;
 name="v3-0005-Do-COPY-FROM-encoding-conversion-verification-in-.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename*0="v3-0005-Do-COPY-FROM-encoding-conversion-verification-in-.pa";
 filename*1="tch"



^ permalink  raw  reply  [nested|flat] 46+ messages in thread

* [PATCH v3 4/5] Fix bugs in the commit to change conversion function signature.
@ 2021-02-01 15:30  Heikki Linnakangas <[email protected]>
  0 siblings, 0 replies; 46+ messages in thread

From: Heikki Linnakangas @ 2021-02-01 15:30 UTC (permalink / raw)

Per John Naylor's report, and some bugs I found while testing.

TODO: to be squashed.
---
 src/backend/utils/mb/conv.c                      | 16 ++++++----------
 .../utf8_and_iso8859_1/utf8_and_iso8859_1.c      |  8 ++++++++
 src/backend/utils/mb/mbutils.c                   |  5 +++--
 3 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/src/backend/utils/mb/conv.c b/src/backend/utils/mb/conv.c
index b83358bc7a5..f3bda5753d0 100644
--- a/src/backend/utils/mb/conv.c
+++ b/src/backend/utils/mb/conv.c
@@ -497,7 +497,6 @@ UtfToLocal(const unsigned char *utf, int len,
 	int			l;
 	const pg_utf_to_local_combined *cp;
 	const unsigned char *start = utf;
-	const unsigned char *cur = utf;
 
 	if (!PG_VALID_ENCODING(encoding))
 		ereport(ERROR,
@@ -511,8 +510,6 @@ UtfToLocal(const unsigned char *utf, int len,
 		unsigned char b3 = 0;
 		unsigned char b4 = 0;
 
-		cur = iso;
-
 		/* "break" cases all represent errors */
 		if (*utf == '\0')
 			break;
@@ -648,10 +645,11 @@ UtfToLocal(const unsigned char *utf, int len,
 		}
 
 		/* failed to translate this character */
+		utf -= l;
 		if (noError)
 			break;
 		report_untranslatable_char(PG_UTF8, encoding,
-								   (const char *) (utf - l), len);
+								   (const char *) utf, len);
 	}
 
 	/* if we broke out of loop early, must be invalid input */
@@ -660,7 +658,7 @@ UtfToLocal(const unsigned char *utf, int len,
 
 	*iso = '\0';
 
-	return cur - start;
+	return utf - start;
 }
 
 /*
@@ -701,7 +699,6 @@ LocalToUtf(const unsigned char *iso, int len,
 	int			l;
 	const pg_local_to_utf_combined *cp;
 	const unsigned char *start = iso;
-	const unsigned char *cur = iso;
 
 	if (!PG_VALID_ENCODING(encoding))
 		ereport(ERROR,
@@ -715,8 +712,6 @@ LocalToUtf(const unsigned char *iso, int len,
 		unsigned char b3 = 0;
 		unsigned char b4 = 0;
 
-		cur = iso;
-
 		/* "break" cases all represent errors */
 		if (*iso == '\0')
 			break;
@@ -799,10 +794,11 @@ LocalToUtf(const unsigned char *iso, int len,
 		}
 
 		/* failed to translate this character */
+		iso -= l;
 		if (noError)
 			break;
 		report_untranslatable_char(encoding, PG_UTF8,
-								   (const char *) (iso - l), len);
+								   (const char *) iso, len);
 	}
 
 	/* if we broke out of loop early, must be invalid input */
@@ -811,5 +807,5 @@ LocalToUtf(const unsigned char *iso, int len,
 
 	*utf = '\0';
 
-	return cur - start;
+	return iso - start;
 }
diff --git a/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c b/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
index 8ac93604a1b..d0dc4cca378 100644
--- a/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
+++ b/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
@@ -104,10 +104,18 @@ utf8_to_iso8859_1(PG_FUNCTION_ARGS)
 			int			l = pg_utf_mblen(src);
 
 			if (l > len || !pg_utf8_islegal(src, l))
+			{
+				if (noError)
+					break;
 				report_invalid_encoding(PG_UTF8, (const char *) src, len);
+			}
 			if (l != 2)
+			{
+				if (noError)
+					break;
 				report_untranslatable_char(PG_UTF8, PG_LATIN1,
 										   (const char *) src, len);
+			}
 			c1 = src[1] & 0x3f;
 			c = ((c & 0x1f) << 6) | c1;
 			if (c >= 0x80 && c <= 0xff)
diff --git a/src/backend/utils/mb/mbutils.c b/src/backend/utils/mb/mbutils.c
index 3e106027d75..af1cafe523a 100644
--- a/src/backend/utils/mb/mbutils.c
+++ b/src/backend/utils/mb/mbutils.c
@@ -818,12 +818,13 @@ perform_default_encoding_conversion(const char *src, int len,
 		MemoryContextAllocHuge(CurrentMemoryContext,
 							   (Size) len * MAX_CONVERSION_GROWTH + 1);
 
-	FunctionCall5(flinfo,
+	FunctionCall6(flinfo,
 				  Int32GetDatum(src_encoding),
 				  Int32GetDatum(dest_encoding),
 				  CStringGetDatum(src),
 				  CStringGetDatum(result),
-				  Int32GetDatum(len));
+				  Int32GetDatum(len),
+				  BoolGetDatum(false));
 
 	/*
 	 * Release extra space if there might be a lot --- see comments in
-- 
2.29.2


--------------39E46EF15EBFAA37D53332E5
Content-Type: text/x-patch; charset=UTF-8;
 name="v3-0005-Do-COPY-FROM-encoding-conversion-verification-in-.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename*0="v3-0005-Do-COPY-FROM-encoding-conversion-verification-in-.pa";
 filename*1="tch"



^ permalink  raw  reply  [nested|flat] 46+ messages in thread

* [PATCH v3 4/5] Fix bugs in the commit to change conversion function signature.
@ 2021-02-01 15:30  Heikki Linnakangas <[email protected]>
  0 siblings, 0 replies; 46+ messages in thread

From: Heikki Linnakangas @ 2021-02-01 15:30 UTC (permalink / raw)

Per John Naylor's report, and some bugs I found while testing.

TODO: to be squashed.
---
 src/backend/utils/mb/conv.c                      | 16 ++++++----------
 .../utf8_and_iso8859_1/utf8_and_iso8859_1.c      |  8 ++++++++
 src/backend/utils/mb/mbutils.c                   |  5 +++--
 3 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/src/backend/utils/mb/conv.c b/src/backend/utils/mb/conv.c
index b83358bc7a5..f3bda5753d0 100644
--- a/src/backend/utils/mb/conv.c
+++ b/src/backend/utils/mb/conv.c
@@ -497,7 +497,6 @@ UtfToLocal(const unsigned char *utf, int len,
 	int			l;
 	const pg_utf_to_local_combined *cp;
 	const unsigned char *start = utf;
-	const unsigned char *cur = utf;
 
 	if (!PG_VALID_ENCODING(encoding))
 		ereport(ERROR,
@@ -511,8 +510,6 @@ UtfToLocal(const unsigned char *utf, int len,
 		unsigned char b3 = 0;
 		unsigned char b4 = 0;
 
-		cur = iso;
-
 		/* "break" cases all represent errors */
 		if (*utf == '\0')
 			break;
@@ -648,10 +645,11 @@ UtfToLocal(const unsigned char *utf, int len,
 		}
 
 		/* failed to translate this character */
+		utf -= l;
 		if (noError)
 			break;
 		report_untranslatable_char(PG_UTF8, encoding,
-								   (const char *) (utf - l), len);
+								   (const char *) utf, len);
 	}
 
 	/* if we broke out of loop early, must be invalid input */
@@ -660,7 +658,7 @@ UtfToLocal(const unsigned char *utf, int len,
 
 	*iso = '\0';
 
-	return cur - start;
+	return utf - start;
 }
 
 /*
@@ -701,7 +699,6 @@ LocalToUtf(const unsigned char *iso, int len,
 	int			l;
 	const pg_local_to_utf_combined *cp;
 	const unsigned char *start = iso;
-	const unsigned char *cur = iso;
 
 	if (!PG_VALID_ENCODING(encoding))
 		ereport(ERROR,
@@ -715,8 +712,6 @@ LocalToUtf(const unsigned char *iso, int len,
 		unsigned char b3 = 0;
 		unsigned char b4 = 0;
 
-		cur = iso;
-
 		/* "break" cases all represent errors */
 		if (*iso == '\0')
 			break;
@@ -799,10 +794,11 @@ LocalToUtf(const unsigned char *iso, int len,
 		}
 
 		/* failed to translate this character */
+		iso -= l;
 		if (noError)
 			break;
 		report_untranslatable_char(encoding, PG_UTF8,
-								   (const char *) (iso - l), len);
+								   (const char *) iso, len);
 	}
 
 	/* if we broke out of loop early, must be invalid input */
@@ -811,5 +807,5 @@ LocalToUtf(const unsigned char *iso, int len,
 
 	*utf = '\0';
 
-	return cur - start;
+	return iso - start;
 }
diff --git a/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c b/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
index 8ac93604a1b..d0dc4cca378 100644
--- a/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
+++ b/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
@@ -104,10 +104,18 @@ utf8_to_iso8859_1(PG_FUNCTION_ARGS)
 			int			l = pg_utf_mblen(src);
 
 			if (l > len || !pg_utf8_islegal(src, l))
+			{
+				if (noError)
+					break;
 				report_invalid_encoding(PG_UTF8, (const char *) src, len);
+			}
 			if (l != 2)
+			{
+				if (noError)
+					break;
 				report_untranslatable_char(PG_UTF8, PG_LATIN1,
 										   (const char *) src, len);
+			}
 			c1 = src[1] & 0x3f;
 			c = ((c & 0x1f) << 6) | c1;
 			if (c >= 0x80 && c <= 0xff)
diff --git a/src/backend/utils/mb/mbutils.c b/src/backend/utils/mb/mbutils.c
index 3e106027d75..af1cafe523a 100644
--- a/src/backend/utils/mb/mbutils.c
+++ b/src/backend/utils/mb/mbutils.c
@@ -818,12 +818,13 @@ perform_default_encoding_conversion(const char *src, int len,
 		MemoryContextAllocHuge(CurrentMemoryContext,
 							   (Size) len * MAX_CONVERSION_GROWTH + 1);
 
-	FunctionCall5(flinfo,
+	FunctionCall6(flinfo,
 				  Int32GetDatum(src_encoding),
 				  Int32GetDatum(dest_encoding),
 				  CStringGetDatum(src),
 				  CStringGetDatum(result),
-				  Int32GetDatum(len));
+				  Int32GetDatum(len),
+				  BoolGetDatum(false));
 
 	/*
 	 * Release extra space if there might be a lot --- see comments in
-- 
2.29.2


--------------39E46EF15EBFAA37D53332E5
Content-Type: text/x-patch; charset=UTF-8;
 name="v3-0005-Do-COPY-FROM-encoding-conversion-verification-in-.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename*0="v3-0005-Do-COPY-FROM-encoding-conversion-verification-in-.pa";
 filename*1="tch"



^ permalink  raw  reply  [nested|flat] 46+ messages in thread

* [PATCH v3 4/5] Fix bugs in the commit to change conversion function signature.
@ 2021-02-01 15:30  Heikki Linnakangas <[email protected]>
  0 siblings, 0 replies; 46+ messages in thread

From: Heikki Linnakangas @ 2021-02-01 15:30 UTC (permalink / raw)

Per John Naylor's report, and some bugs I found while testing.

TODO: to be squashed.
---
 src/backend/utils/mb/conv.c                      | 16 ++++++----------
 .../utf8_and_iso8859_1/utf8_and_iso8859_1.c      |  8 ++++++++
 src/backend/utils/mb/mbutils.c                   |  5 +++--
 3 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/src/backend/utils/mb/conv.c b/src/backend/utils/mb/conv.c
index b83358bc7a5..f3bda5753d0 100644
--- a/src/backend/utils/mb/conv.c
+++ b/src/backend/utils/mb/conv.c
@@ -497,7 +497,6 @@ UtfToLocal(const unsigned char *utf, int len,
 	int			l;
 	const pg_utf_to_local_combined *cp;
 	const unsigned char *start = utf;
-	const unsigned char *cur = utf;
 
 	if (!PG_VALID_ENCODING(encoding))
 		ereport(ERROR,
@@ -511,8 +510,6 @@ UtfToLocal(const unsigned char *utf, int len,
 		unsigned char b3 = 0;
 		unsigned char b4 = 0;
 
-		cur = iso;
-
 		/* "break" cases all represent errors */
 		if (*utf == '\0')
 			break;
@@ -648,10 +645,11 @@ UtfToLocal(const unsigned char *utf, int len,
 		}
 
 		/* failed to translate this character */
+		utf -= l;
 		if (noError)
 			break;
 		report_untranslatable_char(PG_UTF8, encoding,
-								   (const char *) (utf - l), len);
+								   (const char *) utf, len);
 	}
 
 	/* if we broke out of loop early, must be invalid input */
@@ -660,7 +658,7 @@ UtfToLocal(const unsigned char *utf, int len,
 
 	*iso = '\0';
 
-	return cur - start;
+	return utf - start;
 }
 
 /*
@@ -701,7 +699,6 @@ LocalToUtf(const unsigned char *iso, int len,
 	int			l;
 	const pg_local_to_utf_combined *cp;
 	const unsigned char *start = iso;
-	const unsigned char *cur = iso;
 
 	if (!PG_VALID_ENCODING(encoding))
 		ereport(ERROR,
@@ -715,8 +712,6 @@ LocalToUtf(const unsigned char *iso, int len,
 		unsigned char b3 = 0;
 		unsigned char b4 = 0;
 
-		cur = iso;
-
 		/* "break" cases all represent errors */
 		if (*iso == '\0')
 			break;
@@ -799,10 +794,11 @@ LocalToUtf(const unsigned char *iso, int len,
 		}
 
 		/* failed to translate this character */
+		iso -= l;
 		if (noError)
 			break;
 		report_untranslatable_char(encoding, PG_UTF8,
-								   (const char *) (iso - l), len);
+								   (const char *) iso, len);
 	}
 
 	/* if we broke out of loop early, must be invalid input */
@@ -811,5 +807,5 @@ LocalToUtf(const unsigned char *iso, int len,
 
 	*utf = '\0';
 
-	return cur - start;
+	return iso - start;
 }
diff --git a/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c b/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
index 8ac93604a1b..d0dc4cca378 100644
--- a/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
+++ b/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
@@ -104,10 +104,18 @@ utf8_to_iso8859_1(PG_FUNCTION_ARGS)
 			int			l = pg_utf_mblen(src);
 
 			if (l > len || !pg_utf8_islegal(src, l))
+			{
+				if (noError)
+					break;
 				report_invalid_encoding(PG_UTF8, (const char *) src, len);
+			}
 			if (l != 2)
+			{
+				if (noError)
+					break;
 				report_untranslatable_char(PG_UTF8, PG_LATIN1,
 										   (const char *) src, len);
+			}
 			c1 = src[1] & 0x3f;
 			c = ((c & 0x1f) << 6) | c1;
 			if (c >= 0x80 && c <= 0xff)
diff --git a/src/backend/utils/mb/mbutils.c b/src/backend/utils/mb/mbutils.c
index 3e106027d75..af1cafe523a 100644
--- a/src/backend/utils/mb/mbutils.c
+++ b/src/backend/utils/mb/mbutils.c
@@ -818,12 +818,13 @@ perform_default_encoding_conversion(const char *src, int len,
 		MemoryContextAllocHuge(CurrentMemoryContext,
 							   (Size) len * MAX_CONVERSION_GROWTH + 1);
 
-	FunctionCall5(flinfo,
+	FunctionCall6(flinfo,
 				  Int32GetDatum(src_encoding),
 				  Int32GetDatum(dest_encoding),
 				  CStringGetDatum(src),
 				  CStringGetDatum(result),
-				  Int32GetDatum(len));
+				  Int32GetDatum(len),
+				  BoolGetDatum(false));
 
 	/*
 	 * Release extra space if there might be a lot --- see comments in
-- 
2.29.2


--------------39E46EF15EBFAA37D53332E5
Content-Type: text/x-patch; charset=UTF-8;
 name="v3-0005-Do-COPY-FROM-encoding-conversion-verification-in-.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename*0="v3-0005-Do-COPY-FROM-encoding-conversion-verification-in-.pa";
 filename*1="tch"



^ permalink  raw  reply  [nested|flat] 46+ messages in thread

* [PATCH v3 4/5] Fix bugs in the commit to change conversion function signature.
@ 2021-02-01 15:30  Heikki Linnakangas <[email protected]>
  0 siblings, 0 replies; 46+ messages in thread

From: Heikki Linnakangas @ 2021-02-01 15:30 UTC (permalink / raw)

Per John Naylor's report, and some bugs I found while testing.

TODO: to be squashed.
---
 src/backend/utils/mb/conv.c                      | 16 ++++++----------
 .../utf8_and_iso8859_1/utf8_and_iso8859_1.c      |  8 ++++++++
 src/backend/utils/mb/mbutils.c                   |  5 +++--
 3 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/src/backend/utils/mb/conv.c b/src/backend/utils/mb/conv.c
index b83358bc7a5..f3bda5753d0 100644
--- a/src/backend/utils/mb/conv.c
+++ b/src/backend/utils/mb/conv.c
@@ -497,7 +497,6 @@ UtfToLocal(const unsigned char *utf, int len,
 	int			l;
 	const pg_utf_to_local_combined *cp;
 	const unsigned char *start = utf;
-	const unsigned char *cur = utf;
 
 	if (!PG_VALID_ENCODING(encoding))
 		ereport(ERROR,
@@ -511,8 +510,6 @@ UtfToLocal(const unsigned char *utf, int len,
 		unsigned char b3 = 0;
 		unsigned char b4 = 0;
 
-		cur = iso;
-
 		/* "break" cases all represent errors */
 		if (*utf == '\0')
 			break;
@@ -648,10 +645,11 @@ UtfToLocal(const unsigned char *utf, int len,
 		}
 
 		/* failed to translate this character */
+		utf -= l;
 		if (noError)
 			break;
 		report_untranslatable_char(PG_UTF8, encoding,
-								   (const char *) (utf - l), len);
+								   (const char *) utf, len);
 	}
 
 	/* if we broke out of loop early, must be invalid input */
@@ -660,7 +658,7 @@ UtfToLocal(const unsigned char *utf, int len,
 
 	*iso = '\0';
 
-	return cur - start;
+	return utf - start;
 }
 
 /*
@@ -701,7 +699,6 @@ LocalToUtf(const unsigned char *iso, int len,
 	int			l;
 	const pg_local_to_utf_combined *cp;
 	const unsigned char *start = iso;
-	const unsigned char *cur = iso;
 
 	if (!PG_VALID_ENCODING(encoding))
 		ereport(ERROR,
@@ -715,8 +712,6 @@ LocalToUtf(const unsigned char *iso, int len,
 		unsigned char b3 = 0;
 		unsigned char b4 = 0;
 
-		cur = iso;
-
 		/* "break" cases all represent errors */
 		if (*iso == '\0')
 			break;
@@ -799,10 +794,11 @@ LocalToUtf(const unsigned char *iso, int len,
 		}
 
 		/* failed to translate this character */
+		iso -= l;
 		if (noError)
 			break;
 		report_untranslatable_char(encoding, PG_UTF8,
-								   (const char *) (iso - l), len);
+								   (const char *) iso, len);
 	}
 
 	/* if we broke out of loop early, must be invalid input */
@@ -811,5 +807,5 @@ LocalToUtf(const unsigned char *iso, int len,
 
 	*utf = '\0';
 
-	return cur - start;
+	return iso - start;
 }
diff --git a/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c b/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
index 8ac93604a1b..d0dc4cca378 100644
--- a/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
+++ b/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
@@ -104,10 +104,18 @@ utf8_to_iso8859_1(PG_FUNCTION_ARGS)
 			int			l = pg_utf_mblen(src);
 
 			if (l > len || !pg_utf8_islegal(src, l))
+			{
+				if (noError)
+					break;
 				report_invalid_encoding(PG_UTF8, (const char *) src, len);
+			}
 			if (l != 2)
+			{
+				if (noError)
+					break;
 				report_untranslatable_char(PG_UTF8, PG_LATIN1,
 										   (const char *) src, len);
+			}
 			c1 = src[1] & 0x3f;
 			c = ((c & 0x1f) << 6) | c1;
 			if (c >= 0x80 && c <= 0xff)
diff --git a/src/backend/utils/mb/mbutils.c b/src/backend/utils/mb/mbutils.c
index 3e106027d75..af1cafe523a 100644
--- a/src/backend/utils/mb/mbutils.c
+++ b/src/backend/utils/mb/mbutils.c
@@ -818,12 +818,13 @@ perform_default_encoding_conversion(const char *src, int len,
 		MemoryContextAllocHuge(CurrentMemoryContext,
 							   (Size) len * MAX_CONVERSION_GROWTH + 1);
 
-	FunctionCall5(flinfo,
+	FunctionCall6(flinfo,
 				  Int32GetDatum(src_encoding),
 				  Int32GetDatum(dest_encoding),
 				  CStringGetDatum(src),
 				  CStringGetDatum(result),
-				  Int32GetDatum(len));
+				  Int32GetDatum(len),
+				  BoolGetDatum(false));
 
 	/*
 	 * Release extra space if there might be a lot --- see comments in
-- 
2.29.2


--------------39E46EF15EBFAA37D53332E5
Content-Type: text/x-patch; charset=UTF-8;
 name="v3-0005-Do-COPY-FROM-encoding-conversion-verification-in-.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename*0="v3-0005-Do-COPY-FROM-encoding-conversion-verification-in-.pa";
 filename*1="tch"



^ permalink  raw  reply  [nested|flat] 46+ messages in thread

* [PATCH v3 4/5] Fix bugs in the commit to change conversion function signature.
@ 2021-02-01 15:30  Heikki Linnakangas <[email protected]>
  0 siblings, 0 replies; 46+ messages in thread

From: Heikki Linnakangas @ 2021-02-01 15:30 UTC (permalink / raw)

Per John Naylor's report, and some bugs I found while testing.

TODO: to be squashed.
---
 src/backend/utils/mb/conv.c                      | 16 ++++++----------
 .../utf8_and_iso8859_1/utf8_and_iso8859_1.c      |  8 ++++++++
 src/backend/utils/mb/mbutils.c                   |  5 +++--
 3 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/src/backend/utils/mb/conv.c b/src/backend/utils/mb/conv.c
index b83358bc7a5..f3bda5753d0 100644
--- a/src/backend/utils/mb/conv.c
+++ b/src/backend/utils/mb/conv.c
@@ -497,7 +497,6 @@ UtfToLocal(const unsigned char *utf, int len,
 	int			l;
 	const pg_utf_to_local_combined *cp;
 	const unsigned char *start = utf;
-	const unsigned char *cur = utf;
 
 	if (!PG_VALID_ENCODING(encoding))
 		ereport(ERROR,
@@ -511,8 +510,6 @@ UtfToLocal(const unsigned char *utf, int len,
 		unsigned char b3 = 0;
 		unsigned char b4 = 0;
 
-		cur = iso;
-
 		/* "break" cases all represent errors */
 		if (*utf == '\0')
 			break;
@@ -648,10 +645,11 @@ UtfToLocal(const unsigned char *utf, int len,
 		}
 
 		/* failed to translate this character */
+		utf -= l;
 		if (noError)
 			break;
 		report_untranslatable_char(PG_UTF8, encoding,
-								   (const char *) (utf - l), len);
+								   (const char *) utf, len);
 	}
 
 	/* if we broke out of loop early, must be invalid input */
@@ -660,7 +658,7 @@ UtfToLocal(const unsigned char *utf, int len,
 
 	*iso = '\0';
 
-	return cur - start;
+	return utf - start;
 }
 
 /*
@@ -701,7 +699,6 @@ LocalToUtf(const unsigned char *iso, int len,
 	int			l;
 	const pg_local_to_utf_combined *cp;
 	const unsigned char *start = iso;
-	const unsigned char *cur = iso;
 
 	if (!PG_VALID_ENCODING(encoding))
 		ereport(ERROR,
@@ -715,8 +712,6 @@ LocalToUtf(const unsigned char *iso, int len,
 		unsigned char b3 = 0;
 		unsigned char b4 = 0;
 
-		cur = iso;
-
 		/* "break" cases all represent errors */
 		if (*iso == '\0')
 			break;
@@ -799,10 +794,11 @@ LocalToUtf(const unsigned char *iso, int len,
 		}
 
 		/* failed to translate this character */
+		iso -= l;
 		if (noError)
 			break;
 		report_untranslatable_char(encoding, PG_UTF8,
-								   (const char *) (iso - l), len);
+								   (const char *) iso, len);
 	}
 
 	/* if we broke out of loop early, must be invalid input */
@@ -811,5 +807,5 @@ LocalToUtf(const unsigned char *iso, int len,
 
 	*utf = '\0';
 
-	return cur - start;
+	return iso - start;
 }
diff --git a/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c b/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
index 8ac93604a1b..d0dc4cca378 100644
--- a/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
+++ b/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
@@ -104,10 +104,18 @@ utf8_to_iso8859_1(PG_FUNCTION_ARGS)
 			int			l = pg_utf_mblen(src);
 
 			if (l > len || !pg_utf8_islegal(src, l))
+			{
+				if (noError)
+					break;
 				report_invalid_encoding(PG_UTF8, (const char *) src, len);
+			}
 			if (l != 2)
+			{
+				if (noError)
+					break;
 				report_untranslatable_char(PG_UTF8, PG_LATIN1,
 										   (const char *) src, len);
+			}
 			c1 = src[1] & 0x3f;
 			c = ((c & 0x1f) << 6) | c1;
 			if (c >= 0x80 && c <= 0xff)
diff --git a/src/backend/utils/mb/mbutils.c b/src/backend/utils/mb/mbutils.c
index 3e106027d75..af1cafe523a 100644
--- a/src/backend/utils/mb/mbutils.c
+++ b/src/backend/utils/mb/mbutils.c
@@ -818,12 +818,13 @@ perform_default_encoding_conversion(const char *src, int len,
 		MemoryContextAllocHuge(CurrentMemoryContext,
 							   (Size) len * MAX_CONVERSION_GROWTH + 1);
 
-	FunctionCall5(flinfo,
+	FunctionCall6(flinfo,
 				  Int32GetDatum(src_encoding),
 				  Int32GetDatum(dest_encoding),
 				  CStringGetDatum(src),
 				  CStringGetDatum(result),
-				  Int32GetDatum(len));
+				  Int32GetDatum(len),
+				  BoolGetDatum(false));
 
 	/*
 	 * Release extra space if there might be a lot --- see comments in
-- 
2.29.2


--------------39E46EF15EBFAA37D53332E5
Content-Type: text/x-patch; charset=UTF-8;
 name="v3-0005-Do-COPY-FROM-encoding-conversion-verification-in-.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename*0="v3-0005-Do-COPY-FROM-encoding-conversion-verification-in-.pa";
 filename*1="tch"



^ permalink  raw  reply  [nested|flat] 46+ messages in thread

* [PATCH v3 4/5] Fix bugs in the commit to change conversion function signature.
@ 2021-02-01 15:30  Heikki Linnakangas <[email protected]>
  0 siblings, 0 replies; 46+ messages in thread

From: Heikki Linnakangas @ 2021-02-01 15:30 UTC (permalink / raw)

Per John Naylor's report, and some bugs I found while testing.

TODO: to be squashed.
---
 src/backend/utils/mb/conv.c                      | 16 ++++++----------
 .../utf8_and_iso8859_1/utf8_and_iso8859_1.c      |  8 ++++++++
 src/backend/utils/mb/mbutils.c                   |  5 +++--
 3 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/src/backend/utils/mb/conv.c b/src/backend/utils/mb/conv.c
index b83358bc7a5..f3bda5753d0 100644
--- a/src/backend/utils/mb/conv.c
+++ b/src/backend/utils/mb/conv.c
@@ -497,7 +497,6 @@ UtfToLocal(const unsigned char *utf, int len,
 	int			l;
 	const pg_utf_to_local_combined *cp;
 	const unsigned char *start = utf;
-	const unsigned char *cur = utf;
 
 	if (!PG_VALID_ENCODING(encoding))
 		ereport(ERROR,
@@ -511,8 +510,6 @@ UtfToLocal(const unsigned char *utf, int len,
 		unsigned char b3 = 0;
 		unsigned char b4 = 0;
 
-		cur = iso;
-
 		/* "break" cases all represent errors */
 		if (*utf == '\0')
 			break;
@@ -648,10 +645,11 @@ UtfToLocal(const unsigned char *utf, int len,
 		}
 
 		/* failed to translate this character */
+		utf -= l;
 		if (noError)
 			break;
 		report_untranslatable_char(PG_UTF8, encoding,
-								   (const char *) (utf - l), len);
+								   (const char *) utf, len);
 	}
 
 	/* if we broke out of loop early, must be invalid input */
@@ -660,7 +658,7 @@ UtfToLocal(const unsigned char *utf, int len,
 
 	*iso = '\0';
 
-	return cur - start;
+	return utf - start;
 }
 
 /*
@@ -701,7 +699,6 @@ LocalToUtf(const unsigned char *iso, int len,
 	int			l;
 	const pg_local_to_utf_combined *cp;
 	const unsigned char *start = iso;
-	const unsigned char *cur = iso;
 
 	if (!PG_VALID_ENCODING(encoding))
 		ereport(ERROR,
@@ -715,8 +712,6 @@ LocalToUtf(const unsigned char *iso, int len,
 		unsigned char b3 = 0;
 		unsigned char b4 = 0;
 
-		cur = iso;
-
 		/* "break" cases all represent errors */
 		if (*iso == '\0')
 			break;
@@ -799,10 +794,11 @@ LocalToUtf(const unsigned char *iso, int len,
 		}
 
 		/* failed to translate this character */
+		iso -= l;
 		if (noError)
 			break;
 		report_untranslatable_char(encoding, PG_UTF8,
-								   (const char *) (iso - l), len);
+								   (const char *) iso, len);
 	}
 
 	/* if we broke out of loop early, must be invalid input */
@@ -811,5 +807,5 @@ LocalToUtf(const unsigned char *iso, int len,
 
 	*utf = '\0';
 
-	return cur - start;
+	return iso - start;
 }
diff --git a/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c b/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
index 8ac93604a1b..d0dc4cca378 100644
--- a/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
+++ b/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
@@ -104,10 +104,18 @@ utf8_to_iso8859_1(PG_FUNCTION_ARGS)
 			int			l = pg_utf_mblen(src);
 
 			if (l > len || !pg_utf8_islegal(src, l))
+			{
+				if (noError)
+					break;
 				report_invalid_encoding(PG_UTF8, (const char *) src, len);
+			}
 			if (l != 2)
+			{
+				if (noError)
+					break;
 				report_untranslatable_char(PG_UTF8, PG_LATIN1,
 										   (const char *) src, len);
+			}
 			c1 = src[1] & 0x3f;
 			c = ((c & 0x1f) << 6) | c1;
 			if (c >= 0x80 && c <= 0xff)
diff --git a/src/backend/utils/mb/mbutils.c b/src/backend/utils/mb/mbutils.c
index 3e106027d75..af1cafe523a 100644
--- a/src/backend/utils/mb/mbutils.c
+++ b/src/backend/utils/mb/mbutils.c
@@ -818,12 +818,13 @@ perform_default_encoding_conversion(const char *src, int len,
 		MemoryContextAllocHuge(CurrentMemoryContext,
 							   (Size) len * MAX_CONVERSION_GROWTH + 1);
 
-	FunctionCall5(flinfo,
+	FunctionCall6(flinfo,
 				  Int32GetDatum(src_encoding),
 				  Int32GetDatum(dest_encoding),
 				  CStringGetDatum(src),
 				  CStringGetDatum(result),
-				  Int32GetDatum(len));
+				  Int32GetDatum(len),
+				  BoolGetDatum(false));
 
 	/*
 	 * Release extra space if there might be a lot --- see comments in
-- 
2.29.2


--------------39E46EF15EBFAA37D53332E5
Content-Type: text/x-patch; charset=UTF-8;
 name="v3-0005-Do-COPY-FROM-encoding-conversion-verification-in-.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename*0="v3-0005-Do-COPY-FROM-encoding-conversion-verification-in-.pa";
 filename*1="tch"



^ permalink  raw  reply  [nested|flat] 46+ messages in thread

* [PATCH v3 4/5] Fix bugs in the commit to change conversion function signature.
@ 2021-02-01 15:30  Heikki Linnakangas <[email protected]>
  0 siblings, 0 replies; 46+ messages in thread

From: Heikki Linnakangas @ 2021-02-01 15:30 UTC (permalink / raw)

Per John Naylor's report, and some bugs I found while testing.

TODO: to be squashed.
---
 src/backend/utils/mb/conv.c                      | 16 ++++++----------
 .../utf8_and_iso8859_1/utf8_and_iso8859_1.c      |  8 ++++++++
 src/backend/utils/mb/mbutils.c                   |  5 +++--
 3 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/src/backend/utils/mb/conv.c b/src/backend/utils/mb/conv.c
index b83358bc7a5..f3bda5753d0 100644
--- a/src/backend/utils/mb/conv.c
+++ b/src/backend/utils/mb/conv.c
@@ -497,7 +497,6 @@ UtfToLocal(const unsigned char *utf, int len,
 	int			l;
 	const pg_utf_to_local_combined *cp;
 	const unsigned char *start = utf;
-	const unsigned char *cur = utf;
 
 	if (!PG_VALID_ENCODING(encoding))
 		ereport(ERROR,
@@ -511,8 +510,6 @@ UtfToLocal(const unsigned char *utf, int len,
 		unsigned char b3 = 0;
 		unsigned char b4 = 0;
 
-		cur = iso;
-
 		/* "break" cases all represent errors */
 		if (*utf == '\0')
 			break;
@@ -648,10 +645,11 @@ UtfToLocal(const unsigned char *utf, int len,
 		}
 
 		/* failed to translate this character */
+		utf -= l;
 		if (noError)
 			break;
 		report_untranslatable_char(PG_UTF8, encoding,
-								   (const char *) (utf - l), len);
+								   (const char *) utf, len);
 	}
 
 	/* if we broke out of loop early, must be invalid input */
@@ -660,7 +658,7 @@ UtfToLocal(const unsigned char *utf, int len,
 
 	*iso = '\0';
 
-	return cur - start;
+	return utf - start;
 }
 
 /*
@@ -701,7 +699,6 @@ LocalToUtf(const unsigned char *iso, int len,
 	int			l;
 	const pg_local_to_utf_combined *cp;
 	const unsigned char *start = iso;
-	const unsigned char *cur = iso;
 
 	if (!PG_VALID_ENCODING(encoding))
 		ereport(ERROR,
@@ -715,8 +712,6 @@ LocalToUtf(const unsigned char *iso, int len,
 		unsigned char b3 = 0;
 		unsigned char b4 = 0;
 
-		cur = iso;
-
 		/* "break" cases all represent errors */
 		if (*iso == '\0')
 			break;
@@ -799,10 +794,11 @@ LocalToUtf(const unsigned char *iso, int len,
 		}
 
 		/* failed to translate this character */
+		iso -= l;
 		if (noError)
 			break;
 		report_untranslatable_char(encoding, PG_UTF8,
-								   (const char *) (iso - l), len);
+								   (const char *) iso, len);
 	}
 
 	/* if we broke out of loop early, must be invalid input */
@@ -811,5 +807,5 @@ LocalToUtf(const unsigned char *iso, int len,
 
 	*utf = '\0';
 
-	return cur - start;
+	return iso - start;
 }
diff --git a/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c b/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
index 8ac93604a1b..d0dc4cca378 100644
--- a/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
+++ b/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
@@ -104,10 +104,18 @@ utf8_to_iso8859_1(PG_FUNCTION_ARGS)
 			int			l = pg_utf_mblen(src);
 
 			if (l > len || !pg_utf8_islegal(src, l))
+			{
+				if (noError)
+					break;
 				report_invalid_encoding(PG_UTF8, (const char *) src, len);
+			}
 			if (l != 2)
+			{
+				if (noError)
+					break;
 				report_untranslatable_char(PG_UTF8, PG_LATIN1,
 										   (const char *) src, len);
+			}
 			c1 = src[1] & 0x3f;
 			c = ((c & 0x1f) << 6) | c1;
 			if (c >= 0x80 && c <= 0xff)
diff --git a/src/backend/utils/mb/mbutils.c b/src/backend/utils/mb/mbutils.c
index 3e106027d75..af1cafe523a 100644
--- a/src/backend/utils/mb/mbutils.c
+++ b/src/backend/utils/mb/mbutils.c
@@ -818,12 +818,13 @@ perform_default_encoding_conversion(const char *src, int len,
 		MemoryContextAllocHuge(CurrentMemoryContext,
 							   (Size) len * MAX_CONVERSION_GROWTH + 1);
 
-	FunctionCall5(flinfo,
+	FunctionCall6(flinfo,
 				  Int32GetDatum(src_encoding),
 				  Int32GetDatum(dest_encoding),
 				  CStringGetDatum(src),
 				  CStringGetDatum(result),
-				  Int32GetDatum(len));
+				  Int32GetDatum(len),
+				  BoolGetDatum(false));
 
 	/*
 	 * Release extra space if there might be a lot --- see comments in
-- 
2.29.2


--------------39E46EF15EBFAA37D53332E5
Content-Type: text/x-patch; charset=UTF-8;
 name="v3-0005-Do-COPY-FROM-encoding-conversion-verification-in-.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename*0="v3-0005-Do-COPY-FROM-encoding-conversion-verification-in-.pa";
 filename*1="tch"



^ permalink  raw  reply  [nested|flat] 46+ messages in thread

* [PATCH v3 4/5] Fix bugs in the commit to change conversion function signature.
@ 2021-02-01 15:30  Heikki Linnakangas <[email protected]>
  0 siblings, 0 replies; 46+ messages in thread

From: Heikki Linnakangas @ 2021-02-01 15:30 UTC (permalink / raw)

Per John Naylor's report, and some bugs I found while testing.

TODO: to be squashed.
---
 src/backend/utils/mb/conv.c                      | 16 ++++++----------
 .../utf8_and_iso8859_1/utf8_and_iso8859_1.c      |  8 ++++++++
 src/backend/utils/mb/mbutils.c                   |  5 +++--
 3 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/src/backend/utils/mb/conv.c b/src/backend/utils/mb/conv.c
index b83358bc7a5..f3bda5753d0 100644
--- a/src/backend/utils/mb/conv.c
+++ b/src/backend/utils/mb/conv.c
@@ -497,7 +497,6 @@ UtfToLocal(const unsigned char *utf, int len,
 	int			l;
 	const pg_utf_to_local_combined *cp;
 	const unsigned char *start = utf;
-	const unsigned char *cur = utf;
 
 	if (!PG_VALID_ENCODING(encoding))
 		ereport(ERROR,
@@ -511,8 +510,6 @@ UtfToLocal(const unsigned char *utf, int len,
 		unsigned char b3 = 0;
 		unsigned char b4 = 0;
 
-		cur = iso;
-
 		/* "break" cases all represent errors */
 		if (*utf == '\0')
 			break;
@@ -648,10 +645,11 @@ UtfToLocal(const unsigned char *utf, int len,
 		}
 
 		/* failed to translate this character */
+		utf -= l;
 		if (noError)
 			break;
 		report_untranslatable_char(PG_UTF8, encoding,
-								   (const char *) (utf - l), len);
+								   (const char *) utf, len);
 	}
 
 	/* if we broke out of loop early, must be invalid input */
@@ -660,7 +658,7 @@ UtfToLocal(const unsigned char *utf, int len,
 
 	*iso = '\0';
 
-	return cur - start;
+	return utf - start;
 }
 
 /*
@@ -701,7 +699,6 @@ LocalToUtf(const unsigned char *iso, int len,
 	int			l;
 	const pg_local_to_utf_combined *cp;
 	const unsigned char *start = iso;
-	const unsigned char *cur = iso;
 
 	if (!PG_VALID_ENCODING(encoding))
 		ereport(ERROR,
@@ -715,8 +712,6 @@ LocalToUtf(const unsigned char *iso, int len,
 		unsigned char b3 = 0;
 		unsigned char b4 = 0;
 
-		cur = iso;
-
 		/* "break" cases all represent errors */
 		if (*iso == '\0')
 			break;
@@ -799,10 +794,11 @@ LocalToUtf(const unsigned char *iso, int len,
 		}
 
 		/* failed to translate this character */
+		iso -= l;
 		if (noError)
 			break;
 		report_untranslatable_char(encoding, PG_UTF8,
-								   (const char *) (iso - l), len);
+								   (const char *) iso, len);
 	}
 
 	/* if we broke out of loop early, must be invalid input */
@@ -811,5 +807,5 @@ LocalToUtf(const unsigned char *iso, int len,
 
 	*utf = '\0';
 
-	return cur - start;
+	return iso - start;
 }
diff --git a/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c b/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
index 8ac93604a1b..d0dc4cca378 100644
--- a/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
+++ b/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
@@ -104,10 +104,18 @@ utf8_to_iso8859_1(PG_FUNCTION_ARGS)
 			int			l = pg_utf_mblen(src);
 
 			if (l > len || !pg_utf8_islegal(src, l))
+			{
+				if (noError)
+					break;
 				report_invalid_encoding(PG_UTF8, (const char *) src, len);
+			}
 			if (l != 2)
+			{
+				if (noError)
+					break;
 				report_untranslatable_char(PG_UTF8, PG_LATIN1,
 										   (const char *) src, len);
+			}
 			c1 = src[1] & 0x3f;
 			c = ((c & 0x1f) << 6) | c1;
 			if (c >= 0x80 && c <= 0xff)
diff --git a/src/backend/utils/mb/mbutils.c b/src/backend/utils/mb/mbutils.c
index 3e106027d75..af1cafe523a 100644
--- a/src/backend/utils/mb/mbutils.c
+++ b/src/backend/utils/mb/mbutils.c
@@ -818,12 +818,13 @@ perform_default_encoding_conversion(const char *src, int len,
 		MemoryContextAllocHuge(CurrentMemoryContext,
 							   (Size) len * MAX_CONVERSION_GROWTH + 1);
 
-	FunctionCall5(flinfo,
+	FunctionCall6(flinfo,
 				  Int32GetDatum(src_encoding),
 				  Int32GetDatum(dest_encoding),
 				  CStringGetDatum(src),
 				  CStringGetDatum(result),
-				  Int32GetDatum(len));
+				  Int32GetDatum(len),
+				  BoolGetDatum(false));
 
 	/*
 	 * Release extra space if there might be a lot --- see comments in
-- 
2.29.2


--------------39E46EF15EBFAA37D53332E5
Content-Type: text/x-patch; charset=UTF-8;
 name="v3-0005-Do-COPY-FROM-encoding-conversion-verification-in-.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename*0="v3-0005-Do-COPY-FROM-encoding-conversion-verification-in-.pa";
 filename*1="tch"



^ permalink  raw  reply  [nested|flat] 46+ messages in thread

* [PATCH v3 4/5] Fix bugs in the commit to change conversion function signature.
@ 2021-02-01 15:30  Heikki Linnakangas <[email protected]>
  0 siblings, 0 replies; 46+ messages in thread

From: Heikki Linnakangas @ 2021-02-01 15:30 UTC (permalink / raw)

Per John Naylor's report, and some bugs I found while testing.

TODO: to be squashed.
---
 src/backend/utils/mb/conv.c                      | 16 ++++++----------
 .../utf8_and_iso8859_1/utf8_and_iso8859_1.c      |  8 ++++++++
 src/backend/utils/mb/mbutils.c                   |  5 +++--
 3 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/src/backend/utils/mb/conv.c b/src/backend/utils/mb/conv.c
index b83358bc7a5..f3bda5753d0 100644
--- a/src/backend/utils/mb/conv.c
+++ b/src/backend/utils/mb/conv.c
@@ -497,7 +497,6 @@ UtfToLocal(const unsigned char *utf, int len,
 	int			l;
 	const pg_utf_to_local_combined *cp;
 	const unsigned char *start = utf;
-	const unsigned char *cur = utf;
 
 	if (!PG_VALID_ENCODING(encoding))
 		ereport(ERROR,
@@ -511,8 +510,6 @@ UtfToLocal(const unsigned char *utf, int len,
 		unsigned char b3 = 0;
 		unsigned char b4 = 0;
 
-		cur = iso;
-
 		/* "break" cases all represent errors */
 		if (*utf == '\0')
 			break;
@@ -648,10 +645,11 @@ UtfToLocal(const unsigned char *utf, int len,
 		}
 
 		/* failed to translate this character */
+		utf -= l;
 		if (noError)
 			break;
 		report_untranslatable_char(PG_UTF8, encoding,
-								   (const char *) (utf - l), len);
+								   (const char *) utf, len);
 	}
 
 	/* if we broke out of loop early, must be invalid input */
@@ -660,7 +658,7 @@ UtfToLocal(const unsigned char *utf, int len,
 
 	*iso = '\0';
 
-	return cur - start;
+	return utf - start;
 }
 
 /*
@@ -701,7 +699,6 @@ LocalToUtf(const unsigned char *iso, int len,
 	int			l;
 	const pg_local_to_utf_combined *cp;
 	const unsigned char *start = iso;
-	const unsigned char *cur = iso;
 
 	if (!PG_VALID_ENCODING(encoding))
 		ereport(ERROR,
@@ -715,8 +712,6 @@ LocalToUtf(const unsigned char *iso, int len,
 		unsigned char b3 = 0;
 		unsigned char b4 = 0;
 
-		cur = iso;
-
 		/* "break" cases all represent errors */
 		if (*iso == '\0')
 			break;
@@ -799,10 +794,11 @@ LocalToUtf(const unsigned char *iso, int len,
 		}
 
 		/* failed to translate this character */
+		iso -= l;
 		if (noError)
 			break;
 		report_untranslatable_char(encoding, PG_UTF8,
-								   (const char *) (iso - l), len);
+								   (const char *) iso, len);
 	}
 
 	/* if we broke out of loop early, must be invalid input */
@@ -811,5 +807,5 @@ LocalToUtf(const unsigned char *iso, int len,
 
 	*utf = '\0';
 
-	return cur - start;
+	return iso - start;
 }
diff --git a/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c b/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
index 8ac93604a1b..d0dc4cca378 100644
--- a/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
+++ b/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
@@ -104,10 +104,18 @@ utf8_to_iso8859_1(PG_FUNCTION_ARGS)
 			int			l = pg_utf_mblen(src);
 
 			if (l > len || !pg_utf8_islegal(src, l))
+			{
+				if (noError)
+					break;
 				report_invalid_encoding(PG_UTF8, (const char *) src, len);
+			}
 			if (l != 2)
+			{
+				if (noError)
+					break;
 				report_untranslatable_char(PG_UTF8, PG_LATIN1,
 										   (const char *) src, len);
+			}
 			c1 = src[1] & 0x3f;
 			c = ((c & 0x1f) << 6) | c1;
 			if (c >= 0x80 && c <= 0xff)
diff --git a/src/backend/utils/mb/mbutils.c b/src/backend/utils/mb/mbutils.c
index 3e106027d75..af1cafe523a 100644
--- a/src/backend/utils/mb/mbutils.c
+++ b/src/backend/utils/mb/mbutils.c
@@ -818,12 +818,13 @@ perform_default_encoding_conversion(const char *src, int len,
 		MemoryContextAllocHuge(CurrentMemoryContext,
 							   (Size) len * MAX_CONVERSION_GROWTH + 1);
 
-	FunctionCall5(flinfo,
+	FunctionCall6(flinfo,
 				  Int32GetDatum(src_encoding),
 				  Int32GetDatum(dest_encoding),
 				  CStringGetDatum(src),
 				  CStringGetDatum(result),
-				  Int32GetDatum(len));
+				  Int32GetDatum(len),
+				  BoolGetDatum(false));
 
 	/*
 	 * Release extra space if there might be a lot --- see comments in
-- 
2.29.2


--------------39E46EF15EBFAA37D53332E5
Content-Type: text/x-patch; charset=UTF-8;
 name="v3-0005-Do-COPY-FROM-encoding-conversion-verification-in-.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename*0="v3-0005-Do-COPY-FROM-encoding-conversion-verification-in-.pa";
 filename*1="tch"



^ permalink  raw  reply  [nested|flat] 46+ messages in thread

* [PATCH v3 4/5] Fix bugs in the commit to change conversion function signature.
@ 2021-02-01 15:30  Heikki Linnakangas <[email protected]>
  0 siblings, 0 replies; 46+ messages in thread

From: Heikki Linnakangas @ 2021-02-01 15:30 UTC (permalink / raw)

Per John Naylor's report, and some bugs I found while testing.

TODO: to be squashed.
---
 src/backend/utils/mb/conv.c                      | 16 ++++++----------
 .../utf8_and_iso8859_1/utf8_and_iso8859_1.c      |  8 ++++++++
 src/backend/utils/mb/mbutils.c                   |  5 +++--
 3 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/src/backend/utils/mb/conv.c b/src/backend/utils/mb/conv.c
index b83358bc7a5..f3bda5753d0 100644
--- a/src/backend/utils/mb/conv.c
+++ b/src/backend/utils/mb/conv.c
@@ -497,7 +497,6 @@ UtfToLocal(const unsigned char *utf, int len,
 	int			l;
 	const pg_utf_to_local_combined *cp;
 	const unsigned char *start = utf;
-	const unsigned char *cur = utf;
 
 	if (!PG_VALID_ENCODING(encoding))
 		ereport(ERROR,
@@ -511,8 +510,6 @@ UtfToLocal(const unsigned char *utf, int len,
 		unsigned char b3 = 0;
 		unsigned char b4 = 0;
 
-		cur = iso;
-
 		/* "break" cases all represent errors */
 		if (*utf == '\0')
 			break;
@@ -648,10 +645,11 @@ UtfToLocal(const unsigned char *utf, int len,
 		}
 
 		/* failed to translate this character */
+		utf -= l;
 		if (noError)
 			break;
 		report_untranslatable_char(PG_UTF8, encoding,
-								   (const char *) (utf - l), len);
+								   (const char *) utf, len);
 	}
 
 	/* if we broke out of loop early, must be invalid input */
@@ -660,7 +658,7 @@ UtfToLocal(const unsigned char *utf, int len,
 
 	*iso = '\0';
 
-	return cur - start;
+	return utf - start;
 }
 
 /*
@@ -701,7 +699,6 @@ LocalToUtf(const unsigned char *iso, int len,
 	int			l;
 	const pg_local_to_utf_combined *cp;
 	const unsigned char *start = iso;
-	const unsigned char *cur = iso;
 
 	if (!PG_VALID_ENCODING(encoding))
 		ereport(ERROR,
@@ -715,8 +712,6 @@ LocalToUtf(const unsigned char *iso, int len,
 		unsigned char b3 = 0;
 		unsigned char b4 = 0;
 
-		cur = iso;
-
 		/* "break" cases all represent errors */
 		if (*iso == '\0')
 			break;
@@ -799,10 +794,11 @@ LocalToUtf(const unsigned char *iso, int len,
 		}
 
 		/* failed to translate this character */
+		iso -= l;
 		if (noError)
 			break;
 		report_untranslatable_char(encoding, PG_UTF8,
-								   (const char *) (iso - l), len);
+								   (const char *) iso, len);
 	}
 
 	/* if we broke out of loop early, must be invalid input */
@@ -811,5 +807,5 @@ LocalToUtf(const unsigned char *iso, int len,
 
 	*utf = '\0';
 
-	return cur - start;
+	return iso - start;
 }
diff --git a/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c b/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
index 8ac93604a1b..d0dc4cca378 100644
--- a/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
+++ b/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
@@ -104,10 +104,18 @@ utf8_to_iso8859_1(PG_FUNCTION_ARGS)
 			int			l = pg_utf_mblen(src);
 
 			if (l > len || !pg_utf8_islegal(src, l))
+			{
+				if (noError)
+					break;
 				report_invalid_encoding(PG_UTF8, (const char *) src, len);
+			}
 			if (l != 2)
+			{
+				if (noError)
+					break;
 				report_untranslatable_char(PG_UTF8, PG_LATIN1,
 										   (const char *) src, len);
+			}
 			c1 = src[1] & 0x3f;
 			c = ((c & 0x1f) << 6) | c1;
 			if (c >= 0x80 && c <= 0xff)
diff --git a/src/backend/utils/mb/mbutils.c b/src/backend/utils/mb/mbutils.c
index 3e106027d75..af1cafe523a 100644
--- a/src/backend/utils/mb/mbutils.c
+++ b/src/backend/utils/mb/mbutils.c
@@ -818,12 +818,13 @@ perform_default_encoding_conversion(const char *src, int len,
 		MemoryContextAllocHuge(CurrentMemoryContext,
 							   (Size) len * MAX_CONVERSION_GROWTH + 1);
 
-	FunctionCall5(flinfo,
+	FunctionCall6(flinfo,
 				  Int32GetDatum(src_encoding),
 				  Int32GetDatum(dest_encoding),
 				  CStringGetDatum(src),
 				  CStringGetDatum(result),
-				  Int32GetDatum(len));
+				  Int32GetDatum(len),
+				  BoolGetDatum(false));
 
 	/*
 	 * Release extra space if there might be a lot --- see comments in
-- 
2.29.2


--------------39E46EF15EBFAA37D53332E5
Content-Type: text/x-patch; charset=UTF-8;
 name="v3-0005-Do-COPY-FROM-encoding-conversion-verification-in-.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename*0="v3-0005-Do-COPY-FROM-encoding-conversion-verification-in-.pa";
 filename*1="tch"



^ permalink  raw  reply  [nested|flat] 46+ messages in thread

* [PATCH v3 4/5] Fix bugs in the commit to change conversion function signature.
@ 2021-02-01 15:30  Heikki Linnakangas <[email protected]>
  0 siblings, 0 replies; 46+ messages in thread

From: Heikki Linnakangas @ 2021-02-01 15:30 UTC (permalink / raw)

Per John Naylor's report, and some bugs I found while testing.

TODO: to be squashed.
---
 src/backend/utils/mb/conv.c                      | 16 ++++++----------
 .../utf8_and_iso8859_1/utf8_and_iso8859_1.c      |  8 ++++++++
 src/backend/utils/mb/mbutils.c                   |  5 +++--
 3 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/src/backend/utils/mb/conv.c b/src/backend/utils/mb/conv.c
index b83358bc7a5..f3bda5753d0 100644
--- a/src/backend/utils/mb/conv.c
+++ b/src/backend/utils/mb/conv.c
@@ -497,7 +497,6 @@ UtfToLocal(const unsigned char *utf, int len,
 	int			l;
 	const pg_utf_to_local_combined *cp;
 	const unsigned char *start = utf;
-	const unsigned char *cur = utf;
 
 	if (!PG_VALID_ENCODING(encoding))
 		ereport(ERROR,
@@ -511,8 +510,6 @@ UtfToLocal(const unsigned char *utf, int len,
 		unsigned char b3 = 0;
 		unsigned char b4 = 0;
 
-		cur = iso;
-
 		/* "break" cases all represent errors */
 		if (*utf == '\0')
 			break;
@@ -648,10 +645,11 @@ UtfToLocal(const unsigned char *utf, int len,
 		}
 
 		/* failed to translate this character */
+		utf -= l;
 		if (noError)
 			break;
 		report_untranslatable_char(PG_UTF8, encoding,
-								   (const char *) (utf - l), len);
+								   (const char *) utf, len);
 	}
 
 	/* if we broke out of loop early, must be invalid input */
@@ -660,7 +658,7 @@ UtfToLocal(const unsigned char *utf, int len,
 
 	*iso = '\0';
 
-	return cur - start;
+	return utf - start;
 }
 
 /*
@@ -701,7 +699,6 @@ LocalToUtf(const unsigned char *iso, int len,
 	int			l;
 	const pg_local_to_utf_combined *cp;
 	const unsigned char *start = iso;
-	const unsigned char *cur = iso;
 
 	if (!PG_VALID_ENCODING(encoding))
 		ereport(ERROR,
@@ -715,8 +712,6 @@ LocalToUtf(const unsigned char *iso, int len,
 		unsigned char b3 = 0;
 		unsigned char b4 = 0;
 
-		cur = iso;
-
 		/* "break" cases all represent errors */
 		if (*iso == '\0')
 			break;
@@ -799,10 +794,11 @@ LocalToUtf(const unsigned char *iso, int len,
 		}
 
 		/* failed to translate this character */
+		iso -= l;
 		if (noError)
 			break;
 		report_untranslatable_char(encoding, PG_UTF8,
-								   (const char *) (iso - l), len);
+								   (const char *) iso, len);
 	}
 
 	/* if we broke out of loop early, must be invalid input */
@@ -811,5 +807,5 @@ LocalToUtf(const unsigned char *iso, int len,
 
 	*utf = '\0';
 
-	return cur - start;
+	return iso - start;
 }
diff --git a/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c b/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
index 8ac93604a1b..d0dc4cca378 100644
--- a/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
+++ b/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
@@ -104,10 +104,18 @@ utf8_to_iso8859_1(PG_FUNCTION_ARGS)
 			int			l = pg_utf_mblen(src);
 
 			if (l > len || !pg_utf8_islegal(src, l))
+			{
+				if (noError)
+					break;
 				report_invalid_encoding(PG_UTF8, (const char *) src, len);
+			}
 			if (l != 2)
+			{
+				if (noError)
+					break;
 				report_untranslatable_char(PG_UTF8, PG_LATIN1,
 										   (const char *) src, len);
+			}
 			c1 = src[1] & 0x3f;
 			c = ((c & 0x1f) << 6) | c1;
 			if (c >= 0x80 && c <= 0xff)
diff --git a/src/backend/utils/mb/mbutils.c b/src/backend/utils/mb/mbutils.c
index 3e106027d75..af1cafe523a 100644
--- a/src/backend/utils/mb/mbutils.c
+++ b/src/backend/utils/mb/mbutils.c
@@ -818,12 +818,13 @@ perform_default_encoding_conversion(const char *src, int len,
 		MemoryContextAllocHuge(CurrentMemoryContext,
 							   (Size) len * MAX_CONVERSION_GROWTH + 1);
 
-	FunctionCall5(flinfo,
+	FunctionCall6(flinfo,
 				  Int32GetDatum(src_encoding),
 				  Int32GetDatum(dest_encoding),
 				  CStringGetDatum(src),
 				  CStringGetDatum(result),
-				  Int32GetDatum(len));
+				  Int32GetDatum(len),
+				  BoolGetDatum(false));
 
 	/*
 	 * Release extra space if there might be a lot --- see comments in
-- 
2.29.2


--------------39E46EF15EBFAA37D53332E5
Content-Type: text/x-patch; charset=UTF-8;
 name="v3-0005-Do-COPY-FROM-encoding-conversion-verification-in-.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename*0="v3-0005-Do-COPY-FROM-encoding-conversion-verification-in-.pa";
 filename*1="tch"



^ permalink  raw  reply  [nested|flat] 46+ messages in thread

* [PATCH v3 4/5] Fix bugs in the commit to change conversion function signature.
@ 2021-02-01 15:30  Heikki Linnakangas <[email protected]>
  0 siblings, 0 replies; 46+ messages in thread

From: Heikki Linnakangas @ 2021-02-01 15:30 UTC (permalink / raw)

Per John Naylor's report, and some bugs I found while testing.

TODO: to be squashed.
---
 src/backend/utils/mb/conv.c                      | 16 ++++++----------
 .../utf8_and_iso8859_1/utf8_and_iso8859_1.c      |  8 ++++++++
 src/backend/utils/mb/mbutils.c                   |  5 +++--
 3 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/src/backend/utils/mb/conv.c b/src/backend/utils/mb/conv.c
index b83358bc7a5..f3bda5753d0 100644
--- a/src/backend/utils/mb/conv.c
+++ b/src/backend/utils/mb/conv.c
@@ -497,7 +497,6 @@ UtfToLocal(const unsigned char *utf, int len,
 	int			l;
 	const pg_utf_to_local_combined *cp;
 	const unsigned char *start = utf;
-	const unsigned char *cur = utf;
 
 	if (!PG_VALID_ENCODING(encoding))
 		ereport(ERROR,
@@ -511,8 +510,6 @@ UtfToLocal(const unsigned char *utf, int len,
 		unsigned char b3 = 0;
 		unsigned char b4 = 0;
 
-		cur = iso;
-
 		/* "break" cases all represent errors */
 		if (*utf == '\0')
 			break;
@@ -648,10 +645,11 @@ UtfToLocal(const unsigned char *utf, int len,
 		}
 
 		/* failed to translate this character */
+		utf -= l;
 		if (noError)
 			break;
 		report_untranslatable_char(PG_UTF8, encoding,
-								   (const char *) (utf - l), len);
+								   (const char *) utf, len);
 	}
 
 	/* if we broke out of loop early, must be invalid input */
@@ -660,7 +658,7 @@ UtfToLocal(const unsigned char *utf, int len,
 
 	*iso = '\0';
 
-	return cur - start;
+	return utf - start;
 }
 
 /*
@@ -701,7 +699,6 @@ LocalToUtf(const unsigned char *iso, int len,
 	int			l;
 	const pg_local_to_utf_combined *cp;
 	const unsigned char *start = iso;
-	const unsigned char *cur = iso;
 
 	if (!PG_VALID_ENCODING(encoding))
 		ereport(ERROR,
@@ -715,8 +712,6 @@ LocalToUtf(const unsigned char *iso, int len,
 		unsigned char b3 = 0;
 		unsigned char b4 = 0;
 
-		cur = iso;
-
 		/* "break" cases all represent errors */
 		if (*iso == '\0')
 			break;
@@ -799,10 +794,11 @@ LocalToUtf(const unsigned char *iso, int len,
 		}
 
 		/* failed to translate this character */
+		iso -= l;
 		if (noError)
 			break;
 		report_untranslatable_char(encoding, PG_UTF8,
-								   (const char *) (iso - l), len);
+								   (const char *) iso, len);
 	}
 
 	/* if we broke out of loop early, must be invalid input */
@@ -811,5 +807,5 @@ LocalToUtf(const unsigned char *iso, int len,
 
 	*utf = '\0';
 
-	return cur - start;
+	return iso - start;
 }
diff --git a/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c b/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
index 8ac93604a1b..d0dc4cca378 100644
--- a/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
+++ b/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
@@ -104,10 +104,18 @@ utf8_to_iso8859_1(PG_FUNCTION_ARGS)
 			int			l = pg_utf_mblen(src);
 
 			if (l > len || !pg_utf8_islegal(src, l))
+			{
+				if (noError)
+					break;
 				report_invalid_encoding(PG_UTF8, (const char *) src, len);
+			}
 			if (l != 2)
+			{
+				if (noError)
+					break;
 				report_untranslatable_char(PG_UTF8, PG_LATIN1,
 										   (const char *) src, len);
+			}
 			c1 = src[1] & 0x3f;
 			c = ((c & 0x1f) << 6) | c1;
 			if (c >= 0x80 && c <= 0xff)
diff --git a/src/backend/utils/mb/mbutils.c b/src/backend/utils/mb/mbutils.c
index 3e106027d75..af1cafe523a 100644
--- a/src/backend/utils/mb/mbutils.c
+++ b/src/backend/utils/mb/mbutils.c
@@ -818,12 +818,13 @@ perform_default_encoding_conversion(const char *src, int len,
 		MemoryContextAllocHuge(CurrentMemoryContext,
 							   (Size) len * MAX_CONVERSION_GROWTH + 1);
 
-	FunctionCall5(flinfo,
+	FunctionCall6(flinfo,
 				  Int32GetDatum(src_encoding),
 				  Int32GetDatum(dest_encoding),
 				  CStringGetDatum(src),
 				  CStringGetDatum(result),
-				  Int32GetDatum(len));
+				  Int32GetDatum(len),
+				  BoolGetDatum(false));
 
 	/*
 	 * Release extra space if there might be a lot --- see comments in
-- 
2.29.2


--------------39E46EF15EBFAA37D53332E5
Content-Type: text/x-patch; charset=UTF-8;
 name="v3-0005-Do-COPY-FROM-encoding-conversion-verification-in-.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename*0="v3-0005-Do-COPY-FROM-encoding-conversion-verification-in-.pa";
 filename*1="tch"



^ permalink  raw  reply  [nested|flat] 46+ messages in thread

* [PATCH v3 4/5] Fix bugs in the commit to change conversion function signature.
@ 2021-02-01 15:30  Heikki Linnakangas <[email protected]>
  0 siblings, 0 replies; 46+ messages in thread

From: Heikki Linnakangas @ 2021-02-01 15:30 UTC (permalink / raw)

Per John Naylor's report, and some bugs I found while testing.

TODO: to be squashed.
---
 src/backend/utils/mb/conv.c                      | 16 ++++++----------
 .../utf8_and_iso8859_1/utf8_and_iso8859_1.c      |  8 ++++++++
 src/backend/utils/mb/mbutils.c                   |  5 +++--
 3 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/src/backend/utils/mb/conv.c b/src/backend/utils/mb/conv.c
index b83358bc7a5..f3bda5753d0 100644
--- a/src/backend/utils/mb/conv.c
+++ b/src/backend/utils/mb/conv.c
@@ -497,7 +497,6 @@ UtfToLocal(const unsigned char *utf, int len,
 	int			l;
 	const pg_utf_to_local_combined *cp;
 	const unsigned char *start = utf;
-	const unsigned char *cur = utf;
 
 	if (!PG_VALID_ENCODING(encoding))
 		ereport(ERROR,
@@ -511,8 +510,6 @@ UtfToLocal(const unsigned char *utf, int len,
 		unsigned char b3 = 0;
 		unsigned char b4 = 0;
 
-		cur = iso;
-
 		/* "break" cases all represent errors */
 		if (*utf == '\0')
 			break;
@@ -648,10 +645,11 @@ UtfToLocal(const unsigned char *utf, int len,
 		}
 
 		/* failed to translate this character */
+		utf -= l;
 		if (noError)
 			break;
 		report_untranslatable_char(PG_UTF8, encoding,
-								   (const char *) (utf - l), len);
+								   (const char *) utf, len);
 	}
 
 	/* if we broke out of loop early, must be invalid input */
@@ -660,7 +658,7 @@ UtfToLocal(const unsigned char *utf, int len,
 
 	*iso = '\0';
 
-	return cur - start;
+	return utf - start;
 }
 
 /*
@@ -701,7 +699,6 @@ LocalToUtf(const unsigned char *iso, int len,
 	int			l;
 	const pg_local_to_utf_combined *cp;
 	const unsigned char *start = iso;
-	const unsigned char *cur = iso;
 
 	if (!PG_VALID_ENCODING(encoding))
 		ereport(ERROR,
@@ -715,8 +712,6 @@ LocalToUtf(const unsigned char *iso, int len,
 		unsigned char b3 = 0;
 		unsigned char b4 = 0;
 
-		cur = iso;
-
 		/* "break" cases all represent errors */
 		if (*iso == '\0')
 			break;
@@ -799,10 +794,11 @@ LocalToUtf(const unsigned char *iso, int len,
 		}
 
 		/* failed to translate this character */
+		iso -= l;
 		if (noError)
 			break;
 		report_untranslatable_char(encoding, PG_UTF8,
-								   (const char *) (iso - l), len);
+								   (const char *) iso, len);
 	}
 
 	/* if we broke out of loop early, must be invalid input */
@@ -811,5 +807,5 @@ LocalToUtf(const unsigned char *iso, int len,
 
 	*utf = '\0';
 
-	return cur - start;
+	return iso - start;
 }
diff --git a/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c b/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
index 8ac93604a1b..d0dc4cca378 100644
--- a/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
+++ b/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
@@ -104,10 +104,18 @@ utf8_to_iso8859_1(PG_FUNCTION_ARGS)
 			int			l = pg_utf_mblen(src);
 
 			if (l > len || !pg_utf8_islegal(src, l))
+			{
+				if (noError)
+					break;
 				report_invalid_encoding(PG_UTF8, (const char *) src, len);
+			}
 			if (l != 2)
+			{
+				if (noError)
+					break;
 				report_untranslatable_char(PG_UTF8, PG_LATIN1,
 										   (const char *) src, len);
+			}
 			c1 = src[1] & 0x3f;
 			c = ((c & 0x1f) << 6) | c1;
 			if (c >= 0x80 && c <= 0xff)
diff --git a/src/backend/utils/mb/mbutils.c b/src/backend/utils/mb/mbutils.c
index 3e106027d75..af1cafe523a 100644
--- a/src/backend/utils/mb/mbutils.c
+++ b/src/backend/utils/mb/mbutils.c
@@ -818,12 +818,13 @@ perform_default_encoding_conversion(const char *src, int len,
 		MemoryContextAllocHuge(CurrentMemoryContext,
 							   (Size) len * MAX_CONVERSION_GROWTH + 1);
 
-	FunctionCall5(flinfo,
+	FunctionCall6(flinfo,
 				  Int32GetDatum(src_encoding),
 				  Int32GetDatum(dest_encoding),
 				  CStringGetDatum(src),
 				  CStringGetDatum(result),
-				  Int32GetDatum(len));
+				  Int32GetDatum(len),
+				  BoolGetDatum(false));
 
 	/*
 	 * Release extra space if there might be a lot --- see comments in
-- 
2.29.2


--------------39E46EF15EBFAA37D53332E5
Content-Type: text/x-patch; charset=UTF-8;
 name="v3-0005-Do-COPY-FROM-encoding-conversion-verification-in-.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename*0="v3-0005-Do-COPY-FROM-encoding-conversion-verification-in-.pa";
 filename*1="tch"



^ permalink  raw  reply  [nested|flat] 46+ messages in thread

* [PATCH v3 4/5] Fix bugs in the commit to change conversion function signature.
@ 2021-02-01 15:30  Heikki Linnakangas <[email protected]>
  0 siblings, 0 replies; 46+ messages in thread

From: Heikki Linnakangas @ 2021-02-01 15:30 UTC (permalink / raw)

Per John Naylor's report, and some bugs I found while testing.

TODO: to be squashed.
---
 src/backend/utils/mb/conv.c                      | 16 ++++++----------
 .../utf8_and_iso8859_1/utf8_and_iso8859_1.c      |  8 ++++++++
 src/backend/utils/mb/mbutils.c                   |  5 +++--
 3 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/src/backend/utils/mb/conv.c b/src/backend/utils/mb/conv.c
index b83358bc7a5..f3bda5753d0 100644
--- a/src/backend/utils/mb/conv.c
+++ b/src/backend/utils/mb/conv.c
@@ -497,7 +497,6 @@ UtfToLocal(const unsigned char *utf, int len,
 	int			l;
 	const pg_utf_to_local_combined *cp;
 	const unsigned char *start = utf;
-	const unsigned char *cur = utf;
 
 	if (!PG_VALID_ENCODING(encoding))
 		ereport(ERROR,
@@ -511,8 +510,6 @@ UtfToLocal(const unsigned char *utf, int len,
 		unsigned char b3 = 0;
 		unsigned char b4 = 0;
 
-		cur = iso;
-
 		/* "break" cases all represent errors */
 		if (*utf == '\0')
 			break;
@@ -648,10 +645,11 @@ UtfToLocal(const unsigned char *utf, int len,
 		}
 
 		/* failed to translate this character */
+		utf -= l;
 		if (noError)
 			break;
 		report_untranslatable_char(PG_UTF8, encoding,
-								   (const char *) (utf - l), len);
+								   (const char *) utf, len);
 	}
 
 	/* if we broke out of loop early, must be invalid input */
@@ -660,7 +658,7 @@ UtfToLocal(const unsigned char *utf, int len,
 
 	*iso = '\0';
 
-	return cur - start;
+	return utf - start;
 }
 
 /*
@@ -701,7 +699,6 @@ LocalToUtf(const unsigned char *iso, int len,
 	int			l;
 	const pg_local_to_utf_combined *cp;
 	const unsigned char *start = iso;
-	const unsigned char *cur = iso;
 
 	if (!PG_VALID_ENCODING(encoding))
 		ereport(ERROR,
@@ -715,8 +712,6 @@ LocalToUtf(const unsigned char *iso, int len,
 		unsigned char b3 = 0;
 		unsigned char b4 = 0;
 
-		cur = iso;
-
 		/* "break" cases all represent errors */
 		if (*iso == '\0')
 			break;
@@ -799,10 +794,11 @@ LocalToUtf(const unsigned char *iso, int len,
 		}
 
 		/* failed to translate this character */
+		iso -= l;
 		if (noError)
 			break;
 		report_untranslatable_char(encoding, PG_UTF8,
-								   (const char *) (iso - l), len);
+								   (const char *) iso, len);
 	}
 
 	/* if we broke out of loop early, must be invalid input */
@@ -811,5 +807,5 @@ LocalToUtf(const unsigned char *iso, int len,
 
 	*utf = '\0';
 
-	return cur - start;
+	return iso - start;
 }
diff --git a/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c b/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
index 8ac93604a1b..d0dc4cca378 100644
--- a/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
+++ b/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
@@ -104,10 +104,18 @@ utf8_to_iso8859_1(PG_FUNCTION_ARGS)
 			int			l = pg_utf_mblen(src);
 
 			if (l > len || !pg_utf8_islegal(src, l))
+			{
+				if (noError)
+					break;
 				report_invalid_encoding(PG_UTF8, (const char *) src, len);
+			}
 			if (l != 2)
+			{
+				if (noError)
+					break;
 				report_untranslatable_char(PG_UTF8, PG_LATIN1,
 										   (const char *) src, len);
+			}
 			c1 = src[1] & 0x3f;
 			c = ((c & 0x1f) << 6) | c1;
 			if (c >= 0x80 && c <= 0xff)
diff --git a/src/backend/utils/mb/mbutils.c b/src/backend/utils/mb/mbutils.c
index 3e106027d75..af1cafe523a 100644
--- a/src/backend/utils/mb/mbutils.c
+++ b/src/backend/utils/mb/mbutils.c
@@ -818,12 +818,13 @@ perform_default_encoding_conversion(const char *src, int len,
 		MemoryContextAllocHuge(CurrentMemoryContext,
 							   (Size) len * MAX_CONVERSION_GROWTH + 1);
 
-	FunctionCall5(flinfo,
+	FunctionCall6(flinfo,
 				  Int32GetDatum(src_encoding),
 				  Int32GetDatum(dest_encoding),
 				  CStringGetDatum(src),
 				  CStringGetDatum(result),
-				  Int32GetDatum(len));
+				  Int32GetDatum(len),
+				  BoolGetDatum(false));
 
 	/*
 	 * Release extra space if there might be a lot --- see comments in
-- 
2.29.2


--------------39E46EF15EBFAA37D53332E5
Content-Type: text/x-patch; charset=UTF-8;
 name="v3-0005-Do-COPY-FROM-encoding-conversion-verification-in-.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename*0="v3-0005-Do-COPY-FROM-encoding-conversion-verification-in-.pa";
 filename*1="tch"



^ permalink  raw  reply  [nested|flat] 46+ messages in thread

* [PATCH v3 4/5] Fix bugs in the commit to change conversion function signature.
@ 2021-02-01 15:30  Heikki Linnakangas <[email protected]>
  0 siblings, 0 replies; 46+ messages in thread

From: Heikki Linnakangas @ 2021-02-01 15:30 UTC (permalink / raw)

Per John Naylor's report, and some bugs I found while testing.

TODO: to be squashed.
---
 src/backend/utils/mb/conv.c                      | 16 ++++++----------
 .../utf8_and_iso8859_1/utf8_and_iso8859_1.c      |  8 ++++++++
 src/backend/utils/mb/mbutils.c                   |  5 +++--
 3 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/src/backend/utils/mb/conv.c b/src/backend/utils/mb/conv.c
index b83358bc7a5..f3bda5753d0 100644
--- a/src/backend/utils/mb/conv.c
+++ b/src/backend/utils/mb/conv.c
@@ -497,7 +497,6 @@ UtfToLocal(const unsigned char *utf, int len,
 	int			l;
 	const pg_utf_to_local_combined *cp;
 	const unsigned char *start = utf;
-	const unsigned char *cur = utf;
 
 	if (!PG_VALID_ENCODING(encoding))
 		ereport(ERROR,
@@ -511,8 +510,6 @@ UtfToLocal(const unsigned char *utf, int len,
 		unsigned char b3 = 0;
 		unsigned char b4 = 0;
 
-		cur = iso;
-
 		/* "break" cases all represent errors */
 		if (*utf == '\0')
 			break;
@@ -648,10 +645,11 @@ UtfToLocal(const unsigned char *utf, int len,
 		}
 
 		/* failed to translate this character */
+		utf -= l;
 		if (noError)
 			break;
 		report_untranslatable_char(PG_UTF8, encoding,
-								   (const char *) (utf - l), len);
+								   (const char *) utf, len);
 	}
 
 	/* if we broke out of loop early, must be invalid input */
@@ -660,7 +658,7 @@ UtfToLocal(const unsigned char *utf, int len,
 
 	*iso = '\0';
 
-	return cur - start;
+	return utf - start;
 }
 
 /*
@@ -701,7 +699,6 @@ LocalToUtf(const unsigned char *iso, int len,
 	int			l;
 	const pg_local_to_utf_combined *cp;
 	const unsigned char *start = iso;
-	const unsigned char *cur = iso;
 
 	if (!PG_VALID_ENCODING(encoding))
 		ereport(ERROR,
@@ -715,8 +712,6 @@ LocalToUtf(const unsigned char *iso, int len,
 		unsigned char b3 = 0;
 		unsigned char b4 = 0;
 
-		cur = iso;
-
 		/* "break" cases all represent errors */
 		if (*iso == '\0')
 			break;
@@ -799,10 +794,11 @@ LocalToUtf(const unsigned char *iso, int len,
 		}
 
 		/* failed to translate this character */
+		iso -= l;
 		if (noError)
 			break;
 		report_untranslatable_char(encoding, PG_UTF8,
-								   (const char *) (iso - l), len);
+								   (const char *) iso, len);
 	}
 
 	/* if we broke out of loop early, must be invalid input */
@@ -811,5 +807,5 @@ LocalToUtf(const unsigned char *iso, int len,
 
 	*utf = '\0';
 
-	return cur - start;
+	return iso - start;
 }
diff --git a/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c b/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
index 8ac93604a1b..d0dc4cca378 100644
--- a/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
+++ b/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
@@ -104,10 +104,18 @@ utf8_to_iso8859_1(PG_FUNCTION_ARGS)
 			int			l = pg_utf_mblen(src);
 
 			if (l > len || !pg_utf8_islegal(src, l))
+			{
+				if (noError)
+					break;
 				report_invalid_encoding(PG_UTF8, (const char *) src, len);
+			}
 			if (l != 2)
+			{
+				if (noError)
+					break;
 				report_untranslatable_char(PG_UTF8, PG_LATIN1,
 										   (const char *) src, len);
+			}
 			c1 = src[1] & 0x3f;
 			c = ((c & 0x1f) << 6) | c1;
 			if (c >= 0x80 && c <= 0xff)
diff --git a/src/backend/utils/mb/mbutils.c b/src/backend/utils/mb/mbutils.c
index 3e106027d75..af1cafe523a 100644
--- a/src/backend/utils/mb/mbutils.c
+++ b/src/backend/utils/mb/mbutils.c
@@ -818,12 +818,13 @@ perform_default_encoding_conversion(const char *src, int len,
 		MemoryContextAllocHuge(CurrentMemoryContext,
 							   (Size) len * MAX_CONVERSION_GROWTH + 1);
 
-	FunctionCall5(flinfo,
+	FunctionCall6(flinfo,
 				  Int32GetDatum(src_encoding),
 				  Int32GetDatum(dest_encoding),
 				  CStringGetDatum(src),
 				  CStringGetDatum(result),
-				  Int32GetDatum(len));
+				  Int32GetDatum(len),
+				  BoolGetDatum(false));
 
 	/*
 	 * Release extra space if there might be a lot --- see comments in
-- 
2.29.2


--------------39E46EF15EBFAA37D53332E5
Content-Type: text/x-patch; charset=UTF-8;
 name="v3-0005-Do-COPY-FROM-encoding-conversion-verification-in-.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename*0="v3-0005-Do-COPY-FROM-encoding-conversion-verification-in-.pa";
 filename*1="tch"



^ permalink  raw  reply  [nested|flat] 46+ messages in thread

* [PATCH v3 4/5] Fix bugs in the commit to change conversion function signature.
@ 2021-02-01 15:30  Heikki Linnakangas <[email protected]>
  0 siblings, 0 replies; 46+ messages in thread

From: Heikki Linnakangas @ 2021-02-01 15:30 UTC (permalink / raw)

Per John Naylor's report, and some bugs I found while testing.

TODO: to be squashed.
---
 src/backend/utils/mb/conv.c                      | 16 ++++++----------
 .../utf8_and_iso8859_1/utf8_and_iso8859_1.c      |  8 ++++++++
 src/backend/utils/mb/mbutils.c                   |  5 +++--
 3 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/src/backend/utils/mb/conv.c b/src/backend/utils/mb/conv.c
index b83358bc7a5..f3bda5753d0 100644
--- a/src/backend/utils/mb/conv.c
+++ b/src/backend/utils/mb/conv.c
@@ -497,7 +497,6 @@ UtfToLocal(const unsigned char *utf, int len,
 	int			l;
 	const pg_utf_to_local_combined *cp;
 	const unsigned char *start = utf;
-	const unsigned char *cur = utf;
 
 	if (!PG_VALID_ENCODING(encoding))
 		ereport(ERROR,
@@ -511,8 +510,6 @@ UtfToLocal(const unsigned char *utf, int len,
 		unsigned char b3 = 0;
 		unsigned char b4 = 0;
 
-		cur = iso;
-
 		/* "break" cases all represent errors */
 		if (*utf == '\0')
 			break;
@@ -648,10 +645,11 @@ UtfToLocal(const unsigned char *utf, int len,
 		}
 
 		/* failed to translate this character */
+		utf -= l;
 		if (noError)
 			break;
 		report_untranslatable_char(PG_UTF8, encoding,
-								   (const char *) (utf - l), len);
+								   (const char *) utf, len);
 	}
 
 	/* if we broke out of loop early, must be invalid input */
@@ -660,7 +658,7 @@ UtfToLocal(const unsigned char *utf, int len,
 
 	*iso = '\0';
 
-	return cur - start;
+	return utf - start;
 }
 
 /*
@@ -701,7 +699,6 @@ LocalToUtf(const unsigned char *iso, int len,
 	int			l;
 	const pg_local_to_utf_combined *cp;
 	const unsigned char *start = iso;
-	const unsigned char *cur = iso;
 
 	if (!PG_VALID_ENCODING(encoding))
 		ereport(ERROR,
@@ -715,8 +712,6 @@ LocalToUtf(const unsigned char *iso, int len,
 		unsigned char b3 = 0;
 		unsigned char b4 = 0;
 
-		cur = iso;
-
 		/* "break" cases all represent errors */
 		if (*iso == '\0')
 			break;
@@ -799,10 +794,11 @@ LocalToUtf(const unsigned char *iso, int len,
 		}
 
 		/* failed to translate this character */
+		iso -= l;
 		if (noError)
 			break;
 		report_untranslatable_char(encoding, PG_UTF8,
-								   (const char *) (iso - l), len);
+								   (const char *) iso, len);
 	}
 
 	/* if we broke out of loop early, must be invalid input */
@@ -811,5 +807,5 @@ LocalToUtf(const unsigned char *iso, int len,
 
 	*utf = '\0';
 
-	return cur - start;
+	return iso - start;
 }
diff --git a/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c b/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
index 8ac93604a1b..d0dc4cca378 100644
--- a/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
+++ b/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
@@ -104,10 +104,18 @@ utf8_to_iso8859_1(PG_FUNCTION_ARGS)
 			int			l = pg_utf_mblen(src);
 
 			if (l > len || !pg_utf8_islegal(src, l))
+			{
+				if (noError)
+					break;
 				report_invalid_encoding(PG_UTF8, (const char *) src, len);
+			}
 			if (l != 2)
+			{
+				if (noError)
+					break;
 				report_untranslatable_char(PG_UTF8, PG_LATIN1,
 										   (const char *) src, len);
+			}
 			c1 = src[1] & 0x3f;
 			c = ((c & 0x1f) << 6) | c1;
 			if (c >= 0x80 && c <= 0xff)
diff --git a/src/backend/utils/mb/mbutils.c b/src/backend/utils/mb/mbutils.c
index 3e106027d75..af1cafe523a 100644
--- a/src/backend/utils/mb/mbutils.c
+++ b/src/backend/utils/mb/mbutils.c
@@ -818,12 +818,13 @@ perform_default_encoding_conversion(const char *src, int len,
 		MemoryContextAllocHuge(CurrentMemoryContext,
 							   (Size) len * MAX_CONVERSION_GROWTH + 1);
 
-	FunctionCall5(flinfo,
+	FunctionCall6(flinfo,
 				  Int32GetDatum(src_encoding),
 				  Int32GetDatum(dest_encoding),
 				  CStringGetDatum(src),
 				  CStringGetDatum(result),
-				  Int32GetDatum(len));
+				  Int32GetDatum(len),
+				  BoolGetDatum(false));
 
 	/*
 	 * Release extra space if there might be a lot --- see comments in
-- 
2.29.2


--------------39E46EF15EBFAA37D53332E5
Content-Type: text/x-patch; charset=UTF-8;
 name="v3-0005-Do-COPY-FROM-encoding-conversion-verification-in-.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename*0="v3-0005-Do-COPY-FROM-encoding-conversion-verification-in-.pa";
 filename*1="tch"



^ permalink  raw  reply  [nested|flat] 46+ messages in thread

* [PATCH v3 4/5] Fix bugs in the commit to change conversion function signature.
@ 2021-02-01 15:30  Heikki Linnakangas <[email protected]>
  0 siblings, 0 replies; 46+ messages in thread

From: Heikki Linnakangas @ 2021-02-01 15:30 UTC (permalink / raw)

Per John Naylor's report, and some bugs I found while testing.

TODO: to be squashed.
---
 src/backend/utils/mb/conv.c                      | 16 ++++++----------
 .../utf8_and_iso8859_1/utf8_and_iso8859_1.c      |  8 ++++++++
 src/backend/utils/mb/mbutils.c                   |  5 +++--
 3 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/src/backend/utils/mb/conv.c b/src/backend/utils/mb/conv.c
index b83358bc7a5..f3bda5753d0 100644
--- a/src/backend/utils/mb/conv.c
+++ b/src/backend/utils/mb/conv.c
@@ -497,7 +497,6 @@ UtfToLocal(const unsigned char *utf, int len,
 	int			l;
 	const pg_utf_to_local_combined *cp;
 	const unsigned char *start = utf;
-	const unsigned char *cur = utf;
 
 	if (!PG_VALID_ENCODING(encoding))
 		ereport(ERROR,
@@ -511,8 +510,6 @@ UtfToLocal(const unsigned char *utf, int len,
 		unsigned char b3 = 0;
 		unsigned char b4 = 0;
 
-		cur = iso;
-
 		/* "break" cases all represent errors */
 		if (*utf == '\0')
 			break;
@@ -648,10 +645,11 @@ UtfToLocal(const unsigned char *utf, int len,
 		}
 
 		/* failed to translate this character */
+		utf -= l;
 		if (noError)
 			break;
 		report_untranslatable_char(PG_UTF8, encoding,
-								   (const char *) (utf - l), len);
+								   (const char *) utf, len);
 	}
 
 	/* if we broke out of loop early, must be invalid input */
@@ -660,7 +658,7 @@ UtfToLocal(const unsigned char *utf, int len,
 
 	*iso = '\0';
 
-	return cur - start;
+	return utf - start;
 }
 
 /*
@@ -701,7 +699,6 @@ LocalToUtf(const unsigned char *iso, int len,
 	int			l;
 	const pg_local_to_utf_combined *cp;
 	const unsigned char *start = iso;
-	const unsigned char *cur = iso;
 
 	if (!PG_VALID_ENCODING(encoding))
 		ereport(ERROR,
@@ -715,8 +712,6 @@ LocalToUtf(const unsigned char *iso, int len,
 		unsigned char b3 = 0;
 		unsigned char b4 = 0;
 
-		cur = iso;
-
 		/* "break" cases all represent errors */
 		if (*iso == '\0')
 			break;
@@ -799,10 +794,11 @@ LocalToUtf(const unsigned char *iso, int len,
 		}
 
 		/* failed to translate this character */
+		iso -= l;
 		if (noError)
 			break;
 		report_untranslatable_char(encoding, PG_UTF8,
-								   (const char *) (iso - l), len);
+								   (const char *) iso, len);
 	}
 
 	/* if we broke out of loop early, must be invalid input */
@@ -811,5 +807,5 @@ LocalToUtf(const unsigned char *iso, int len,
 
 	*utf = '\0';
 
-	return cur - start;
+	return iso - start;
 }
diff --git a/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c b/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
index 8ac93604a1b..d0dc4cca378 100644
--- a/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
+++ b/src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/utf8_and_iso8859_1.c
@@ -104,10 +104,18 @@ utf8_to_iso8859_1(PG_FUNCTION_ARGS)
 			int			l = pg_utf_mblen(src);
 
 			if (l > len || !pg_utf8_islegal(src, l))
+			{
+				if (noError)
+					break;
 				report_invalid_encoding(PG_UTF8, (const char *) src, len);
+			}
 			if (l != 2)
+			{
+				if (noError)
+					break;
 				report_untranslatable_char(PG_UTF8, PG_LATIN1,
 										   (const char *) src, len);
+			}
 			c1 = src[1] & 0x3f;
 			c = ((c & 0x1f) << 6) | c1;
 			if (c >= 0x80 && c <= 0xff)
diff --git a/src/backend/utils/mb/mbutils.c b/src/backend/utils/mb/mbutils.c
index 3e106027d75..af1cafe523a 100644
--- a/src/backend/utils/mb/mbutils.c
+++ b/src/backend/utils/mb/mbutils.c
@@ -818,12 +818,13 @@ perform_default_encoding_conversion(const char *src, int len,
 		MemoryContextAllocHuge(CurrentMemoryContext,
 							   (Size) len * MAX_CONVERSION_GROWTH + 1);
 
-	FunctionCall5(flinfo,
+	FunctionCall6(flinfo,
 				  Int32GetDatum(src_encoding),
 				  Int32GetDatum(dest_encoding),
 				  CStringGetDatum(src),
 				  CStringGetDatum(result),
-				  Int32GetDatum(len));
+				  Int32GetDatum(len),
+				  BoolGetDatum(false));
 
 	/*
 	 * Release extra space if there might be a lot --- see comments in
-- 
2.29.2


--------------39E46EF15EBFAA37D53332E5
Content-Type: text/x-patch; charset=UTF-8;
 name="v3-0005-Do-COPY-FROM-encoding-conversion-verification-in-.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename*0="v3-0005-Do-COPY-FROM-encoding-conversion-verification-in-.pa";
 filename*1="tch"



^ permalink  raw  reply  [nested|flat] 46+ messages in thread

* Re: Fix an incorrect assertion condition in mdwritev().
@ 2024-06-03 22:24  Andres Freund <[email protected]>
  0 siblings, 2 replies; 46+ messages in thread

From: Andres Freund @ 2024-06-03 22:24 UTC (permalink / raw)
  To: Michael Paquier <[email protected]>; +Cc: Tom Lane <[email protected]>; Xing Guo <[email protected]>; [email protected]

Hi,

On 2024-06-04 07:17:51 +0900, Michael Paquier wrote:
> On Sun, May 26, 2024 at 12:08:46AM -0400, Tom Lane wrote:
> > After a few minutes' thought, how about:
> > 
> > 	Assert((uint64) blocknum + (uint64) nblocks <= (uint64) mdnblocks(reln, forknum));
> > 
> > This'd stop being helpful if we ever widen BlockNumber to 64 bits,
> > but I think that's unlikely.  (Partitioning seems like a better answer
> > for giant tables.)
> 
> No idea if this will happen or not, but that's not the only area where
> we are going to need a native uint128 implementation to control the
> overflows with uint64.

I'm confused - isn't using common/int.h entirely sufficient for that? Nearly
all architectures have more efficient ways to check for 64bit overflows than
doing actual 128 bit math.

Greetings,

Andres Freund






^ permalink  raw  reply  [nested|flat] 46+ messages in thread

* Re: Fix an incorrect assertion condition in mdwritev().
@ 2024-06-03 22:43  Michael Paquier <[email protected]>
  parent: Andres Freund <[email protected]>
  1 sibling, 0 replies; 46+ messages in thread

From: Michael Paquier @ 2024-06-03 22:43 UTC (permalink / raw)
  To: Andres Freund <[email protected]>; +Cc: Tom Lane <[email protected]>; Xing Guo <[email protected]>; [email protected]

On Mon, Jun 03, 2024 at 03:24:07PM -0700, Andres Freund wrote:
> I'm confused - isn't using common/int.h entirely sufficient for that? Nearly
> all architectures have more efficient ways to check for 64bit overflows than
> doing actual 128 bit math.

Oh, right.  We could just plug in pg_add_u32_overflow here.  Funny
thing is that I'm the one who committed these toys with
__builtin_add_overflow(), still nobody has found a case where this one
would be useful.  At least until now.
--
Michael


Attachments:

  [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
  download

^ permalink  raw  reply  [nested|flat] 46+ messages in thread

* Re: Fix an incorrect assertion condition in mdwritev().
@ 2024-06-05 04:10  Michael Paquier <[email protected]>
  parent: Andres Freund <[email protected]>
  1 sibling, 0 replies; 46+ messages in thread

From: Michael Paquier @ 2024-06-05 04:10 UTC (permalink / raw)
  To: Andres Freund <[email protected]>; +Cc: Tom Lane <[email protected]>; Xing Guo <[email protected]>; [email protected]

On Mon, Jun 03, 2024 at 03:24:07PM -0700, Andres Freund wrote:
> I'm confused - isn't using common/int.h entirely sufficient for that? Nearly
> all architectures have more efficient ways to check for 64bit overflows than
> doing actual 128 bit math.

One simple way to change the assertion would be something like that, I
assume.  Andres, does it answer your concerns?
--
Michael


Attachments:

  [text/x-diff] assert_mdwritev.patch (977B, ../../[email protected]/2-assert_mdwritev.patch)
  download | inline diff:
diff --git a/src/backend/storage/smgr/md.c b/src/backend/storage/smgr/md.c
index 6796756358..3849397b25 100644
--- a/src/backend/storage/smgr/md.c
+++ b/src/backend/storage/smgr/md.c
@@ -28,6 +28,7 @@
 #include "access/xlogutils.h"
 #include "commands/tablespace.h"
 #include "common/file_utils.h"
+#include "common/int.h"
 #include "miscadmin.h"
 #include "pg_trace.h"
 #include "pgstat.h"
@@ -929,8 +930,13 @@ mdwritev(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum,
 		 const void **buffers, BlockNumber nblocks, bool skipFsync)
 {
 	/* This assert is too expensive to have on normally ... */
-#ifdef CHECK_WRITE_VS_EXTEND
-	Assert((uint64) blocknum + (uint64) nblocks <= (uint64) mdnblocks(reln, forknum));
+#if defined(USE_ASSERT_CHECKING) && defined(CHECK_WRITE_VS_EXTEND)
+	uint32		tot_blocks;
+
+	if (pg_add_u32_overflow(blocknum, nblocks, &tot_blocks))
+		Assert(false);
+
+	Assert(tot_blocks <= mdnblocks(reln, forknum));
 #endif
 
 	while (nblocks > 0)


  [application/pgp-signature] signature.asc (833B, ../../[email protected]/3-signature.asc)
  download

^ permalink  raw  reply  [nested|flat] 46+ messages in thread

end of thread, other threads:[~2024-06-05 04:10 UTC | newest]

Thread overview: 46+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2017-06-19 18:24 Optional message to user when terminating/cancelling backend Daniel Gustafsson <[email protected]>
2017-06-19 18:40 ` Pavel Stehule <[email protected]>
2017-06-19 19:08   ` Satyanarayana Narlapuram <[email protected]>
2017-06-20 18:54     ` Alvaro Herrera <[email protected]>
2017-06-20 19:43       ` David G. Johnston <[email protected]>
2017-06-20 19:49         ` Satyanarayana Narlapuram <[email protected]>
2017-06-26 14:15       ` Joel Jacobson <[email protected]>
2017-06-26 14:39         ` Joshua D. Drake <[email protected]>
2017-06-20 20:01 ` Andres Freund <[email protected]>
2017-06-21 03:06 ` Michael Paquier <[email protected]>
2017-06-21 14:30   ` Yugo Nagata <[email protected]>
2017-06-21 14:42     ` Daniel Gustafsson <[email protected]>
2017-06-22 00:24       ` Michael Paquier <[email protected]>
2017-06-22 08:24         ` Yugo Nagata <[email protected]>
2017-06-22 13:48           ` Daniel Gustafsson <[email protected]>
2017-06-22 15:52             ` Dilip Kumar <[email protected]>
2017-06-26 07:44               ` Daniel Gustafsson <[email protected]>
2017-09-02 00:21             ` Thomas Munro <[email protected]>
2017-09-03 20:47               ` Daniel Gustafsson <[email protected]>
2017-09-28 12:55                 ` Yugo Nagata <[email protected]>
2017-06-21 13:57 ` Yugo Nagata <[email protected]>
2021-02-01 15:30 [PATCH v3 4/5] Fix bugs in the commit to change conversion function signature. Heikki Linnakangas <[email protected]>
2021-02-01 15:30 [PATCH v3 4/5] Fix bugs in the commit to change conversion function signature. Heikki Linnakangas <[email protected]>
2021-02-01 15:30 [PATCH v3 4/5] Fix bugs in the commit to change conversion function signature. Heikki Linnakangas <[email protected]>
2021-02-01 15:30 [PATCH v3 4/5] Fix bugs in the commit to change conversion function signature. Heikki Linnakangas <[email protected]>
2021-02-01 15:30 [PATCH v3 4/5] Fix bugs in the commit to change conversion function signature. Heikki Linnakangas <[email protected]>
2021-02-01 15:30 [PATCH v3 4/5] Fix bugs in the commit to change conversion function signature. Heikki Linnakangas <[email protected]>
2021-02-01 15:30 [PATCH v3 4/5] Fix bugs in the commit to change conversion function signature. Heikki Linnakangas <[email protected]>
2021-02-01 15:30 [PATCH v3 4/5] Fix bugs in the commit to change conversion function signature. Heikki Linnakangas <[email protected]>
2021-02-01 15:30 [PATCH v3 4/5] Fix bugs in the commit to change conversion function signature. Heikki Linnakangas <[email protected]>
2021-02-01 15:30 [PATCH v3 4/5] Fix bugs in the commit to change conversion function signature. Heikki Linnakangas <[email protected]>
2021-02-01 15:30 [PATCH v3 4/5] Fix bugs in the commit to change conversion function signature. Heikki Linnakangas <[email protected]>
2021-02-01 15:30 [PATCH v3 4/5] Fix bugs in the commit to change conversion function signature. Heikki Linnakangas <[email protected]>
2021-02-01 15:30 [PATCH v3 4/5] Fix bugs in the commit to change conversion function signature. Heikki Linnakangas <[email protected]>
2021-02-01 15:30 [PATCH v3 4/5] Fix bugs in the commit to change conversion function signature. Heikki Linnakangas <[email protected]>
2021-02-01 15:30 [PATCH v3 4/5] Fix bugs in the commit to change conversion function signature. Heikki Linnakangas <[email protected]>
2021-02-01 15:30 [PATCH v3 4/5] Fix bugs in the commit to change conversion function signature. Heikki Linnakangas <[email protected]>
2021-02-01 15:30 [PATCH v3 4/5] Fix bugs in the commit to change conversion function signature. Heikki Linnakangas <[email protected]>
2021-02-01 15:30 [PATCH v3 4/5] Fix bugs in the commit to change conversion function signature. Heikki Linnakangas <[email protected]>
2021-02-01 15:30 [PATCH v3 4/5] Fix bugs in the commit to change conversion function signature. Heikki Linnakangas <[email protected]>
2021-02-01 15:30 [PATCH v3 4/5] Fix bugs in the commit to change conversion function signature. Heikki Linnakangas <[email protected]>
2021-02-01 15:30 [PATCH v3 4/5] Fix bugs in the commit to change conversion function signature. Heikki Linnakangas <[email protected]>
2021-02-01 15:30 [PATCH v3 4/5] Fix bugs in the commit to change conversion function signature. Heikki Linnakangas <[email protected]>
2024-06-03 22:24 Re: Fix an incorrect assertion condition in mdwritev(). Andres Freund <[email protected]>
2024-06-03 22:43 ` Re: Fix an incorrect assertion condition in mdwritev(). Michael Paquier <[email protected]>
2024-06-05 04:10 ` Re: Fix an incorrect assertion condition in mdwritev(). Michael Paquier <[email protected]>

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox