Обсуждение: Size vs size_t or, um, PgSize?
Hi,
--
I've run into an issue of a name clash with system libraries. Specifically, the `Size` type seems to be just an alias for `size_t` and, at least on macOS, it clashes with the core SDK, as it is also defined by MacTypes.h, which is used by some of the libraries one may want to use from within a Postgres extension.
While in my case, I believe I have a workaround, I couldn't find any rationale as to why we might want to have this alias and not use size_t. Any insight on this would be appreciated.
Would there be any sense in changing it all to size_t or renaming it to something else?
I understand that they will break some extensions, so if we don't want them to have to go through with the renaming, can we enable backward compatibility with a macro?
If there's a willingness to try this out, I am happy to prepare a patch.
Y.
> On 3 Jul 2023, at 20:32, Yurii Rashkovskii <yrashk@gmail.com> wrote: > I couldn't find any rationale as to why we might want to have this alias and not use size_t. Any insight on this wouldbe appreciated. This used to be a typedef for unsigned int a very long time ago. > Would there be any sense in changing it all to size_t or renaming it to something else? > > I understand that they will break some extensions, so if we don't want them to have to go through with the renaming, canwe enable backward compatibility with a macro? > > If there's a willingness to try this out, I am happy to prepare a patch. This has been discussed a number of times in the past, and the conclusion from last time IIRC was to use size_t for new code and only change the existing instances when touched for other reasons to avoid churn. -- Daniel Gustafsson
On Tue, Jul 4, 2023 at 6:46 AM Daniel Gustafsson <daniel@yesql.se> wrote: > > On 3 Jul 2023, at 20:32, Yurii Rashkovskii <yrashk@gmail.com> wrote: > > If there's a willingness to try this out, I am happy to prepare a patch. > > This has been discussed a number of times in the past, and the conclusion from > last time IIRC was to use size_t for new code and only change the existing > instances when touched for other reasons to avoid churn. One such earlier discussion: https://www.postgresql.org/message-id/flat/CAEepm%3D1eA0vsgA7-2oigKzqg10YeXoPWiS-fCuQRDLwwmgMXag%40mail.gmail.com I personally wouldn't mind if we just flipped to standard types everywhere, but I guess it wouldn't help with your problem with extensions on macOS as you probably also want to target released branches, not just master/17+. But renaming in the back branches doesn't sound like something we'd do...
Hi Thomas,
Of course, it would have been great to have it backported in the ideal world, but it isn't realistic, as you say.
On Mon, Jul 3, 2023 at 12:03 PM Thomas Munro <thomas.munro@gmail.com> wrote:
On Tue, Jul 4, 2023 at 6:46 AM Daniel Gustafsson <daniel@yesql.se> wrote:
> > On 3 Jul 2023, at 20:32, Yurii Rashkovskii <yrashk@gmail.com> wrote:
> > If there's a willingness to try this out, I am happy to prepare a patch.
>
> This has been discussed a number of times in the past, and the conclusion from
> last time IIRC was to use size_t for new code and only change the existing
> instances when touched for other reasons to avoid churn.
One such earlier discussion:
https://www.postgresql.org/message-id/flat/CAEepm%3D1eA0vsgA7-2oigKzqg10YeXoPWiS-fCuQRDLwwmgMXag%40mail.gmail.com
I personally wouldn't mind if we just flipped to standard types
everywhere, but I guess it wouldn't help with your problem with
extensions on macOS as you probably also want to target released
branches, not just master/17+. But renaming in the back branches
doesn't sound like something we'd do...
Of course, it would have been great to have it backported in the ideal world, but it isn't realistic, as you say.
That being said, going ahead with the global renaming of Size to size_t will mostly eliminate this clash in, say, five years when old versions will be gone. At least it'll be fixed then. Otherwise, it'll never be fixed at all. To me, having the problem gone in the future beats having the problem forever.
--
Y.
> On 3 Jul 2023, at 21:14, Yurii Rashkovskii <yrashk@gmail.com> wrote: > That being said, going ahead with the global renaming of Size to size_t will mostly eliminate this clash in, say, fiveyears when old versions will be gone. At least it'll be fixed then. Otherwise, it'll never be fixed at all. To me, havingthe problem gone in the future beats having the problem forever. I would also like all Size instances gone, but the cost during backpatching will likely be very high. There are ~1300 or so of them in the code, and that's a lot of potential conflicts during the coming 5 years of backpatches. -- Daniel Gustafsson
Daniel,
On Mon, Jul 3, 2023 at 12:20 PM Daniel Gustafsson <daniel@yesql.se> wrote:
> On 3 Jul 2023, at 21:14, Yurii Rashkovskii <yrashk@gmail.com> wrote:
> That being said, going ahead with the global renaming of Size to size_t will mostly eliminate this clash in, say, five years when old versions will be gone. At least it'll be fixed then. Otherwise, it'll never be fixed at all. To me, having the problem gone in the future beats having the problem forever.
I would also like all Size instances gone, but the cost during backpatching
will likely be very high. There are ~1300 or so of them in the code, and
that's a lot of potential conflicts during the coming 5 years of backpatches.
I understand. How about a workaround for extension builders? Something like
```
/* Use this if you run into Size type redefinition */
#ifdef DONT_TYPEDEF_SIZE
#define Size size_t
#else
typedef size_t Size;
#endif
```
Not beautiful, but better than freezing the status quo forever?
--
Y.
Daniel Gustafsson <daniel@yesql.se> writes:
> On 3 Jul 2023, at 20:32, Yurii Rashkovskii <yrashk@gmail.com> wrote:
>> I couldn't find any rationale as to why we might want to have this alias and not use size_t. Any insight on this
wouldbe appreciated.
> This used to be a typedef for unsigned int a very long time ago.
I'm fairly sure that Size dates from before we could expect the system
headers to provide size_t everywhere.
> This has been discussed a number of times in the past, and the conclusion from
> last time IIRC was to use size_t for new code and only change the existing
> instances when touched for other reasons to avoid churn.
Yeah. The code-churn costs of s/Size/size_t/g outweigh the possible
gain, at least from our admittedly project-centric point of view.
But I don't have a whole lot of sympathy for arguments about "this
other code I'd like to also use has its own definition for Size",
because you could potentially make that complaint about just about
every typedef we've got. If you have conflicts like that, you have
to resolve them by methods like #define hacks or factoring your code
so it doesn't need to include Postgres headers in the same files that
include $other-project-headers.
regards, tom lane