Обсуждение: Example 43.6. A PL/pgSQL Trigger Function for Maintaining a Summary Table

Hello!

Example 43.6 on the page https://www.postgresql.org/docs/16/plpgsql-trigger.html has two tables called main. But it seems that the table time_dimension is not used any further, it's a bit strange for one of main tables of the example. Am I missing something, or should this example be extended (or reduced)?

> On 26 Oct 2023, at 09:33, Maxim Yablokov <m.yablokov@postgrespro.ru> wrote:

> Example 43.6 on the page https://www.postgresql.org/docs/16/plpgsql-trigger.html has two tables called main. But it
seemsthat the table time_dimension is not used any further, it's a bit strange for one of main tables of the example.
AmI missing something, or should this example be extended (or reduced)? 

Interesting catch, time_dimension has been unused since introduced with commit
a294726bc1d (in 2005), and the archives doesn't provide any further insights.
While I don't have access to the book referred to for the example, we don't
expect our readers to either so it seems to me that we could remove it from the
example to avoid risk of confusion. Would you like to propose a patch for this?

--
Daniel Gustafsson

Yeah, sure thing! Thanks! I'll send a patch soon.

On 26 Oct 2023, at 09:33, Maxim Yablokov <m.yablokov@postgrespro.ru> wrote:

Example 43.6 on the page https://www.postgresql.org/docs/16/plpgsql-trigger.html has two tables called main. But it seems that the table time_dimension is not used any further, it's a bit strange for one of main tables of the example. Am I missing something, or should this example be extended (or reduced)?

Interesting catch, time_dimension has been unused since introduced with commit
a294726bc1d (in 2005), and the archives doesn't provide any further insights.
While I don't have access to the book referred to for the example, we don't
expect our readers to either so it seems to me that we could remove it from the
example to avoid risk of confusion. Would you like to propose a patch for this?

--
Daniel Gustafsson

I believe that the attached patch should fix this problem. Please have a look.

On 26 Oct 2023, at 09:33, Maxim Yablokov <m.yablokov@postgrespro.ru> wrote:

Example 43.6 on the page https://www.postgresql.org/docs/16/plpgsql-trigger.html has two tables called main. But it seems that the table time_dimension is not used any further, it's a bit strange for one of main tables of the example. Am I missing something, or should this example be extended (or reduced)?

Interesting catch, time_dimension has been unused since introduced with commit
a294726bc1d (in 2005), and the archives doesn't provide any further insights.
While I don't have access to the book referred to for the example, we don't
expect our readers to either so it seems to me that we could remove it from the
example to avoid risk of confusion. Would you like to propose a patch for this?

--
Daniel Gustafsson

> On 26 Oct 2023, at 12:31, Maxim Yablokov <m.yablokov@postgrespro.ru> wrote:
>
> I believe that the attached patch should fix this problem. Please have a look.

Thanks, I'll await pushing and backpatching if Tom who committed it has
insights into whether it was missed or if it indeed serves a purpose.  If not I
think it should be backpatched all the way since it exists for all supported
versions.

--
Daniel Gustafsson

Daniel Gustafsson <daniel@yesql.se> writes:
> Thanks, I'll await pushing and backpatching if Tom who committed it has
> insights into whether it was missed or if it indeed serves a purpose.

Hey, I just pushed that for somebody else, I don't claim authorship ;-)

It seems clear that the example intends to show a star-schema database
where the fact table refers to various dimension tables.  But it's
incomplete --- there's no foreign-key constraint on time_key, and
even less infrastructure for product_key or store_key.  I don't
have the cited book either, so I don't know how complete the original
example was.  Perhaps the bit in the trigger function about forbidding
updates to time_key has something to do with that model.

Anyway, I don't see any reason to object to this patch.  The extra
table isn't adding much.  My only thought is would it make sense to
change time_key to be a timestamp or timestamptz value?

            regards, tom lane