/ext/pgsql: Remove unnecessary +1 in memcpy when appending newline

[LamentXU123]

Same as #21564 and #21567

[iluuu1994]

The difference being that we don't need an actual zend_string in this case, but only a char*. This will increase the allocation by 24 bytes (offsetof(zend_string, val)), which might or might not be worth it for the improved readability.

The previous + 1 in memcpy(zquery, ZSTR_VAL(tmp), ZSTR_LEN(tmp) + 1); was superfluous though, given the \0 is immediately overwritten with \n on the next line.

[LamentXU123]

The difference being that we don't need an actual zend_string in this case, but only a char*. This will increase the allocation by 24 bytes (offsetof(zend_string, val)), which might or might not be worth it for the improved readability.

The previous + 1 in memcpy(zquery, ZSTR_VAL(tmp), ZSTR_LEN(tmp) + 1); was superfluous though, given the \0 is immediately overwritten with \n on the next line.

I didn't realize the zend_string_concat api will lower performance. Given the fact that it actually is, I don't think this change would be good here since the original readability is not bad enough. Also including #21598.

However, I do think the +1 is redundant. I would switch this PR to fix this soon.

[LamentXU123]

Oh @iluuu1994 I have some thought on this:

Since using zend_string_concat2 or similar APIs forces the allocation of an actual zend_string (which is unnecessary when we only need a temporary char* for standard C library calls like PQputCopyData, and this case is quite common in the codebase), would it make sense to introduce a dedicated inline helper in the Zend API for this specific pattern?

In specific, I mean something like zend_str_concat_to_raw or a more specific zend_str_append_char_to_raw to get clean readability without the offsetof(zend_string, val) overhead.

It would be like:

static zend_always_inline char *zend_str_append_char_to_raw(const char *str, size_t len, char c) {
    char *res = (char *)emalloc(len + 2);
    memcpy(res, str, len);
    res[len] = c;
    res[len + 1] = '\0';
    return res;
}

static zend_always_inline char *zend_str_concat_to_raw(const char *s1, size_t len1, const char *s2, size_t len2) {
    char *res = (char *)emalloc(len1 + len2 + 1);
    memcpy(res, s1, len1);
    memcpy(res + len1, s2, len2);
    res[len1 + len2] = '\0';
    return res;
}

So in this case we could rewrite this in

if (ZSTR_LEN(tmp) > 0 && ZSTR_VAL(tmp)[ZSTR_LEN(tmp) - 1] != '\n') {
    char *zquery = zend_str_append_char_to_raw(ZSTR_VAL(tmp), ZSTR_LEN(tmp), '\n');
    result = PQputCopyData(pgsql, zquery, ZSTR_LEN(tmp) + 1);
    efree(zquery);
}

which could obtain both readability and performance?

[iluuu1994]

No strong opinions, though I'd keep this local to the given file. @devnexen can make the final call as codeowner.

[devnexen]

@LamentXU123 if you can find another usage outside of the extension then why not, otherwise no need to create new helpers but the +1 removal change itself is ok.

[LamentXU123]

@LamentXU123 if you can find another usage outside of the extension then why not,

This pattern is quite common. For example in

php-src/ext/standard/fsock.c

Lines 26 to 46 in f4a2e33

	static size_t php_fsockopen_format_host_port(char **message, const char *prefix, size_t prefix_len,
	const char *host, size_t host_len, zend_long port)
	{
	char portbuf[32];
	int portlen = snprintf(portbuf, sizeof(portbuf), ":" ZEND_LONG_FMT, port);
	size_t total_len = prefix_len + host_len + portlen;
	char *result = emalloc(total_len + 1);
	if (prefix_len > 0) {
	memcpy(result, prefix, prefix_len);
	}
	memcpy(result + prefix_len, host, host_len);
	memcpy(result + prefix_len + host_len, portbuf, portlen);
	result[total_len] = '\0';
	*message = result;
	return total_len;
	}

could be changed to

static size_t php_fsockopen_format_host_port(char **message, const char *prefix, size_t prefix_len,
    const char *host, size_t host_len, zend_long port)
{
    char portbuf[32];
    int portlen = snprintf(portbuf, sizeof(portbuf), ":" ZEND_LONG_FMT, port);
    char *tmp_prefix_host = zend_str_concat_to_raw(prefix, prefix_len, host, host_len);
    *message = zend_str_concat_to_raw(tmp_prefix_host, prefix_len + host_len, portbuf, portlen);
    efree(tmp_prefix_host);

    return prefix_len + host_len + portlen;
}

but the +1 removal change itself is ok.

Thanks, then we can first merge this. I think it will be great for the new API to be discussed in a separate PR ;)

[iluuu1994]

Thanks!